Add [TestClass] code fix for MSTEST0041 and document condition attribute stacking

[Evangelink]

Addresses the actionable, non-speculative parts of #10271.

What changed

Task 1 (High) — code fix for MSTEST0041

UseConditionBaseWithTestClassAnalyzer (MSTEST0041) fires when a ConditionBaseAttribute-derived attribute is applied to a type that is not decorated with [TestClass], but it shipped without a code fix. The fix is purely mechanical — add [TestClass] — and AddTestClassFixer already does exactly that for MSTEST0004 and MSTEST0030, so MSTEST0041 is now registered there rather than duplicating the logic in a new fixer file.

// before
[OSCondition(OperatingSystems.Windows)]
public class MyClass { }

// after
[OSCondition(OperatingSystems.Windows)]
[TestClass]
public class MyClass { }

While wiring this up, AddTestClassFixer was hardened for the new entry point:

• OfType<TypeDeclarationSyntax>().First() → FirstOrDefault() with a null check. A custom condition attribute can redeclare its own AttributeUsage to target a type kind that has no TypeDeclarationSyntax (an enum, for instance), and First() would have thrown.
• Interfaces are skipped, since [TestClass] is meaningless there.

Task 4 (Low) — document the AllowMultiple behaviour

The issue suspected that ConditionBaseAttribute "silently allows AllowMultiple = true via inheritance". That is not what the code does — every derived attribute sets the value explicitly, and the split is deliberate:

Attribute	AllowMultiple	Rationale
MemberConditionAttribute, ExecutableConditionAttribute	true	Each usage names a different member/command, producing a distinct GroupName, so stacked usages AND together.
OSConditionAttribute, ArchitectureConditionAttribute	false	They take a flags enum — use OperatingSystems.Windows | OperatingSystems.Linux instead of stacking.
CIConditionAttribute	false	Only carries a ConditionMode; there is nothing to combine.

Rather than change observable attribute behaviour, the ConditionBaseAttribute XML docs now spell this out so users hitting the compiler error know what to write instead. No public API surface changed.

Not included

Tasks 2 and 3 (MSTEST0078 UseArchitectureConditionAttributeInsteadOfRuntimeCheck and MSTEST0079 UseCIConditionAttributeInsteadOfEnvironmentCheck) introduce brand-new public diagnostic IDs that need their own learn.microsoft.com documentation pages and a product decision on the detection heuristics — in particular MSTEST0079's proposal to hard-code CI environment-variable names (CI, TF_BUILD, …) diverges from how CIConditionAttribute actually detects CI via CIEnvironmentDetector, so it needs design agreement before implementation. Those are better served by dedicated PRs, so #10271 is intentionally left open.

Testing

UseConditionBaseWithTestClassAnalyzerTests now verifies against AddTestClassFixer instead of EmptyCodeFixProvider. The existing diagnostic tests were converted to VerifyCodeFixAsync, and coverage was added for record classes, nested classes and generic classes with constraints.

• MSTest.Analyzers.UnitTests: 1547 passed, 0 failed (net8.0)
• TestFramework builds clean across net462/netstandard2.0/net8.0/net9.0 with 0 warnings

[Copilot]

Pull request overview

Adds an MSTEST0041 code fix and clarifies condition-attribute stacking behavior.

Changes:

• Registers MSTEST0041 with AddTestClassFixer and hardens unsupported-type handling.
• Expands analyzer code-fix tests.
• Documents condition grouping and updates the changelog.

Show a summary per file

File	Description
AddTestClassFixer.cs	Registers and hardens the MSTEST0041 fix.
UseConditionBaseWithTestClassAnalyzerTests.cs	Adds code-fix coverage.
ConditionBaseAttribute.cs	Documents stacking and grouping behavior.
docs/Changelog.md	Records the new code fix.

Review details

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

[github-actions]

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Clean PR. The code fix reuse is well done — registering MSTEST0041 in the existing AddTestClassFixer is the right call, and the defensive null/interface guard is correct. The new tests cover record classes, nested types, and generics nicely.

One minor doc nit flagged inline: ArchitectureConditionAttribute uses <c> tags where <see cref> would be consistent with the rest of the block and provide IDE navigation.

