[RSDK-782] Flaky tests: TestSingleOperationManager in operation manager test go #1610
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
viambot
added
the
safe to test
penguinland
reviewed
Nov 22, 2022
operation/manager_test.go
Outdated
| @@ -105,44 +129,65 @@ func TestSingleOperationManager(t *testing.T) { | |||
| <-c | |||
|
|
|||
| som.CancelRunning(ctx) | |||
| test.That(t, ctx.Err(), test.ShouldNotBeNil) | |||
| if ctx.Err() == nil { | |||
| t.Skip("skipping test since context was not cancelled, no race condition started") | |||
There was a problem hiding this comment.
Not sure how to reconcile som.CancelRunning(ctx) with "context was not cancelled." Are you sure about this? I had expected the flaky test to be fixed by waiting for the SOM to finish cancelling the thing before asserting that it was cancelled, but it kinda looks like we continue to have a race condition and are just ignoring the times when the wrong thing wins the race. but maybe I've misunderstood what's going on here?
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
edaniels
reviewed
Nov 23, 2022
operation/manager_test.go
Outdated
| ctx := context.Background() | ||
| som := SingleOperationManager{} | ||
| var ( | ||
| ctx = context.Background() |
There was a problem hiding this comment.
this context should be within tests. it's better practice to not have these two globals
There was a problem hiding this comment.
Okay, I'll move them back. Why is is better practice not to have them global?
There was a problem hiding this comment.
Expanding the answer I gave below: if they're global and reused in lots of tests, a failure in test A can leave it in a bad state, causing a failure in test B. and now you're stuck trying to figure out what's wrong with test B, and the answer is nothing at all, that the state was corrupted by test A, and it's really hard to figure that out because B is one of the tests that's failing so surely there's something wrong with B.
By not reusing state between tests, you keep them independent of each other so that the failures are limited to the tests that actually have problems.
There was a problem hiding this comment.
so context is probably fine as a global since it's immutable but context in general is to be used on a request by request basis (it's request/api context)
op manager is very dangerous as global because of what Alan mentioned, shared state
penguinland
reviewed
Nov 23, 2022
viambot
added
safe to test
penguinland
approved these changes
Nov 23, 2022
| @@ -176,5 +187,5 @@ func (m *mock) stopFail(ctx context.Context, extra map[string]interface{}) error | |||
| } | |||
|
|
|||
| func (m *mock) IsPowered(ctx context.Context, extra map[string]interface{}) (bool, float64, error) { | |||
| return false, 0, nil | |||
| return true, 1, nil | |||
There was a problem hiding this comment.
One more time: you're sure this line needed changing? I don't see a call to IsPowered anywhere in this file. which doesn't mean it's not used, just that it's hard for me to see that.
…2-flaky-test-test-single-operation-manager-in-operation-manager-test-go
viambot
added
safe to test
randhid
changed the title
|
randhid
deleted the
RSDK-782-flaky-test-test-single-operation-manager-in-operation-manager-test-go
branch
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing out with 10000 counts on my machine right now. Feel free to ask me to recombine tests, separated them out in a pretty mindless way.