-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deflake some tests - 2020-05-27 edition #8060
Conversation
Fix some docker test flakiness where image cleanup process may contaminate other tests. A clean up process may attempt to delete an image while it's used by another test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Left a couple notes where we could be expressing the test intent better if you want.
nomad/node_endpoint_test.go
Outdated
case structs.EvalStatusPending, structs.EvalStatusComplete: | ||
// success | ||
default: | ||
require.Equal(t, structs.EvalStatusPending, eval.Status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we saying EvalStatusPending
or EvalStatusComplete
are the success case, do you think this would be more clear (and provide more accurate error messages) if we used assert.Fail
here instead?
nomad/drainer_int_test.go
Outdated
require.Len(node.Events, 3) | ||
|
||
// sometimes test gets a duplicate node drain complete event | ||
if len(node.Events) < 3 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use require.GreaterOrEqual
to make the intent more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:TIL: Agreed.
c6c8bbe
to
74af591
Compare
Deflake some tests - 2020-05-27 edition
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Deflake some tests that I noticed recently. Best reviewed on commit by commit basis:
In high level, I noticed the following:
Fixed this by ensuring that background goroutines are cancelled after the driver context is cancelled. Also, default to having tests run with disabled GC.
Some CSI tests expect the nodes to be in the Server State Store, but block until a connection is established. This results into spurious failures such as ones found in https://circleci.com/gh/hashicorp/nomad/69646 . Notice how the failure is reported 1ms after "client: node registration complete" log line.
Weird sparious failures in
api
package like ones in https://circleci.com/gh/hashicorp/nomad/69688 , where the test pass by gets reported as a failure. My hunch is that it fails because the nomad process is killed before the log newline is flushed and gotestsum get confused. I'll report this to gotestsum developer. Here, I ensure that the process is killed gracefully first.