[Copilot]

Review details

Comments suppressed due to low confidence (1)

src/Analyzers/MSTest.Analyzers.CodeFixes/AddTestClassFixer.cs:186

• This lookup only proves that the MSTest symbol is one candidate. If another imported TestClassAttribute is also visible, it still returns true and the generated [TestClass] is ambiguous; a visible type named TestClass can similarly take precedence during attribute binding. Use the short form only when there is no exact TestClass symbol and TestClassAttribute resolves uniquely to the MSTest attribute; otherwise keep the qualified form.

        INamedTypeSymbol? testClassAttributeSymbol = semanticModel.Compilation.GetTypeByMetadataName(WellKnownTypeNames.MicrosoftVisualStudioTestToolsUnitTestingTestClassAttribute);
        return testClassAttributeSymbol is not null
            && semanticModel.LookupNamespacesAndTypes(position, name: $"{TestClassAttributeName}Attribute")
                .Any(symbol => SymbolEqualityComparer.Default.Equals(symbol, testClassAttributeSymbol));

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

[github-actions]

❌ Build Failure Analysis

Root cause: xlf/Resources.fr.xlf (and other .xlf localization files) are out of date with Resources.resx in src/Analyzers/MSTest.Analyzers.

This failure reproduces on all build legs (Linux Debug/Release, macOS Debug/Release, Windows Debug/Release).

Error

'xlf/Resources.fr.xlf' is out-of-date with 'Resources.resx'. Run `msbuild /t:UpdateXlf` to update .xlf files
  Project: src/Analyzers/MSTest.Analyzers/MSTest.Analyzers.csproj
  Target:  _UpdateXlf

Fix

The PR modified Resources.resx (adding resource strings for the new MSTEST0041 code fix) but did not regenerate the .xlf translation files. Run the following locally and commit the updated files:

dotnet msbuild src/Analyzers/MSTest.Analyzers/MSTest.Analyzers.csproj /t:UpdateXlf

This updates all locale files under src/Analyzers/MSTest.Analyzers/xlf/ (Resources.fr.xlf, Resources.de.xlf, Resources.es.xlf, etc.).

🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · sonnet46 51.3 AIC · ⌖ 5.26 AIC · ⊞ 8K · [◷]( · ◷)

[Evangelink]

The PR modified Resources.resx (adding resource strings for the new MSTEST0041 code fix) but did not regenerate the .xlf translation files.

Correcting the record: this PR does not touch Resources.resx or any .xlf file. git diff --name-only origin/main...HEAD returns exactly four files: docs/Changelog.md, AddTestClassFixer.cs, ConditionBaseAttribute.cs, and UseConditionBaseWithTestClassAnalyzerTests.cs. The MSTEST0041 code fix reuses the existing CodeFixResources.AddTestClassFix string, so no new resources were added.

The failure is pre-existing on main, and this PR only inherits it because PR builds validate the merge with main:

