[test-improver] test: add edge case tests for PreferTestCleanupOverDispose and PreferTestInitializeOverConstructor analyzers

[Amaury Levé (Evangelink)]

Goal and Rationale

Add 3 edge case tests across two related "prefer test lifecycle over C# lifecycle" analyzer test files (PreferTestCleanupOverDisposeAnalyzerTests and PreferTestInitializeOverConstructorAnalyzerTests). These tests cover real code paths through the analyzers that were previously untested.

Approach

PreferTestCleanupOverDisposeAnalyzerTests — 2 new tests:

1. WhenTestClassHasFullDisposePattern_OnlyParameterlessDisposeDiagnostic
   The full dispose pattern uses two methods: public void Dispose() (the IDisposable interface implementation) and protected virtual void Dispose(bool disposing) (the parameterized cleanup helper). The analyzer checks IsDisposeImplementation, which only matches the interface implementation. This test verifies:

   • Only Dispose() is flagged; Dispose(bool) is not
   • The fixer renames Dispose() to TestCleanup() and leaves Dispose(bool) intact

2. WhenTestClassHasBothDisposeAndTestCleanup_DiagnosticOnDispose
   When a test class has both a [TestCleanup] method and implements IDisposable.Dispose(), the analyzer should still flag Dispose() — the existence of TestCleanup does not suppress the diagnostic. This guards against the misconception that having a TestCleanup makes the Dispose pattern acceptable.

PreferTestInitializeOverConstructorAnalyzerTests — 1 new test:

3. WhenTestClassHasStaticConstructor_NoDiagnostic
   The analyzer targets only explicit parameterless instance constructors via InstanceConstructors.FirstOrDefault(x => !x.IsImplicitlyDeclared && x.Parameters.Length == 0). Roslyn's InstanceConstructors property excludes static constructors by design. This test verifies that a static constructor in a [TestClass] does not trigger the rule.

Test Results

All 14 tests pass (7 per file, 3 new + 11 existing):

Test run summary: Passed!
total: 14
failed: 0
succeeded: 14
skipped: 0
duration: ~18s

Command used:

.dotnet/dotnet test test/UnitTests/MSTest.Analyzers.UnitTests/MSTest.Analyzers.UnitTests.csproj \
-f net8.0 --no-build -c Debug \
--filter "FullyQualifiedName~PreferTestCleanupOverDisposeAnalyzerTests|FullyQualifiedName~PreferTestInitializeOverConstructorAnalyzerTests"

Trade-offs

• Minimal maintenance burden: tests cover clear, well-defined analyzer behavior with no hard-coded timing or environment sensitivity.
• The WhenTestClassHasBothDisposeAndTestCleanup_DiagnosticOnDispose test uses VerifyAnalyzerAsync (not VerifyCodeFixAsync) since applying the fixer would produce a second TestCleanup() method — a known limitation worth documenting through the test itself.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Test Improver workflow. · 1.8K AIC · ⌖ 40.8 AIC · ⊞ 58K · [◷]( · ◷)

Add this agentic workflows to your repo

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/test-improver.md@main

[Copilot]

Pull request overview

Adds additional unit-test coverage for MSTest.Analyzers rules that encourage MSTest lifecycle methods over C# lifecycle constructs, specifically targeting previously untested edge cases in the constructor and dispose analyzers.

Changes:

• Add a no-diagnostic test ensuring a [TestClass] static constructor is ignored by PreferTestInitializeOverConstructor.
• Add a code-fix test covering the full IDisposable pattern to ensure only Dispose() is flagged/fixed (not Dispose(bool)).
• Add an analyzer-only test ensuring Dispose() is still flagged even when a [TestCleanup] method already exists.

Show a summary per file

File	Description
test/UnitTests/MSTest.Analyzers.UnitTests/PreferTestInitializeOverConstructorAnalyzerTests.cs	Adds coverage that static constructors do not trigger the “prefer TestInitialize over ctor” rule.
test/UnitTests/MSTest.Analyzers.UnitTests/PreferTestCleanupOverDisposeAnalyzerTests.cs	Adds edge-case coverage for full dispose-pattern handling and for coexistence of [TestCleanup] + Dispose().

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 0

[Amaury Levé (Evangelink)]

🧪 Test quality grade — PR #9382

Δ	Test	Grade	Band	Notes
new	PreferTestCleanupOverDisposeAnalyzerTests. WhenTestClassHasFullDisposePattern_ OnlyParameterlessDisposeDiagnostic	B	80–89	Slightly long body (~46 lines) due to embedded source strings; all other dimensions are strong.
new	PreferTestCleanupOverDisposeAnalyzerTests. WhenTestClassHasBothDisposeAndTestCleanup_ DiagnosticOnDispose	A	90–100	No issues found.
new	PreferTestInitializeOverConstructorAnalyzerTests. WhenTestClassHasStaticConstructor_ NoDiagnostic	A	90–100	No issues found.

