Add analyzer for OS platform conditions - #11044
Conversation
Add MSTEST0084 to keep platform compatibility attributes aligned with MSTest OSCondition attributes, including safe code fixes and coverage for multi-document and malformed syntax cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2e19749e-0880-43e6-9e79-66116fe6ea89
🧵 Parallel-safety audit — PR #11044Parallelization
Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 0. The only test file this PR touches is a newly added file, Nothing to flag for parallel-safety. Advisory only — heuristic, non-blocking. Re-run with
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Important no-fix, mode-equivalence, and FreeBSD branches lack effective regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/Analyzers/MSTest.Analyzers/OSPlatformAttributesShouldBeConsistentAnalyzer.cs — The tests for mixed modes and versioned platforms only call VerifyAnalyzerAsync, while the no-fix… |
What changed in this PR
Adds MSTEST0084 to align platform compatibility attributes with MSTest runtime conditions, including a safe C# fixer and localized diagnostics.
Changes:
- Detects missing or inconsistent
OSConditionattributes. - Adds fixes for representable platform combinations and partial declarations.
- Adds C#/VB tests, release metadata, and localization resources.
| File | Description |
|---|---|
test/UnitTests/MSTest.Analyzers.UnitTests/OSPlatformAttributesShouldBeConsistentAnalyzerTests.cs |
Tests analyzer and fixer behavior. |
src/Analyzers/MSTest.Analyzers/OSPlatformAttributesShouldBeConsistentAnalyzer.cs |
Implements MSTEST0084. |
src/Analyzers/MSTest.Analyzers.CodeFixes/OSPlatformAttributesShouldBeConsistentFixer.cs |
Implements the C# code fix. |
src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs |
Adds platform attribute metadata names. |
src/Analyzers/MSTest.Analyzers/Helpers/DiagnosticIds.cs |
Registers the MSTEST0084 identifier. |
src/Analyzers/MSTest.Analyzers/AnalyzerReleases.Unshipped.md |
Records the new analyzer rule. |
src/Analyzers/MSTest.Analyzers/Resources.resx |
Adds diagnostic strings. |
src/Analyzers/MSTest.Analyzers.CodeFixes/CodeFixResources.resx |
Adds the code-fix title. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlf |
Adds Czech localization entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlf |
Adds German localization entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlf |
Adds Spanish localization entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlf |
Adds French localization entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlf |
Adds Italian localization entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlf |
Adds Japanese localization entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlf |
Adds Korean localization entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlf |
Adds Polish localization entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlf |
Adds Portuguese localization entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlf |
Adds Russian localization entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlf |
Adds Turkish localization entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlf |
Adds Simplified Chinese entries. |
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlf |
Adds Traditional Chinese entries. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.cs.xlf |
Adds Czech code-fix localization. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.de.xlf |
Adds German code-fix localization. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.es.xlf |
Adds Spanish code-fix localization. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.fr.xlf |
Adds French code-fix localization. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.it.xlf |
Adds Italian code-fix localization. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ja.xlf |
Adds Japanese code-fix localization. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ko.xlf |
Adds Korean code-fix localization. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.pl.xlf |
Adds Polish code-fix localization. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.pt-BR.xlf |
Adds Portuguese code-fix localization. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ru.xlf |
Adds Russian code-fix localization. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.tr.xlf |
Adds Turkish code-fix localization. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.zh-Hans.xlf |
Adds Simplified Chinese code-fix entries. |
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.zh-Hant.xlf |
Adds Traditional Chinese code-fix entries. |
Suppressed comments (2)
src/Analyzers/MSTest.Analyzers/OSPlatformAttributesShouldBeConsistentAnalyzer.cs:192
- The two-argument equivalence branch is untested: the equivalent-condition test covers only the one-argument include constructor, and the unsupported-platform test only adds a missing condition. A regression that reverses or ignores
ConditionModewould therefore survive. Add equivalent explicit-Include and Exclude cases, plus a mismatched-mode case that expects a diagnostic.
[{ Value: int actualMode }, { Value: int actualOperatingSystems }]
=> (actualMode == 0) == includeMode && actualOperatingSystems == operatingSystems,
src/Analyzers/MSTest.Analyzers/OSPlatformAttributesShouldBeConsistentAnalyzer.cs:154
FreeBSDis a newly supported mapping, but no test exercises this arm or verifies generation ofOperatingSystems.FreeBSD. A wrong bit or emitted name here would leave the current suite green. Add a FreeBSD fix or equivalence case.
"FREEBSD" => 1 << 3,
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ImmutableDictionary<string, string?> properties = canFix | ||
| ? ImmutableDictionary<string, string?>.Empty | ||
| .Add(ConditionModeKey, includeMode ? "Include" : "Exclude") | ||
| .Add(OperatingSystemsKey, operatingSystemsExpression) | ||
| : ImmutableDictionary<string, string?>.Empty; |
🧪 Expert test review — PR #11044
This advisory comment was generated automatically. Grades are heuristic
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 126 AIC · ⌖ 2.79 AIC · ⊞ 16.9K · ◷
| [TestMethod] | ||
| public async Task WhenSupportedPlatformHasNoOSConditionInVisualBasic_Diagnostic() | ||
| { | ||
| string code = """ | ||
| Imports System.Runtime.Versioning | ||
| Imports Microsoft.VisualStudio.TestTools.UnitTesting | ||
|
|
||
| <TestClass> | ||
| Public Class MyTestClass | ||
| <TestMethod> | ||
| <[|SupportedOSPlatform("linux")|]> | ||
| Public Sub TestMethod() | ||
| End Sub | ||
| End Class | ||
| """; | ||
|
|
||
| await VerifyVB.VerifyAnalyzerAsync(code); | ||
| } |
There was a problem hiding this comment.
🧪 Test review · Grade B (80–89) — Verifies diagnostic location only; unlike its C# counterpart it never asserts the reported message argument (method name).
Assert the diagnostic explicitly with WithArguments("TestMethod") so a regression in the message format goes undetected.
| [TestMethod] | |
| public async Task WhenSupportedPlatformHasNoOSConditionInVisualBasic_Diagnostic() | |
| { | |
| string code = """ | |
| Imports System.Runtime.Versioning | |
| Imports Microsoft.VisualStudio.TestTools.UnitTesting | |
| <TestClass> | |
| Public Class MyTestClass | |
| <TestMethod> | |
| <[|SupportedOSPlatform("linux")|]> | |
| Public Sub TestMethod() | |
| End Sub | |
| End Class | |
| """; | |
| await VerifyVB.VerifyAnalyzerAsync(code); | |
| } | |
| [TestMethod] | |
| public async Task WhenSupportedPlatformHasNoOSConditionInVisualBasic_Diagnostic() | |
| { | |
| string code = """ | |
| Imports System.Runtime.Versioning | |
| Imports Microsoft.VisualStudio.TestTools.UnitTesting | |
| <TestClass> | |
| Public Class MyTestClass | |
| <TestMethod> | |
| <[|SupportedOSPlatform("linux")|]> | |
| Public Sub TestMethod() | |
| End Sub | |
| End Class | |
| """; | |
| await VerifyVB.VerifyAnalyzerAsync( | |
| code, | |
| VerifyVB.Diagnostic().WithLocation(0).WithArguments("TestMethod")); | |
| } | |
| } |
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.
Expert Review — MSTEST0084: OS Platform Conditions Analyzer
PR adds a new analyzer (MSTEST0084) + C# code fix that detects [SupportedOSPlatform]/[UnsupportedOSPlatform] without a matching [OSCondition], and offers to add or update one. Well-structured, focused PR with good test coverage (12 dedicated tests covering include/exclude, partial classes, named argument preservation, mixed-mode bail-out, versioned platforms, and VB diagnostic-only).
Verdict Table
| # | Dimension | Result |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ LGTM — Logic is sound; mixed-mode and versioned-platform bail-outs are correct. Minor: macos→OSX canonicalization noted inline. |
| 2 | Threading & Concurrency | ✅ N/A — Analyzer is stateless, concurrent execution enabled correctly. |
| 3 | Security & IPC Contract Safety | ✅ N/A |
| 4 | Public API & Binary Compatibility | ✅ LGTM — No new public API; analyzer classes follow existing conventions. Diagnostic ID MSTEST0084 added to AnalyzerReleases.Unshipped.md. |
| 5 | Performance & Allocations | ✅ LGTM — Code fix path only; no hot-path concern. Minor nit inline. |
| 6 | Cross-TFM Compatibility | ✅ LGTM — Analyzer targets Roslyn APIs; no TFM-specific APIs used. |
| 7 | Resource & IDisposable Management | ✅ N/A |
| 8 | Defensive Coding at Boundaries | ✅ LGTM — Graceful bail-outs when symbol resolution fails, when constructor arguments don't match expected shapes, and when platform names are unmappable. |
| 9 | Localization & Resources | ✅ LGTM — Strings in .resx, {Locked} markers use bracket/quote form ([OSCondition], SupportedOSPlatformAttribute) — no substring collision risk. XLF files regenerated. |
| 10 | Test Isolation | ✅ N/A |
| 11 | Assertion Quality | ✅ LGTM — Uses CSharpCodeFixVerifier / VisualBasicCodeFixVerifier infrastructure. |
| 12 | Flakiness Patterns | ✅ N/A |
| 13 | Test Completeness & Coverage | "android"). See inline comment. |
| 14 | Data-Driven Test Coverage | ✅ LGTM — Good variety of scenarios. |
| 15 | Code Structure & Simplification | ✅ LGTM — Clean, well-factored code. |
| 16 | Naming & Conventions | ✅ LGTM |
| 17 | Documentation Accuracy | OperatingSystems enum deserves a sync-requirement comment. See inline. |
| 18 | Analyzer & Code Fix Quality | ✅ LGTM — Analyzer doesn't throw on malformed input; code fix produces compilable output; severity Info is appropriate. |
| 19 | IPC Wire Compatibility | ✅ N/A |
| 20 | Build Infrastructure & Dependencies | ✅ N/A |
| 21 | Scope & PR Discipline | ✅ LGTM — Single concern, references #11018. |
| 22 | PowerShell Scripting Hygiene | ✅ N/A |
Summary
Solid, well-tested analyzer addition. No blocking issues found. Two minor suggestions inline (add a comment about bit-position sync, add a test case for unrecognized platform names). Overall this is ready for human sign-off.
| private static bool TryMapPlatform(string platformName, out int operatingSystem) | ||
| { | ||
| operatingSystem = platformName.ToUpperInvariant() switch | ||
| { | ||
| "LINUX" => 1 << 0, | ||
| "OSX" or "MACOS" => 1 << 1, | ||
| "WINDOWS" => 1 << 2, | ||
| "FREEBSD" => 1 << 3, | ||
| _ => 0, |
There was a problem hiding this comment.
nit (Dimension 17 — Documentation): The bit positions here (1 << 0 through 1 << 3) are duplicated from the OperatingSystems enum in TestFramework. Because the analyzer assembly can't reference the framework, the duplication is unavoidable, but a comment noting the sync requirement would help future maintainers:
| private static bool TryMapPlatform(string platformName, out int operatingSystem) | |
| { | |
| operatingSystem = platformName.ToUpperInvariant() switch | |
| { | |
| "LINUX" => 1 << 0, | |
| "OSX" or "MACOS" => 1 << 1, | |
| "WINDOWS" => 1 << 2, | |
| "FREEBSD" => 1 << 3, | |
| _ => 0, | |
| operatingSystem = platformName.ToUpperInvariant() switch | |
| { | |
| // Bit positions must match the OperatingSystems enum in | |
| // Microsoft.VisualStudio.TestTools.UnitTesting. | |
| "LINUX" => 1 << 0, | |
| "OSX" or "MACOS" => 1 << 1, | |
| "WINDOWS" => 1 << 2, | |
| "FREEBSD" => 1 << 3, | |
| _ => 0, | |
| }; |
|
|
||
| await VerifyVB.VerifyAnalyzerAsync(code); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (Dimension 13 — Test Completeness): Consider adding a test for a platform name that is valid for [SupportedOSPlatform] but unmappable to OperatingSystems (e.g., "android" or "ios"). This path differs from the versioned-platform test ("windows10.0.19041") because the base name itself is unrecognized, not just versioned. The analyzer should still report the diagnostic but with no code fix offered.
|
|
||
| private static void AddNameIfSet(List<string> names, int operatingSystems, int value, string name) | ||
| { | ||
| if ((operatingSystems & value) != 0) |
There was a problem hiding this comment.
question (Dimension 1 — Algorithmic Correctness): TryMapPlatform normalises via ToUpperInvariant(), which correctly maps both "OSX" and "MACOS" to the same bit. However, CreateOperatingSystemsExpression always emits "OSX" for the round-tripped name. If the user originally wrote [SupportedOSPlatform("macos")], the code fix will insert OperatingSystems.OSX — which is correct (it's the enum member name) but may surprise a user who expected the code fix to mirror their casing/name choice.
This isn't a bug (the code fix should use the canonical enum name), but it may be worth a brief note in the docs page for MSTEST0084 explaining the macos → OSX canonicalization.
| .Select(name => SyntaxFactory.ParseExpression($"{MSTestNamespace}.OperatingSystems.{name}") | ||
| .WithAdditionalAnnotations(Simplifier.Annotation, Simplifier.AddImportsAnnotation)) | ||
| .ToArray(); | ||
| ExpressionSyntax operatingSystemsExpression = operatingSystemExpressions.Aggregate( |
There was a problem hiding this comment.
nit (Dimension 5 — Performance): .ToArray() + .Aggregate() allocate intermediary arrays. Since this is code-fix-only (not hot path), it's fine — just noting for completeness that SeparatedList accepts IEnumerable, so the .ToArray() on operatingSystemExpressions could be avoided if desired.
Summary
OSConditionattributes on tests usingSupportedOSPlatformAttributeorUnsupportedOSPlatformAttributeTesting
net8.0andnet472net8.0Fixes #11018