fix(flaky): ensure that the space is always unblocked for its deletion #1090
Conversation
MatousJobanek
requested review from
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19 and
mfrancisc
as code owners
| @@ -77,6 +77,20 @@ func TestCreateSpace(t *testing.T) { | |||
| // then | |||
| VerifyResourcesProvisionedForSpace(t, awaitilities, space.Name) | |||
|
|
|||
| // ensure that the space is reset back to the original (valid) target cluster | |||
| // so the cleanup logic can delete the Space | |||
| var resetOnce sync.Once | |||
There was a problem hiding this comment.
Hm... what's the point of using Once here? resetOnce.Do() will be called a single time only (for this specific resetOnce instance) anyway. What am I missing?
There was a problem hiding this comment.
we need to ensure that this reset is called only once - either as part of the sub-test or at the end of the test. If it happens that the sub-test (that resets the target cluster) is called, then the space would be deleted - see:
toolchain-e2e/test/e2e/parallel/space_test.go
Lines 112 to 120 in de8dd00
so the space wouldn't be present anymore
There was a problem hiding this comment.
or were you were asking why do I define the variable resetOnce outside of the function? Well, the variable has to stay outside, so when the function is called, it always uses the same instance of the Once. If it was moved inside of the function, then it would create a new instance of Once for every call of the function
There was a problem hiding this comment.
should not be a problem - but should we move this definition in the parent test TestCreateSpace ( where we define awaitilities , hostAwait, memberAwait ) ? Just to be on the safe side.
There was a problem hiding this comment.
What I was missing is that the reset() was called twice (not once!). Once in the defer and the second time in the sub-test bellow. I saw it only in called by defer and was wondering why do you need once if it's already called only once :)
Sorry for the confusion.
There was a problem hiding this comment.
should we move this definition in the parent test TestCreateSpace ( where we define awaitilities , hostAwait, memberAwait ) ? Just to be on the safe side.
not really, it has to stay in the same (sub)test where the space is provisioned, so it's tied to the actual space (and also the the function that executes this sub-test)
There was a problem hiding this comment.
not really, it has to stay in the same (sub)test where the space is provisioned, so it's tied to the actual space
makes sense , thanks for explaining that 👍
|
| @@ -77,6 +77,20 @@ func TestCreateSpace(t *testing.T) { | |||
| // then | |||
| VerifyResourcesProvisionedForSpace(t, awaitilities, space.Name) | |||
|
|
|||
| // ensure that the space is reset back to the original (valid) target cluster | |||
| // so the cleanup logic can delete the Space | |||
| var resetOnce sync.Once | |||
There was a problem hiding this comment.
should not be a problem - but should we move this definition in the parent test TestCreateSpace ( where we define awaitilities , hostAwait, memberAwait ) ? Just to be on the safe side.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, fbm3307, MatousJobanek, mfrancisc, rajivnathan 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 |
make sure that the space is unblocked for the cleanup, even for the cases when the test execution is stopped in the middle and the sub-test (that is supposed to unblock the space) is not triggered: https://redhat-internal.slack.com/archives/C06MJ2DVBU4/p1736342796950679?thread_ts=1736333943.812749&cid=C06MJ2DVBU4