_{This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.}

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Grade Tests on PR (on open / sync) workflow. · 158.7 AIC · ⌖ 13.1 AIC · ⊞ 43.7K · [◷]( · ◷)

[Amaury Levé (Evangelink)]

Note

🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Verdict table

#	Dimension	Status	Notes
1	Algorithmic Correctness	✅	Tests target the correct code paths; fixed-code expectations match fixer output
2	Threading & Concurrency	N/A	Test-only change
3	Security & IPC Contract Safety	N/A	—
4	Public API & Binary Compatibility	N/A	No public API touched
5	Performance & Allocations	N/A	—
6	Cross-TFM Compatibility	✅	New tests use IDisposable (not IAsyncDisposable), correctly placed outside the #if NET guard
7	Resource & IDisposable Management	N/A	—
8	Defensive Coding at Boundaries	N/A	—
9	Error Handling & Diagnostics	N/A	—
10	Test Coverage & Quality	⚠️	See notes below
11	Naming & Code Style	✅	Method names follow the WhenXxx_YyyVerb convention; consistent with the existing file
12	Documentation & Comments	✅	Inline comments are appropriate for non-obvious edge cases
13	Localization	N/A	—
14	Logging & Observability	N/A	—
15	Configuration & Feature Flags	N/A	—
16	Migration & Upgrade Safety	N/A	—
17	Dependency & Package Hygiene	N/A	—
18	CI/CD & Build Integration	N/A	—
19	Backward Compatibility	N/A	—
20	Type System & Nullability	N/A	—
21	Accessibility & Extensibility	N/A	—
22	PowerShell & Script Hygiene	N/A	—

---

Detailed findings

Test 1 — WhenTestClassHasFullDisposePattern_OnlyParameterlessDisposeDiagnostic

The fixedCode expectation faithfully represents what the fixer produces:

• IDisposable removed from the base-type list.
• Parameterless Dispose() renamed to [TestCleanup] TestCleanup() with its body preserved.
• protected virtual Dispose(bool) left untouched (the fixer only targets the flagged method).
• using System; correctly retained — GC.SuppressFinalize(this) in the fixed body still references System.GC.

One semantic oddity worth being aware of: after the fix, TestCleanup() calls GC.SuppressFinalize(this) even though the class no longer implements IDisposable and has no finalizer, making the call a no-op. This accurately documents the current fixer behaviour rather than a test defect, but it is a signal that the fixer could be smarter about stripping the suppress-finalizer idiom when removing IDisposable.

Test 2 — WhenTestClassHasBothDisposeAndTestCleanup_DiagnosticOnDispose

Using VerifyAnalyzerAsync correctly confirms the diagnostic fires. However, the fixer's behaviour in this scenario is untested. Applying the fixer here would:

1. Rename Dispose() → TestCleanup() (with [TestCleanup]).
2. Leave the existing [TestCleanup] Cleanup() untouched.
3. Produce a class with two [TestCleanup]-decorated methods (Cleanup and TestCleanup) — invalid in MSTest.

The inline comment explains the problem domain but does not explain why VerifyCodeFixAsync is omitted. See the inline comment for a suggested clarification.

Minor inaccuracy in the PR description: the description says "applying the fixer would produce a second TestCleanup() method". In reality it would produce two differently-named* methods that both carry [TestCleanup] (Cleanup + TestCleanup), not two identically named ones. No code issue — just a doc nuance.

Test 3 — WhenTestClassHasStaticConstructor_NoDiagnostic

Clean and correct. VerifyCodeFixAsync(code, code) is the established pattern for no-diagnostic cases in this file and matches WhenTestClassHasImplicitCtor_NoDiagnostic and WhenTestClassHasParameterizedCtor_NoDiagnostic. The test is justified by the fact that INamedTypeSymbol.InstanceConstructors (the Roslyn API the analyzer uses) explicitly excludes static constructors.

---

Overall verdict

No blocking issues. The three tests are well-targeted, use the correct verifier APIs (VerifyAnalyzerAsync / VerifyCodeFixAsync), follow the project's assertion and naming conventions (MSTest assertions; AwesomeAssertions correctly absent), and meaningfully improve edge-case coverage for two related analyzers. The one suggestion (documenting or testing the fixer gap in test 2) is filed as an inline comment.