-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[chore]: fix go routine leaks in tests #34729
Changes from 7 commits
510b63c
31f2f94
4a110b5
7038252
24a5347
b66bb56
987725e
031d09f
5e1cc9d
8c2e8bf
ea73141
381a384
8033ee0
152ba83
cdd6248
0621aff
ee13a00
df890a7
88a46d5
99fc3c2
752375a
756885f
f4b00b8
073cda8
99c1332
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Why is this option removed?
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.
enabling the option led to re-running the failed tests (which mostly failed to to go routine leaks and race conditions), which always led to a passed test. This behavior was from some reason blocking the actual problems which were shown after it was removed
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.
I added this in #31253 because we have a problem with flaky tests and not enough resources to deal with them. I feel like this is still the case and that if we remove this we will get a more flaky CI which is detrimental to contributors. Is there any way we can detect leaks while still re-running other unit tests?
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.
I am not aware of any possibility here. In fact it seems very strange to me that all of the failing tests become passed after re-running them for the 2. time.
We can leave the re-run option in place until we find the root cause why the re-executed tests have very deterministic behavior in terms of go-leaks and data-races, but we should at least consider merging the fixed tests as part of this PR.
Maybe it can be a caching issue in the gotestsum wrapper, not sure here...
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.
Can you report this upstream on the gotestsum repository then to see if it's actually an issue with them?
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.
reported here
But there seems to be a bigger problem with the
--rerun-fails
option, as I see multiple issues created on the upstream related to it https://github.com/gotestyourself/gotestsum/issuesThere 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.
cc @mx-psi
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.
I subscribed to gotestyourself/gotestsum/issues/442 and I would like to wait a bit more to see if upstream has anything to say here. I see some issues related to
--rerun-fails
, but I don't see any specifically related to this.I am happy to merge the rest of the changes and only keep
--rerun-fails
to a separate PR, would that work for you?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.
ok, reverted the
--rerun-fails
and will keep an eye on the issue opened on gotestsum upstream.When it will be resolved, will open a new PR