Cover the fixture-scope branches of MSTEST0075 - #10383
Conversation
There was a problem hiding this comment.
Pull request overview
Adds analyzer edge-case coverage for MSTEST0075 and MSTEST0081 without changing production code.
Changes:
- Tests fixture-method handling and code-fix placement for current-directory mutations.
- Tests duplicate generic and non-generic filter-provider registrations, including CS0579 behavior.
Show a summary per file
| File | Description |
|---|---|
CurrentDirectoryMutationUnderParallelizationAnalyzerTests.cs |
Covers test and assembly initialization scenarios. |
TestFilterProviderShouldBeValidAnalyzerTests.cs |
Covers same-shape duplicate provider registrations. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Summary
This PR adds well-crafted edge-case tests for two analyzers (MSTEST0075 and MSTEST0081), closing three backlog items. No production code is changed.
Review
✅ No issues found. This is a clean, test-only PR that strengthens analyzer coverage.
Highlights
| Dimension | Assessment |
|---|---|
| Correctness | ✅ Tests correctly exercise the fixture hierarchy ([TestInitialize] → diagnostic, [AssemblyInitialize] → no diagnostic) and the AllowMultiple = false CS0579 interaction |
| Completeness | ✅ Code fix assertion included for the diagnostic case; both TFMs validated; #if NET guard correctly scopes the generic-attribute test |
| Naming & readability | ✅ Method names follow the When..._Outcome convention; inline comments explain why each expectation holds |
| Test quality | ✅ No flakiness vectors; assertions are specific (rule + location + arguments); no over-assertion |
| Style | ✅ Consistent with sibling test files; no trailing whitespace; file ends with newline |
| Public API | ✅ N/A — no production changes |
| Security | ✅ N/A |
Minor observation (non-blocking)
The "/tmp" literal in the test snippets is fine for Roslyn in-memory compilation (no real I/O), but if future tests ever execute these snippets end-to-end, a platform-neutral path would be safer. Not actionable here.
Verdict: No changes requested. Ship it. 🚀
This comment has been minimized.
This comment has been minimized.
CurrentDirectoryMutationUnderParallelizationAnalyzer decides per enclosing method whether to report a current-directory mutation and, if so, where the AddResourceLockFixer may place [ResourceLock]. That produces three outcomes, none of which had coverage in this file - every existing test here pins the plain test-method case. Add one test per outcome: - WhenTestInitializeSetsCurrentDirectoryViaDirectory_Diagnostic - [TestInitialize] is a class-scoped fixture, so the mutation is reported and the fix is placed on the *class*, which is where discovery reads resource locks from. This is the first class-scope fix assertion in this file. - WhenAssemblyInitializeSetsCurrentDirectoryViaDirectory_NoDiagnostic - [AssemblyInitialize] is serialized ahead of every worker, so it cannot race a concurrent test and must stay silent. - WhenGlobalTestInitializeSetsCurrentDirectoryViaDirectory_DiagnosticWithoutFix - [GlobalTestInitialize] genuinely races, but a global fixture has no effective lock target (the enclosing class is not one, since locking it would serialize only that class's tests while the fixture kept racing every other class), so the analyzer reports without offering a fix. All three are mutation-sensitive: adding AssemblyInitialize to GetFixtureAttributeSymbols, or GlobalTestInitialize to GetClassScopedFixtureAttributeSymbols, each fails exactly one of them, and no pre-existing test in the repo notices either regression. Closes #10349 Closes #10369 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 38d37b0e-1f72-43cd-923a-1733b2964873
d3bd100 to
4f75f2d
Compare
🧪 Test quality grade — PR #10383
This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Re-run with
|
Fills the 'diagnostic without fix' branch gap for global fixtures in UndeclaredProcessGlobalStateMutationAnalyzer (MSTEST0074) and CultureMutationUnderParallelizationAnalyzer (MSTEST0076), mirroring the GlobalTestInitialize coverage already present in CurrentDirectoryMutationUnderParallelizationAnalyzerTests.cs (MSTEST0075, PR #10383). [GlobalTestInitialize] is present in GetFixtureAttributeSymbols but absent from GetClassScopedFixtureAttributeSymbols, so GetResourceLockFixScope returns null and no fix is offered even though the diagnostic still fires. Neither analyzer's test file previously exercised this branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #10349
Closes #10369
(#10349 and #10369 are duplicates of each other — the test-improver workflow filed the same gap twice.)
What this covers
CurrentDirectoryMutationUnderParallelizationAnalyzerdecides, per enclosing method, whether to report a current-directory mutation and — if so — whereAddResourceLockFixermay place[ResourceLock]. That yields three distinct outcomes, and this file covered none of them: every pre-existing test here pins the plain test-method case.WhenTestInitializeSetsCurrentDirectoryViaDirectory_Diagnostic[TestInitialize]WhenAssemblyInitializeSetsCurrentDirectoryViaDirectory_NoDiagnostic[AssemblyInitialize]WhenGlobalTestInitializeSetsCurrentDirectoryViaDirectory_DiagnosticWithoutFix[GlobalTestInitialize]Notes on each:
[TestInitialize]is a class-scoped fixture, so a[ResourceLock]on the fixture method itself would be ignored — discovery reads locks only from the test class and the test method. The test asserts the fixer's class-scope placement, which is the first class-scope fix assertion in this file. (The siblingCultureMutationUnderParallelizationAnalyzerTestsonly callsVerifyAnalyzerAsync, so it cannot pin this at all.)[AssemblyInitialize]is serialized ahead of every worker, so it cannot race a concurrent test — flagging it would be a false positive.[GlobalTestInitialize]genuinely races, but a global fixture has no effective lock target: the enclosing class is not one, because locking it would serialize only that class's tests while the fixture kept racing the tests of every other class. So the analyzer reports without offering a fix. This third branch had no coverage in any of the three parallel-safety analyzer test files.Mutation-verified
These are not decorative — each was checked by breaking the analyzer and confirming exactly one test caught it, with no pre-existing test in the repo noticing:
AssemblyInitializeAttributetoGetFixtureAttributeSymbols[AssemblyInitialize]test failsGlobalTestInitializeAttributetoGetClassScopedFixtureAttributeSymbols[GlobalTestInitialize]test failsThe second is the interesting one: that regression would make the fixer emit a class-level
[ResourceLock]that does nothing at run time, and before this PR nothing would have caught it.Validation
Full
MSTest.Analyzers.UnitTestssuite: net8.0 1688/1688, net472 1632/1632. Build clean, 0 warnings. No production code changed.The MSTEST0081 half of this backlog is split out into #10386, since it covers a different analyzer and issue.