• main build 1529393 at commit 0b55734 fails with the identical 'xlf/Resources.fr.xlf' is out-of-date with 'Resources.resx' error.
• This branch forks from 68c9411. Since then main gained 866ed183 (OneLocBuild localized file check-in, Localized file check-in by OneLocBuild Task: Build definition ID 1218: Build ID 3033187 #10268), which rewrote 8 MSTest.Analyzers .xlf files.
• In Resources.fr.xlf that check-in added a UTF-8 BOM to the XML declaration and replaced a blank line inside a translated string with a literal {0} placeholder.

Running /t:UpdateXlf on this branch produces zero changes, so the suggested fix would be an empty diff here. The fix belongs on main (correct or revert the OneLocBuild check-in), which will unblock every PR rather than just this one.

[Copilot]

Review details

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

[Evangelink]

Following up on my earlier comment: I said the XLF fix belonged on main rather than here. That was right at the time, but main has since been merged into this branch, so the breakage is now carried by this PR and had to be dealt with here. 5c6a3c6 does that.

The root cause was not a stale regeneration. The OneLocBuild check-in (866ed183, #10268) replaced a blank line inside the GlobalTestFixtureShouldBeValidDescription French target with a literal {0}, which is what XliffTasks flags as out-of-date with Resources.resx. The fix restores the blank line, a single line change.

I deliberately did not run /t:UpdateXlf, even though that is what the failure message and the bot suggested. It clears the error destructively: it resets that unit to state="new" with the English source text, throwing away the French translation. Restoring the blank line clears the same check and keeps the translation.

Verification:

• With the corrupted file, a CI-mode build (/p:UpdateXlfOnBuild=false) reproduces the exact error locally, so the check is genuinely being exercised.
• With the fix, a full solution build in that same mode succeeds and no other .xlf in the repo is stale.

This should also unblock main once it lands, since main is currently red for the same reason.

[Copilot]

Review details

Comments suppressed due to low confidence (2)

src/Analyzers/MSTest.Analyzers.CodeFixes/AddTestClassFixer.cs:185

• Checking only whether TestClassAttribute is in scope does not prove that the shortened [TestClass] is unambiguous. If another type named TestClass is also in scope, C# reports CS1614 between TestClass and TestClassAttribute, so applying this fix introduces a compiler error. Account for the unsuffixed name as well, or emit a global::-qualified attribute and let Roslyn simplify it only when safe.

        INamedTypeSymbol? testClassAttributeSymbol = semanticModel.Compilation.GetTypeByMetadataName(WellKnownTypeNames.MicrosoftVisualStudioTestToolsUnitTestingTestClassAttribute);
        return testClassAttributeSymbol is not null
            && semanticModel.LookupNamespacesAndTypes(position, name: $"{TestClassAttributeName}Attribute")
                .Any(symbol => SymbolEqualityComparer.Default.Equals(symbol, testClassAttributeSymbol));

src/Analyzers/MSTest.Analyzers.CodeFixes/AddTestClassFixer.cs:37

• Registering MSTEST0041 also sends condition-decorated structs through the existing struct-to-class fix. A custom condition declared with [AttributeUsage(AttributeTargets.Struct)] is valid on the original struct, but after this fix converts it to a class the condition attribute itself becomes invalid (CS0592). Please either suppress the MSTEST0041 action for struct/record-struct declarations, as for enums/interfaces, or only offer conversion when the applied condition attribute also permits class targets; add coverage for this entry point.

            DiagnosticIds.UseConditionBaseWithTestClassRuleId);

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

[Copilot]

Review details

Comments suppressed due to low confidence (2)

src/Analyzers/MSTest.Analyzers.CodeFixes/AddTestClassFixer.cs:185

• This check only verifies that the MSTest symbol is one visible TestClassAttribute candidate. With another imported TestClassAttribute, the emitted [TestClass] is ambiguous; with a visible attribute or alias named TestClass, it can bind to the wrong type. Generate the attribute from the metadata symbol/fully qualified name and let Roslyn simplify it safely, or require the MSTest candidate to be unique and ensure the short name has no competing attribute binding.

        string attributeName = semanticModel is not null && IsTestClassAttributeInScope(semanticModel, position)
            ? TestClassAttributeName
            : FullyQualifiedTestClassAttributeName;

src/Analyzers/MSTest.Analyzers.CodeFixes/AddTestClassFixer.cs:73

• This blanket guard also suppresses a valid fix when a custom condition declares [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct)]. In that case converting the struct to a class keeps the condition attribute valid, but MSTEST0041 now offers no action. Please inspect the offending condition attribute's effective AttributeUsage and skip only when Class is not allowed; add class-and-struct coverage for both struct forms.

        // MSTEST0041 fires on whatever target the condition attribute allows. When that target is a struct, the
        // attribute only got there because its own AttributeUsage permits structs, so turning the struct into a
        // class would strand the attribute on a target it doesn't allow (CS0592). The other rules only ever ask for
        // a test class, where the conversion is the intended fix.
        if (isStruct && diagnostic.Id == DiagnosticIds.UseConditionBaseWithTestClassRuleId)

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

[github-actions]

🧪 Test quality grade — PR #10283

