-
-
Notifications
You must be signed in to change notification settings - Fork 978
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
Fix async GlobalSetup/Cleanup with InProcessEmit toolchain #2109
Fix async GlobalSetup/Cleanup with InProcessEmit toolchain #2109
Conversation
a18e5c4
to
d7ceb27
Compare
d7ceb27
to
79ffa8d
Compare
c4a07b0
to
ac7210d
Compare
4618084
to
35948a1
Compare
35948a1
to
a0ebccf
Compare
a0ebccf
to
a84da10
Compare
a84da10
to
f27e83b
Compare
f27e83b
to
95c5485
Compare
95c5485
to
ea5639b
Compare
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.
LGTM, thank you for providing the fix @timcassell and apologies for not noticing the PR (due to the fact that I work on the .NET Team I get few dozens of GH notifications a day).
[InlineData(typeof(GlobalSetupCleanupTask))] | ||
[InlineData(typeof(GlobalSetupCleanupValueTask))] | ||
[InlineData(typeof(GlobalSetupCleanupValueTaskSource))] | ||
public void InProcessEmitToolchainSupportsSetupAndCleanup(Type benchmarkType) |
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.
So far this test was a [Fact]
(having a single test case), so we did not have to worry about multiple tests accessing the same static field at the same time.
As long as we keep the test parallelization disabled here:
"parallelizeAssembly": false, | |
"parallelizeTestCollections": false |
It's OK. But if we ever change our policy on that, multiple tests may update Counters
fields at the same time and make this test flaky.
There is no need to change anything as of now, just something to keep in mind when writing more tests in the future.
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.
According to the documentation,
By default, each test class is a unique test collection. Tests within the same test class will not run in parallel against each other.
So we should be good, regardless of that setting.
Fixes #1738 and fixes #2236.