Refactor e2e test setup #264
Conversation
|
This is still WIP, but pushing what I have since I got a clean e2e run locally with it. I'll amend this PR to remove unused code and clean things up. |
rhmdnd
force-pushed
the
refactor-e2e-test-setup
branch
2 times, most recently
from
635f0a6
to
ac883de
Compare
| @@ -95,3 +156,282 @@ func (f *Framework) waitForScanCleanup() error { | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func (f *Framework) addFrameworks() error { | |||
There was a problem hiding this comment.
Note for reviewers.
These functions are pretty close to what existed in the helpers.go file, but the method signatures are different since we don't need to pass in additional things like namespaces or the framework when we have all that data associated to the framework already.
tests/e2e/helpers.go
Outdated
| @@ -257,33 +241,6 @@ func executeTests(t *testing.T, tests ...testExecution) { | |||
| defer mcTctx.cleanupTrackedInvalidPools() | |||
| defer mcTctx.cleanupTrackedPools() | |||
There was a problem hiding this comment.
These are only used in a couple of tests, but once we remove them here, this method won't contain any setup or teardown code. That will allows us to break the tests into separate suites for serial and parallel tests, and remove the custom runner.
There was a problem hiding this comment.
After doing some more digging, we discovered these are lazy loaded so they don't reboot machines in the pool when they're created.
After testing with 4.12, those machine config pools don't cause reboots when they're created. We can either create them in setup, regardless of their use, or we can create them in the tests still.
rhmdnd
force-pushed
the
refactor-e2e-test-setup
branch
7 times, most recently
from
12c0f7e
to
8f06eab
Compare
|
|
||
| func TestMain(m *testing.M) { | ||
| f := framework.NewFramework() | ||
| err := f.SetUp() |
There was a problem hiding this comment.
Do we use the same e2e pool for all the test?
There was a problem hiding this comment.
From the perspective or parallel tests, yes I believe so.
| @@ -67,7 +168,7 @@ func (f *Framework) setUp() error { | |||
| // deleting the cluster-wide resources, like roles, service accounts, or the deployment. | |||
| // If we don't properly cleanup resources before deleting CRDs, it leaves resources in a | |||
| // terminating state, making them harder to cleanup. | |||
| func (f *Framework) tearDown() error { | |||
| func (f *Framework) TearDown() error { | |||
| // Make sure all scans are cleaned up before we delete the CRDs. Scans should be cleaned up | |||
| // because they're owned by ScanSettingBindings or ScanSuites, which should be cleaned up | |||
| // by each individual test either directly or through deferred cleanup. If the test fails | |||
There was a problem hiding this comment.
I am wondering if we should clean up in a sequence of ssb, tp, scan, and then scansuite?
There was a problem hiding this comment.
Through out the series of patches, I added individual clean up to each test so they clean up after themselves.
I don't install other resource except what's required for the tests when I use the e2e target, but if someone did then running the tests might conflict, or cleanup unintended resources (e.g., deploy-local and e2e on the same cluster?)
Do you use a different workflow?
There was a problem hiding this comment.
I think that makes sense! thanks for explaining it
|
Thanks for making this patch for our e2e tests, this looks really good! |
rhmdnd
force-pushed
the
refactor-e2e-test-setup
branch
from
beb3dce
to
3735b89
Compare
|
/test e2e-aws-parallel |
rhmdnd
force-pushed
the
refactor-e2e-test-setup
branch
from
3735b89
to
7d7bb97
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhmdnd, Vincent056 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
rhmdnd
force-pushed
the
refactor-e2e-test-setup
branch
from
7d7bb97
to
ffd0ecd
Compare
|
Rebased to pick up a fix for the #277 - which also caused the merge conflict. |
|
/lgtm |
|
/lgtm |
We recently made a change to ensure we clean up resources properly after
we perform e2e tests. Specifically to avoid race conditions that leaves
the compliance operator resources in a bricked state, even when the
tests pass.
This commit uses a similar pattern for test setup by leveraging the
setUpmethod before callingm.Run()to make sure the cluster isready before we run the actual tests.
One advantage is that we're simplifying the logic to a single layer
before the tests run.
Further changes will make it easier to decouple the framework, context,
and e2eutils into a single set of utilities we can share across
repositories if needed.