Grade	Test	Mutation	Notes	How to improve
B (80–89)	mod UseConditionBaseWithTestClassAnalyzerTests. WhenNonTestClassHasCIConditionAttribute_ Diagnostic	3/4 killed	Tests same analyzer+fixer path as OSCondition variant; no branch unique to CICondition is exercised.	Fold into a DataRow-driven test over both attribute types to eliminate overlap, or add an assertion unique to CICondition behavior.
A (90–100)	mod UseConditionBaseWithTestClassAnalyzerTests. WhenAbstractNonTestClassHasConditionAttribute_ Diagnostic	3/3 killed	Confirms abstract-class exemption is intentionally absent and fix correctly adds [TestClass] to abstract types.	—
A (90–100)	new UseConditionBaseWithTestClassAnalyzerTests. WhenEnumHasCustomConditionAttribute_ DiagnosticHasNoCodeFix	3/3 killed	Only test covering the enum bail-out path (no TypeDeclarationSyntax → no fix offered) in AddTestClassFixer.	—
A (90–100)	new UseConditionBaseWithTestClassAnalyzerTests. WhenGenericNonTestClassHasConditionAttribute_ FixAddsTestClass	3/3 killed	Verifies that type-parameter list and constraint clauses are preserved after AddAttributeLists mutation.	—
A (90–100)	new UseConditionBaseWithTestClassAnalyzerTests. WhenInterfaceHasCustomConditionAttribute_ DiagnosticHasNoCodeFix	3/3 killed	Covers the InterfaceDeclarationSyntax early-return guard that prevents offering a code fix.	—
A (90–100)	new UseConditionBaseWithTestClassAnalyzerTests. WhenNestedNonTestClassHasConditionAttribute_ FixAddsTestClassToNestedTypeOnly	3/3 killed	Confirms fix is scoped to the inner type and does not duplicate [TestClass] on the outer class.	—
A (90–100)	mod UseConditionBaseWithTestClassAnalyzerTests. WhenNonTestClassHasCustomConditionAttribute_ Diagnostic	4/4 killed	Verifies the Inherits() walk works for a custom ConditionBaseAttribute subclass and fix adds [TestClass].	—
A (90–100)	new UseConditionBaseWithTestClassAnalyzerTests. WhenNonTestClassHasFullyQualifiedOSConditionAttributeWithoutUsing_ FixAddsFullyQualifiedTestClass	3/3 killed	Only test exercising IsTestClassAttributeInScope=false; kills mutations that always use the short attribute name.	—
A (90–100)	mod UseConditionBaseWithTestClassAnalyzerTests. WhenNonTestClassHasMultipleConditionAttributes_ SingleDiagnostic	4/4 killed	Verifies FirstOrDefault single-diagnostic behavior and that the fix appends [TestClass] after both condition attrs.	—
A (90–100)	mod UseConditionBaseWithTestClassAnalyzerTests. WhenNonTestClassHasOSConditionAttribute_ Diagnostic	4/4 killed	Primary scenario: verifies diagnostic location, argument, and exact fixed code for the basic class case.	—
A (90–100)	new UseConditionBaseWithTestClassAnalyzerTests. WhenNonTestRecordClassHasConditionAttribute_ FixAddsTestClass	3/3 killed	Verifies record class takes AddTestClassAttributeAsync path and preserves the record keyword.	—
A (90–100)	new UseConditionBaseWithTestClassAnalyzerTests. WhenRecordStructHasStructOnlyCustomConditionAttribute_ DiagnosticHasNoCodeFix	3/3 killed	Covers record struct variant of the struct+MSTEST0041 no-fix guard via ClassOrStructKeyword check.	—
A (90–100)	new UseConditionBaseWithTestClassAnalyzerTests. WhenStructHasStructOnlyCustomConditionAttribute_ DiagnosticHasNoCodeFix	3/3 killed	Covers plain-struct bail-out guard with a clear CS0592 rationale; prevents regression in the isStruct branch.	—
A (90–100)	mod UseConditionBaseWithTestClassAnalyzerTests. WhenTwoLevelDerivedConditionAttributeOnNonTestClass_ Diagnostic	4/4 killed	Tests the recursive Inherits() walk at depth 2; would catch a non-recursive implementation.	—

_{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. · sonnet46 85.7 AIC · ⌖ 7.34 AIC · ⊞ 10.3K · [◷]( · ◷)