Enable method-level parallelization for true unit test projects - #55090
Conversation
The repo-wide default (test/Directory.Build.props) sets MSTestParallelizeScope to None, fully serializing every MSTest project. The containerize.UnitTests and Microsoft.NET.Build.Containers.UnitTests projects are genuine unit tests with no shared process-global state, so opt them back in to MethodLevel parallelization. The two classes in Containers.UnitTests that mutate process-global environment variables (AuthHandshakeMessageHandlerTests, DockerDaemonTests) are marked [DoNotParallelize] so they run serially under the method-level pool. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request opts two MSTest.Sdk-based unit test projects back into method-level parallel execution by overriding the repo-wide MSTestParallelizeScope=None default, and adds [DoNotParallelize] to the few test classes that mutate process-global environment variables.
Changes:
- Enable
MSTestParallelizeScope=MethodLevelfortest/containerize.UnitTests. - Enable
MSTestParallelizeScope=MethodLevelfortest/Microsoft.NET.Build.Containers.UnitTests. - Mark environment-variable-mutating test classes as
[DoNotParallelize]in the containers unit test project.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.NET.Build.Containers.UnitTests/Microsoft.NET.Build.Containers.UnitTests.csproj | Opt the project into MSTest method-level parallelization via MSTestParallelizeScope=MethodLevel. |
| test/Microsoft.NET.Build.Containers.UnitTests/DockerDaemonTests.cs | Add [DoNotParallelize] to avoid concurrency issues due to DOCKER_HOST mutation. |
| test/Microsoft.NET.Build.Containers.UnitTests/AuthHandshakeMessageHandlerTests.cs | Add [DoNotParallelize] to avoid concurrency issues due to registry-related env var mutations. |
| test/containerize.UnitTests/containerize.UnitTests.csproj | Opt the project into MSTest method-level parallelization via MSTestParallelizeScope=MethodLevel. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 2
Address review feedback: capture and restore process-global environment variables in a finally block so a thrown assertion or a value already set on the test host cannot leak into subsequent tests. - GetDockerCredentialsFromEnvironment_ReturnsCorrectValues now restores the registry credential vars in finally. - Authenticate now captures and restores REGISTRY_AUTH_FILE. - DockerDaemonTests restores the original DOCKER_HOST instead of clearing it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Is it reasonable to request more than one validation run prior to merging to validate stability in the affected tests? |
Absolutely! I'll ping when I have 5 passing runs |
|
Round 3 |
|
Round 5, please @MichaelSimons review/approve if this gets green again |
nohwnd
left a comment
There was a problem hiding this comment.
Would have been nice to inject the env variables only into the child process and not to the current one, but the called tools (like dockerCli) don't allow it and are production code (even though internal class). So good to go.
What
Opt two genuine unit-test projects back in to method-level test parallelization:
test/containerize.UnitTeststest/Microsoft.NET.Build.Containers.UnitTestsThe repo-wide default in
test/Directory.Build.propssetsMSTestParallelizeScope=None, which emits[assembly: DoNotParallelize]and fully serializes every MSTest project. These two projects are true unit tests (no disk/CWD dependence, no shared process-global state), so they can safely run methods in parallel.Why it's safe
I audited both projects for shared-state hazards (environment variables, current-directory mutation, fixed temp paths, mutable statics):
containerize.UnitTests — single class, pure parsing tests; temp dirs are keyed per test. No shared state.
Microsoft.NET.Build.Containers.UnitTests — the only real hazard is two classes that mutate process-global environment variables. Those are now marked
[DoNotParallelize]so they run in MSTest's serial phase:AuthHandshakeMessageHandlerTests(registry credential vars +REGISTRY_AUTH_FILE)DockerDaemonTests(DOCKER_HOST)The rest use fakes (
RegistryTests'SetEnvironmentVariablethrows), read-only statics, and per-testHttpClientinstances.Verification
Both projects build clean (0 warnings, with
MSTestAnalysisMode=Recommendedactive) and the generatedAssemblyInfoemits[assembly: Parallelize(Scope = MethodLevel)]. Full test execution runs in CI (the repo's self-built runtime isn't available locally).