Skip to content

Fix MSTEST0063 to detect invalid constructors on derived TestClass attributes - #9851

Merged
Evangelink merged 1 commit into
mainfrom
dev/amauryleve/curly-lamp
Jul 11, 2026
Merged

Fix MSTEST0063 to detect invalid constructors on derived TestClass attributes#9851
Evangelink merged 1 commit into
mainfrom
dev/amauryleve/curly-lamp

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Fixes #9836

Problem

TestClassConstructorShouldBeValidAnalyzer (MSTEST0063) validates that [TestClass]-decorated classes have a public parameterless constructor or a public TestContext-parameter constructor.

The guard in AnalyzeSymbol used an exact-match check (SymbolEqualityComparer.Default.Equals) against TestClassAttribute, so classes decorated with derived test-class attributes — including the built-in [STATestClass] and any custom class MyAttr : TestClassAttribute — were silently skipped. Because MSTest discovers such classes at runtime via Inherits() logic, a private/internal constructor causes a runtime instantiation failure with no analyzer warning.

Fix

Replace the exact-match guard with the shared IsTestClass() helper, which calls Inherits() and matches the runtime discovery behavior:

// Before
!namedTypeSymbol.GetAttributes().Any(attr =>
    SymbolEqualityComparer.Default.Equals(attr.AttributeClass, testClassAttributeSymbol))

// After
!namedTypeSymbol.IsTestClass(testClassAttributeSymbol)

Tests

Four new tests added to TestClassConstructorShouldBeValidAnalyzerTests.cs:

Scenario Expected
[STATestClass] + private constructor Diagnostic
[STATestClass] + public parameterless constructor NoDiagnostic
Custom derived TestClass attr + internal constructor Diagnostic
Custom derived TestClass attr + public constructor NoDiagnostic

Trade-offs

This is a behavior change: previously, classes with derived [TestClass] attributes and invalid constructors produced no warning; after this fix they do. This is the correct behavior — the fix brings the analyzer in line with MSTest's runtime class-discovery logic.

Build and MSTest.Analyzers.UnitTests (net472 + net8.0) pass locally.

…tributes

TestClassConstructorShouldBeValidAnalyzer used an exact-match check
(SymbolEqualityComparer.Default.Equals) for TestClassAttribute, so
derived attributes like [STATestClass] or custom [MyTestClass] were
silently skipped. MSTest discovers such classes at runtime via
Inherits() logic, so a private/internal constructor causes a runtime
failure but no analyzer warning.

Switch the guard to IsTestClass() (which uses Inherits()) to match
the runtime discovery logic, and add four tests covering derived
and custom TestClass attributes.

Fixes #9836

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d1590213-39c9-4b3d-9fa4-1d10ac4961cc
Copilot AI review requested due to automatic review settings July 11, 2026 12:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates MSTEST0063 to validate constructors on classes using derived TestClassAttribute types.

Changes:

  • Uses the shared inheritance-aware IsTestClass() helper.
  • Adds coverage for built-in and custom derived attributes.
Show a summary per file
File Description
TestClassConstructorShouldBeValidAnalyzer.cs Enables derived test-class attribute detection.
TestClassConstructorShouldBeValidAnalyzerTests.cs Tests valid and invalid constructor scenarios.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Medium

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix. The IsTestClass helper (via Inherits) correctly walks the attribute type hierarchy, which is exactly what was missing with the old SymbolEqualityComparer.Default.Equals exact-match check. This is consistent with how other analyzers (TestClassShouldHaveTestMethodAnalyzer, PublicMethodShouldBeTestMethodAnalyzer, etc.) already guard their IsTestClass checks.

Tests cover both [STATestClass] (built-in derived) and a custom derived attribute, with positive and negative cases. No issues found.

@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test quality grade — PR #9851

GradeTestNotes
A (90–100) new TestClassConstructorShouldBeValidAnalyzerTests.
WhenDerivedTestClassAttributeHasPrivateConstructor_
Diagnostic
Clear AAA; asserts exact diagnostic location and arguments via VerifyCodeFixAsync. No issues found.
A (90–100) new TestClassConstructorShouldBeValidAnalyzerTests.
WhenDerivedTestClassAttributeHasPublicParameterlessConstructor_
NoDiagnostic
Clean negative-path test; VerifyCodeFixAsync(code, code) meaningfully asserts no diagnostic is produced. No issues found.
A (90–100) new TestClassConstructorShouldBeValidAnalyzerTests.
WhenCustomDerivedTestClassAttributeHasInternalConstructor_
Diagnostic
Clear AAA; custom-attribute inheritance chain is exercised with exact diagnostic location and arguments. No issues found.
A (90–100) new TestClassConstructorShouldBeValidAnalyzerTests.
WhenCustomDerivedTestClassAttributeHasPublicConstructor_
NoDiagnostic
Clean negative-path test; mirrors the diagnostic counterpart symmetrically, exercising the full custom-attribute path. 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. Generated by the Grade Tests on PR (on open / sync) workflow. · 32.6 AIC · ⌖ 6.32 AIC · ⊞ 9.5K · [◷]( · )

@github-actions

Copy link
Copy Markdown
Contributor

🔴 Build Failure Analysis

Build: FAILED (236.6s) · 5 errors · 1 warning

Root Cause

All 4 compilation errors are the same issue — analyzer rule CA1416 (platform compatibility) on two call sites in FileLoggerTests.cs:

Line Class Issue
465 LoopTrackingTask Calls ITask.RunLongRunning() which is marked [UnsupportedOSPlatform("browser")]
490 NeverCompletingTask Same

(Each error is reported twice — once per TFM.)

The interface method ITask.RunLongRunning was annotated with [UnsupportedOSPlatform("browser")] and [UnsupportedOSPlatform("wasi")], but the two test helper classes that delegate to it don't suppress the warning.

Suggested Fix

Add [UnsupportedOSPlatform("browser")] to lines 464 and 489, or (simpler for test code) add a #pragma warning disable CA1416 / #pragma warning restore CA1416 around each call site. See inline suggestions below.

🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 45.4 AIC · ⌖ 5.79 AIC · ⊞ 7.3K · [◷]( · )

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 45.4 AIC · ⌖ 5.79 AIC · ⊞ 7.3K ·

@Evangelink

Copy link
Copy Markdown
Member Author

The CA1416 build failure is a pre-existing issue in test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/FileLoggerTests.cs (the LoopTrackingTask / NeverCompletingTask ITask helpers), which this PR does not touch. It stems from the [UnsupportedOSPlatform("browser")] annotation on ITask.RunLongRunning and is unrelated to the MSTEST0063 analyzer change.

It's being handled separately (branch dev/amauryleve/fix-ca1416-filelogger-tests), so I'm keeping this PR surgical and not pulling the unrelated test fix in here. This PR will go green once that fix lands on main and this branch is rebased.

@Evangelink
Evangelink merged commit 59ced3e into main Jul 11, 2026
28 of 35 checks passed
@Evangelink
Evangelink deleted the dev/amauryleve/curly-lamp branch July 11, 2026 12:29
@Evangelink

Copy link
Copy Markdown
Member Author

/backport to rel/4.3

@github-actions

Copy link
Copy Markdown
Contributor

Evangelink added a commit that referenced this pull request Jul 21, 2026
…tributes by @Evangelink in #9851 (backport to rel/4.3) (#10115)

Co-authored-by: Amaury Levé <amauryleve@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[test-improver] Fix MSTEST0063 to detect invalid constructors on derived TestClass attributes

2 participants