[test-improver] test: add edge case tests for RemoveClassCleanupBehaviorArgumentFixer - #9410
Conversation
- WhenClassCleanupBehaviorIsFirstArgument_RemovesBehaviorKeepsInheritance: verifies that when ClassCleanupBehavior appears as the first argument (before InheritanceBehavior), the fixer removes it and preserves the remaining argument. - WhenClassCleanupBehaviorReferencedOutsideAttribute_NoFix: verifies the attributeArgument-is-null guard — the fixer does not register a fix when ClassCleanupBehavior is used in a method body instead of an attribute argument. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds missing unit-test coverage for RemoveClassCleanupBehaviorArgumentFixer (MSTest analyzers) by exercising two previously untested branches: removing ClassCleanupBehavior when it is the first of two arguments, and ensuring no fix is offered when ClassCleanupBehavior appears outside an attribute argument.
Changes:
- Add a test where
ClassCleanupBehavioris the first argument and verify the fixer preserves the remainingInheritanceBehaviorargument. - Add a test ensuring no fix is registered when the
ClassCleanupBehaviorcompiler diagnostic occurs in a method body (non-attribute context).
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/MSTest.Analyzers.UnitTests/RemoveClassCleanupBehaviorArgumentFixerTests.cs | Adds two new test methods covering the fixer’s “first argument” removal path and its “not inside attribute argument” no-op guard. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
🧪 Test quality grade — PR #9410
This advisory comment was generated automatically. Grades are heuristic
|
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
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.
✅ 10/10 applied dimensions clean — no findings.
Skipped (not applicable to a test-only diff): Threading & Concurrency, Security & IPC Contract Safety, Public API & Binary Compatibility, Performance & Allocations, Cross-TFM Compatibility, Resource & IDisposable Management, Defensive Coding at Boundaries, Localization & Resources, Analyzer & Code Fix Quality (src), IPC Wire Compatibility, Build Infrastructure & Dependencies, PowerShell Scripting Hygiene.
Applied dimensions:
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ LGTM |
| 2 | Test Isolation | ✅ LGTM |
| 3 | Assertion Quality | ✅ LGTM |
| 4 | Flakiness Patterns | ✅ LGTM |
| 5 | Test Completeness & Coverage | ✅ LGTM |
| 6 | Data-Driven Test Coverage | ✅ LGTM |
| 7 | Code Structure & Simplification | ✅ LGTM |
| 8 | Naming & Conventions | ✅ LGTM |
| 9 | Documentation Accuracy | ✅ LGTM |
| 10 | Scope & PR Discipline | ✅ LGTM |
Notes:
-
Algorithmic Correctness: Both diagnostic markers are correct.
{|CS0103:ClassCleanupBehavior|}fires because the type is gone;{|CS1729:ClassCleanup(...)|}fires because no overload matches the erroneous first argument — exactly mirroring the second-argument pattern already exercised inWhenClassCleanupBehaviorArgument_Simple. The fixer'sRemoveNode(attributeArgument, SyntaxRemoveOptions.KeepNoTrivia)on aSeparatedSyntaxListremoves the first element and its following separator, producing(InheritanceBehavior.None)with no stray space — consistent with the expectedfixedCode. -
No-fix guard path:
VerifyCodeFixAsync(code, code)is the correct idiom for asserting that no fix is registered. The guard in the fixer (attributeArgument is null → skip registration) is cleanly exercised by placing theCS0103reference inside a method body statement, not inside anyAttributeArgumentSyntaxancestor. -
Inline comments: The
// Exercises the branch...comments in the new test bodies are the only body-level comments in the file. They fall within the project's "comment what needs clarification" guideline and genuinely help readers understand which fixer branch is being targeted without requiring them to trace into the fixer implementation.
Goal and Rationale
RemoveClassCleanupBehaviorArgumentFixerhad only 2 tests covering the "happy path" (sole argument removed, second of two arguments removed). Two code paths inside the fixer were completely untested:ClassCleanupBehavioras the first of two arguments — the fixer removes the first argument and preserves the remainingInheritanceBehaviorargument. This is the mirror of the existing test whereClassCleanupBehavioris second.attributeArgument is nullguard — when aCS0103diagnostic forClassCleanupBehaviorfires outside an attribute argument (e.g. in a method body), the fixer explicitly skips registration. This path was never exercised by a test.Approach
Added two new
[TestMethod]entries toRemoveClassCleanupBehaviorArgumentFixerTests.cs:WhenClassCleanupBehaviorIsFirstArgument_RemovesBehaviorKeepsInheritance[ClassCleanup(ClassCleanupBehavior.EndOfClass, InheritanceBehavior.None)]→[ClassCleanup(InheritanceBehavior.None)]WhenClassCleanupBehaviorReferencedOutsideAttribute_NoFix_ = ClassCleanupBehavior.EndOfClass;in a method body — no fix registered, code unchangedTrade-offs
Both tests are simple and low-maintenance. The "no fix" test uses
VerifyCodeFixAsync(code, code), the same pattern used elsewhere in the codebase for cases where a diagnostic fires but no fix is appropriate.Test Status
All 4 tests pass (
2pre-existing +2new):Command:
Add this agentic workflows to your repo
To install this agentic workflow, run