Skip to content

Add parallel-safety analyzers (MSTEST0074-MSTEST0077) - #10248

Merged
Evangelink merged 13 commits into
mainfrom
dev/amauryleve/parallel-safety-analyzers
Jul 27, 2026
Merged

Add parallel-safety analyzers (MSTEST0074-MSTEST0077)#10248
Evangelink merged 13 commits into
mainfrom
dev/amauryleve/parallel-safety-analyzers

Conversation

@Evangelink

@Evangelink Evangelink commented Jul 27, 2026

Copy link
Copy Markdown
Member

Why

[ResourceLock] (RFC 020, #10234) coordinates access to process-global state, but its free-form string keys fail open: forget to declare a lock and you get a silent race, not an error. These four analyzers make the statically decidable cases fail closed at compile time — the enforcement counterpart to [ResourceLock]. They flag test code that mutates process-global state (environment variables, current directory, culture, console) or writes to a shared/ambiguous filesystem path, which silently breaks the day someone adds [assembly: Parallelize].

Rules

Id Rule Detects (mutations only) Severity Code fix
MSTEST0074 R1 — Undeclared process-global mutation Environment.SetEnvironmentVariable; Console.SetOut/SetError/SetIn Info ✅ adds [ResourceLock(WellKnownResources.EnvironmentVariables/Console)]
MSTEST0075 R2 — Current-directory mutation Environment.CurrentDirectory setter; Directory.SetCurrentDirectory Info ✅ adds [ResourceLock(...CurrentDirectory)]
MSTEST0076 R3 — Culture mutation CultureInfo.DefaultThreadCurrentCulture / DefaultThreadCurrentUICulture (process-wide only) Info
MSTEST0077 R4 — Shared filesystem path constant/relative literal passed directly to a mutating File.*/Directory.* API Info ❌ (gated on TestContext.TestTempDirectory, #10233)

All four are Info + enabled-by-default. Per MSTest policy (now documented in docs/AddingAnalyzerCodeFix.md), enabled-by-default + Warning is reserved for known runtime breaks, because many consumers build with TreatWarningsAsErrors and a parallel-safety heuristic must never break their build. Info surfaces the finding without that risk and is unlikely to be mass-suppressed, protecting the ruleset's credibility budget.

R1–R3 stay silent when the mutation is already coordinated by a matching [ResourceLock], and all four rules stay silent under [DoNotParallelize]. R4 is the deliberate exception on [ResourceLock]: its remedy is a unique path (TestContext.TestTempDirectory), so a lock around a shared fixed path only serializes the collision rather than removing it — R4 keeps steering toward a unique path. Each site is reported by exactly one rule (CWD→R2, culture→R3, env/console→R1), never double-fired. Simple and compound (+=) assignments are treated as mutations everywhere. Coalescing (??=) is registered per-property according to declared nullability: R3 handles it because CultureInfo.DefaultThreadCurrentCulture is nullable, while R2 does not, because Environment.CurrentDirectory is declared non-nullable and throws rather than returning null — so ??= there can never reach the setter and flagging it would be a guaranteed false positive.

Firing gate and its known limitation

Diagnostics are live only when parallelization is enabled, and MSTest does not parallelize by default. The rules fire when [assembly: Parallelize] is syntactically present OR the .editorconfig knob mstest_parallel_safety_mode = always is set (mirrors how UseParallelizeAttributeAnalyzer reasons about [Parallelize]). Assembly-level [DoNotParallelize] disables all four rules regardless of the always override.

Known limitation: an analyzer cannot read runsettings or the MSTestParallelizeScope MSBuild property, so parallelization opted in only through those channels is invisible to the syntactic gate. The .editorconfig knob exists precisely to cover that case (read from both GlobalOptions and every syntax tree's [*.cs] options). ExecutionScope is not encoded in the firing logic (each rule flags a single unprotected mutation, which is live cross-class even under the default ClassLevel); scope nuance lives in the message text.

Fixture-aware code-fix scope

MSTest discovery reads resource locks only from the test class and the test method — a lock on a fixture method ([TestInitialize], [ClassInitialize], …) would be silently ignored. The fixer therefore places the lock where it actually takes effect: on the enclosing class for class-scoped fixtures, on the method for test methods, and offers no fix at all for the global per-test fixtures, since no lock target would work — the diagnostic still reports, but a fix that silently does nothing would be worse than none.

Assembly-scoped fixtures are not analyzed at all. [AssemblyInitialize] runs under TestAssemblyInfo's SemaphoreSlim(1, 1) and every worker awaits it before running its test, and [AssemblyCleanup] runs only after the last runnable test in the whole assembly — so a process-global mutation in either cannot race a concurrent test and reporting it would be a false positive. Class- and test-scoped fixtures and the global per-test fixtures are analyzed, because those genuinely can overlap a parallel test.

R4 narrowing — the precision story

R4 originally also detected Path.Combine(Path.GetTempPath(), <const>). Dogfooding the built DLL against Microsoft.Testing.Extensions.UnitTests (all four rules elevated to warning) made that heuristic fire 8 times — every one a false positive:

  • TrxArtifactPostProcessorTests → temp path fed to CreateMergeRunId (pure string hash)
  • VideoRecorderSessionHandlerTests"missing-ffmpeg" deliberately-nonexistent sentinel
  • CrashDumpTests → path fed to pure string manipulation, asserted by equality
  • TrxReportEngineMergeTests → path stored as an XML attribute value
  • TrxTests → path returned from a mock, never written

What they had in common: the constructed path is a string used as a pure value — hash input, sentinel, XML data, mock return — never for colliding I/O. The analyzer sees the call, not the resource, so it cannot tell construction from use. R4 was therefore narrowed to fire only on a constant/relative literal passed directly to a filesystem-mutating File.*/Directory.* method (WriteAllText, Create, Copy, Move, Delete, CreateSymbolicLink, SetAttributes, the Set*Time family, Directory.CreateDirectory, …). Each API's path role is respected — File.Copy flags only its destination (the source is read), File.Move both endpoints, File.Replace its destination and backup — so a constant source handed to File.Copy stays silent. Reads (File.ReadAllText of a shared fixture is common and safe) and path construction are excluded. Re-dogfooding produced 0 hits — and the mechanism is proven, because the same injection produced 8 hits with the pre-narrowing DLL, so "0 hits" means "correctly silent," not "analyzer never ran." The rationale is preserved as a comment in SharedFileSystemPathInTestAnalyzer.cs so the heuristic is not re-added later. Everything fuzzier than this — paths traced through helpers, lower-precision heuristics — is deliberately left to the sibling parallel-safety-audit skill (below).

R4 found zero true positives in testfx, which does not generalise: this repo's authors already know why hardcoded paths are dangerous. File.WriteAllText("output.txt", …) / Directory.CreateDirectory("testdata") in a test method is an extremely common pattern in ordinary suites and is exactly what R4 now catches, at zero false-positive cost.

Culture determination (R3 — verified against dotnet/runtime source)

API Scope Treatment
CultureInfo.DefaultThreadCurrentCulture / DefaultThreadCurrentUICulture process-wide static field Flag
Thread.CurrentThread.CurrentCulture / CurrentUICulture AsyncLocal-backed, flows with ExecutionContext Omit
CultureInfo.CurrentCulture / CurrentUICulture AsyncLocal-backed, flows with ExecutionContext Omit

R3 was narrowed during review to flag only the DefaultThreadCurrent* setters. On modern .NET the Thread.CurrentThread.CurrentCulture/CurrentUICulture setter delegates to the ambient CultureInfo.CurrentCulture, which stores an AsyncLocal-backed value that flows with the ExecutionContext — so it neither corrupts a concurrently running sibling test nor leaks onto a later-pooled thread. Only DefaultThreadCurrent* assigns a genuinely process-wide static field observed by every concurrent test, so that is the one unambiguously unsafe shape. Flagging the per-thread/ambient forms would be a false positive on the safe form (the maintainer flagged exactly this); omitting DefaultThreadCurrent* would miss the dangerous one. This corrects the earlier "pooled-thread leak" framing.

Division of labour

Known limitations (deliberate, tracked in review)

Two gaps are known and left for a follow-up, both false negatives — the rules stay silent where they could speak, never the reverse:

  • Test-class constructors and Dispose/DisposeAsync are not analyzed. They run per test inside the parallel chunk but carry none of the fixture attributes GetEnclosingTestMethod looks for.
  • A class-level [DoNotParallelize]/[ResourceLock] incorrectly suppresses diagnostics on [GlobalTestInitialize]/[GlobalTestCleanup]. Global fixtures are collected assembly-wide and run around every test, so the declaring class's opt-out does not serialize them; only the assembly-level gate does.

Both share one root cause — GetEnclosingTestMethod collapses every fixture kind into a bare IMethodSymbol, discarding the kind each fix needs — so they are best fixed together by preserving fixture kind and applying per-kind opt-out, lock and fix-scope semantics, rather than patched separately here.
This analyzer owns the statically certain subset only. The forthcoming parallel-safety-audit skill owns the judgement-requiring cases (paths traced through helpers, declared-vs-observed lock reconciliation, over-serialization) where lower precision is acceptable because a human reads the output.

Testing

62 unit tests across the four rules (C# and VB), weighted toward negative cases — declared-lock silent, non-test-class silent, variable-path File.* silent, read-mode File.Open silent, per-thread/ambient CurrentCulture silent, reads silent, [Parallelize]-absent silent, assembly [DoNotParallelize] silent, a method-level [DoNotParallelize] on a fixture method still flagged (discovery ignores it there), and File.Copy(<const source>, <var dest>) silent (the source is read). Every positive test now asserts the rendered message content (the offending API / resource key / path) via .WithArguments, not merely the diagnostic's presence. Attribute inheritance follows the runtime: the adapter reads member attributes with inherit: true, so an override inheriting [DoNotParallelize] or [ResourceLock] (both Inherited = true) stays silent, while [TestMethod] (Inherited = false) is deliberately not inherited when classifying test methods. Positive regressions cover compound (+=) assignments, R3's coalescing (??=) culture assignment, R2's coalescing form staying silent, fixture-scope fix placement, record test classes, derived [DoNotParallelize], R4's per-API path roles, and a fixture-scope [ResourceLock] still being flagged (discovery reads method-level locks only from real test methods, so a lock on [TestInitialize] has no runtime effect). Assembly-scoped fixtures are covered on both sides: [AssemblyInitialize] and [AssemblyCleanup] stay silent, while [ClassInitialize] keeps firing to pin the boundary so the exclusion cannot silently widen. The shared firing gate is covered directly on both of its config paths — mstest_parallel_safety_mode = always fires without [assembly: Parallelize] via a .globalconfig (GlobalOptions) and via an ordinary [*.cs] .editorconfig section (the per-syntax-tree fallback), [assembly: DoNotParallelize] silences, and the assembly opt-out wins when both are present. Anti-vacuous check: forcing the parallelization gate off makes all positive tests fail, and each review-fix regression test was individually confirmed non-vacuous by inverting its production rule and observing exactly the matching tests fail — including in the negative direction, where a "stays silent" test is re-checked by removing the suppressing construct and confirming the diagnostic reappears, so silence means real suppression rather than an analyzer that never ran. .resx updated and .xlf regenerated via UpdateXlf (never hand-edited).


Please do not add reviewers or self-merge; awaiting maintainer review.

…077)

Introduces the diagnostic ids, well-known type/resource-key names, the shared ParallelSafetyHelper (parallelization-in-effect gate, enclosing-test detection, resource-lock reconciliation), the AnalyzerReleases.Unshipped entries, and the localized diagnostic/code-fix strings consumed by the four rules.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
R1/MSTEST0074 (Info) flags undeclared Environment.SetEnvironmentVariable and Console.Set{Out,Error,In}; R2/MSTEST0075 (Warning) flags Environment.CurrentDirectory / Directory.SetCurrentDirectory when undeclared; R3/MSTEST0076 (Warning) flags CultureInfo.DefaultThreadCurrent[UI]Culture and Thread.CurrentThread.Current[UI]Culture (CurrentCulture/UICulture omitted as ExecutionContext-flowed); R4/MSTEST0077 (Info) flags a constant/relative path literal passed directly to a filesystem-mutating File.*/Directory.* API. AddResourceLockFixer provides the [ResourceLock(...)] code fix for R1 and R2.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
39 tests across the four rules, each with positive cases and (weighted more heavily) negative cases: declared-lock stays silent, non-test class stays silent, variable-path File.* stays silent, CultureInfo.CurrentCulture stays silent, reads stay silent, and diagnostics are gated off when parallelization is not in effect. The R4 tests encode the narrowing decision -- Path.Combine(Path.GetTempPath(), const) construction and read APIs are asserted silent.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
Copilot AI review requested due to automatic review settings July 27, 2026 08:43
@github-actions

This comment has been minimized.

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

Adds MSTEST0074–MSTEST0077 to detect unsafe global-state and filesystem mutations in parallel MSTest suites.

Changes:

  • Adds four parallel-safety analyzers and shared detection helpers.
  • Adds C# resource-lock fixes for MSTEST0074/0075.
  • Adds analyzer tests, release metadata, and localized resources.
Show a summary per file
File Description
test/UnitTests/MSTest.Analyzers.UnitTests/UndeclaredProcessGlobalStateMutationAnalyzerTests.cs Tests MSTEST0074 and fixes.
test/UnitTests/MSTest.Analyzers.UnitTests/SharedFileSystemPathInTestAnalyzerTests.cs Tests MSTEST0077 path detection.
test/UnitTests/MSTest.Analyzers.UnitTests/CurrentDirectoryMutationUnderParallelizationAnalyzerTests.cs Tests MSTEST0075 and fixes.
test/UnitTests/MSTest.Analyzers.UnitTests/CultureMutationUnderParallelizationAnalyzerTests.cs Tests MSTEST0076 behavior.
src/Analyzers/MSTest.Analyzers/UndeclaredProcessGlobalStateMutationAnalyzer.cs Implements MSTEST0074.
src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs Implements MSTEST0077.
src/Analyzers/MSTest.Analyzers/CurrentDirectoryMutationUnderParallelizationAnalyzer.cs Implements MSTEST0075.
src/Analyzers/MSTest.Analyzers/CultureMutationUnderParallelizationAnalyzer.cs Implements MSTEST0076.
src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs Shares parallelization and attribute checks.
src/Analyzers/MSTest.Analyzers/Helpers/WellKnownResourceKeys.cs Mirrors well-known lock keys.
src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs Adds referenced BCL type names.
src/Analyzers/MSTest.Analyzers/Helpers/DiagnosticIds.cs Registers MSTEST0074–0077 IDs.
src/Analyzers/MSTest.Analyzers/Resources.resx Adds analyzer messages.
src/Analyzers/MSTest.Analyzers/AnalyzerReleases.Unshipped.md Records the new rules.
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/AddResourceLockFixer.cs Implements resource-lock fixes.
src/Analyzers/MSTest.Analyzers.CodeFixes/CodeFixResources.resx Adds the fix title.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.cs.xlf Adds Czech fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.de.xlf Adds German fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.es.xlf Adds Spanish fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.fr.xlf Adds French fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.it.xlf Adds Italian fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ja.xlf Adds Japanese fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ko.xlf Adds Korean fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.pl.xlf Adds Polish fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.pt-BR.xlf Adds Portuguese fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.ru.xlf Adds Russian fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.tr.xlf Adds Turkish fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.zh-Hans.xlf Adds Simplified Chinese fix resources.
src/Analyzers/MSTest.Analyzers.CodeFixes/xlf/CodeFixResources.zh-Hant.xlf Adds Traditional Chinese fix resources.

Review details

Comments suppressed due to low confidence (6)

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:79

  • Assembly initialize/cleanup run once outside the parallel test scheduler, so mutations there cannot race sibling tests. Including these attributes makes every rule emit false positives for safe assembly setup/teardown and can offer an ineffective method-level lock fix. Keep per-test, class, and global fixtures, but exclude assembly fixtures.
        AddIfPresent(WellKnownTypeNames.MicrosoftVisualStudioTestToolsUnitTestingAssemblyInitializeAttribute);
        AddIfPresent(WellKnownTypeNames.MicrosoftVisualStudioTestToolsUnitTestingAssemblyCleanupAttribute);

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:119

  • Returning the fixture method makes downstream checks treat [DoNotParallelize] or [ResourceLock] placed on that fixture as effective. The runtime only reads these scheduling attributes from the discovered test method and test class (TypeEnumerator.cs:156-160), so decorating TestInitialize still leaves its mutation racing while the analyzer becomes silent. Fixture diagnostics must evaluate the effective test/class scheduling scope instead of the fixture's own attributes.
                if (fixtureAttributeSymbols.Contains(attributeClass))
                {
                    return method;

src/Analyzers/MSTest.Analyzers/CultureMutationUnderParallelizationAnalyzer.cs:134

  • Suppressing the warning for any resource lock creates a false negative: [ResourceLock("Database")] does not coordinate culture with another test using [ResourceLock("Culture")] or no lock. Since resource keys are opaque and there is no canonical culture key, an unrelated lock provides no evidence that this mutation is protected. Keep reporting unless the test is actually non-parallel, or introduce a specific documented culture key that can be compared exactly.
        // No well-known culture resource key exists, so treat any declared [ResourceLock] as the author having
        // coordinated culture access and stay silent rather than guessing which custom key maps to culture.
        if (ParallelSafetyHelper.HasAnyResourceLock(testMethod, resourceLockAttributeSymbol))
        {
            return;

src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs:159

  • Treating every sourceFileName as a mutated path makes read-only sources trigger. For example, File.Copy("shared-fixture.txt", uniqueDestination) only reads the fixed fixture (which the rule explicitly permits), but this method reports it before examining the variable destination. Path roles must be method-specific: Copy mutates only its destination, while Move mutates both.
            if (parameter.Name is not ("path" or "sourceFileName" or "destFileName" or "sourceDirName" or "destDirName"))
            {

src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs:138

  • File.Open is not necessarily mutating: File.Open(path, FileMode.Open, FileAccess.Read) is a read-only API call, but this name-only allowlist reports it. Inspect the FileMode/FileAccess arguments and skip read-only opens rather than classifying every overload as a write.
                or "OpenWrite" or "Open"

src/Analyzers/MSTest.Analyzers/CultureMutationUnderParallelizationAnalyzer.cs:111

  • On modern .NET, Thread.CurrentCulture's setter delegates directly to CultureInfo.CurrentCulture = value (current dotnet/runtime Thread.cs), so Thread.CurrentThread.CurrentCulture has the same ExecutionContext-flowed behavior that this rule intentionally treats as safe. Reporting it as a pooled-thread leak therefore produces a warning for safe code on .NET Core/.NET 5+. Either restrict this branch to .NET Framework compilations or omit it on modern targets.
        else if (threadSymbol is not null
            && property.Name is "CurrentCulture" or "CurrentUICulture"
            && SymbolEqualityComparer.Default.Equals(containingType, threadSymbol))
        {
            api = $"Thread.CurrentThread.{property.Name}";
  • Files reviewed: 42/42 changed files
  • Comments generated: 7
  • Review effort level: Medium

Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers.CodeFixes/AddResourceLockFixer.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers/CultureMutationUnderParallelizationAnalyzer.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers.CodeFixes/AddResourceLockFixer.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs Fixed
Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs Fixed
Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs Dismissed
An enabled-by-default analyzer may only use Warning severity when it
reports a known runtime break, because many consumers build with
TreatWarningsAsErrors and a new default warning fails their build.

Parallel-safety races (R2 current-directory, R3 culture) are latent,
configuration-dependent risks that only manifest once in-assembly
parallelization is enabled -- not unconditional runtime breaks -- so
they are lowered from Warning to Info while staying enabled by default,
matching R1 (MSTEST0074) and R4 (MSTEST0077).

Also documents this policy in docs/AddingAnalyzerCodeFix.md.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
Copilot AI review requested due to automatic review settings July 27, 2026 09:02

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.

Review details

Comments suppressed due to low confidence (12)

src/Analyzers/MSTest.Analyzers.CodeFixes/AddResourceLockFixer.cs:1

  • This new C# file is UTF-8 without a BOM, while .editorconfig:66-67 requires utf-8-bom for every *.cs file. Please resave it with the BOM so it follows the repository encoding rule.
// Copyright (c) Microsoft Corporation. All rights reserved.

src/Analyzers/MSTest.Analyzers.CodeFixes/AddResourceLockFixer.cs:64

  • For a diagnostic in [TestInitialize], [TestCleanup], or another fixture, this selects the fixture method and adds the lock there. Runtime discovery only merges locks from the test method and test class (TypeEnumerator.cs:160, 216-239), so the generated attribute is ignored even though the analyzer then considers the diagnostic fixed. Target the containing test class for applicable fixtures, and avoid offering this fix where no effective lock target exists (for example assembly/global fixtures).
        SyntaxToken syntaxToken = root.FindToken(diagnosticSpan.Start);
        MethodDeclarationSyntax? methodDeclaration = syntaxToken.Parent?.AncestorsAndSelf().OfType<MethodDeclarationSyntax>().FirstOrDefault();
        if (methodDeclaration is null)

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:159

  • [DoNotParallelize] also targets assemblies, but this helper checks only the method and containing types. With mstest_parallel_safety_mode = always, an assembly-level opt-out therefore still produces R1-R3 diagnostics even though the entire assembly runs sequentially. Include method.ContainingAssembly in the attribute check.
        if (HasAttribute(method, doNotParallelizeAttributeSymbol))
        {
            return true;
        }

        for (INamedTypeSymbol? type = method.ContainingType; type is not null; type = type.BaseType)

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:188

  • When method is a fixture method, a method-level [ResourceLock] is not an effective runtime declaration: discovery only reads locks from each test method and its test class (TypeEnumerator.cs:160, 216-239). Returning true here suppresses R1/R2 for a manually annotated fixture even though its mutation remains uncoordinated. Fixture methods need scope-aware handling rather than accepting their own attribute.
        if (DeclaresResourceLock(method, resourceLockAttributeSymbol, resourceKey))
        {
            return true;

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:244

  • This treats a matching lock key as sufficient without checking ResourceLockAttribute.Mode. A mutating test annotated with Mode = ResourceAccessMode.Read can run concurrently with another read holder, so the environment/current-directory race remains while R1/R2 are suppressed. Only an exclusive ReadWrite declaration should satisfy mutation rules.
            if (attribute.ConstructorArguments.Length > 0
                && attribute.ConstructorArguments[0].Value is string declaredKey
                && string.Equals(declaredKey, resourceKey, StringComparison.Ordinal))
            {
                return true;

src/Analyzers/MSTest.Analyzers/CultureMutationUnderParallelizationAnalyzer.cs:134

  • An arbitrary lock such as [ResourceLock("Database")] gives no evidence that culture access is coordinated; resource keys only conflict by exact equality. Suppressing R3 for any lock recreates the fail-open behavior this analyzer is intended to prevent and misses definite DefaultThreadCurrentCulture mutations. Since there is no well-known culture key, keep reporting unless the test is actually non-parallelized (or introduce a verifiable culture key).
        // No well-known culture resource key exists, so treat any declared [ResourceLock] as the author having
        // coordinated culture access and stay silent rather than guessing which custom key maps to culture.
        if (ParallelSafetyHelper.HasAnyResourceLock(testMethod, resourceLockAttributeSymbol))
        {
            return;

src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs:138

  • Classifying every File.Open overload as mutating causes a deterministic false positive for File.Open("fixture.txt", FileMode.Open, FileAccess.Read), contradicting the rule's promise to exclude reads. Inspect FileMode/FileAccess and report only modes that create, truncate, append, or grant write access.
                or "OpenWrite" or "Open"

src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs:160

  • The generic parameter-name scan is incorrect for multi-path APIs. For example, File.Copy("shared-fixture", uniqueDestination) reports the read-only source as a collision, while File.Replace(variableSource, "shared-destination", "shared-backup") is missed because the actual parameter names are destinationFileName and destinationBackupFileName. Select the mutated path parameters per target method instead of treating every recognized string parameter alike.
            if (parameter.Name is not ("path" or "sourceFileName" or "destFileName" or "sourceDirName" or "destDirName"))
            {
                continue;

src/Analyzers/MSTest.Analyzers/CurrentDirectoryMutationUnderParallelizationAnalyzer.cs:82

  • Registering only SimpleAssignment misses valid setter mutations such as Environment.CurrentDirectory += "/subdir" (CompoundAssignment). That operation still writes the process-global property and should produce R2. Register the assignment kinds that can invoke the setter and analyze them through their common assignment target.
                context.RegisterOperationAction(
                    context => AnalyzeAssignment(context, environmentSymbol, testMethodAttributeSymbol, fixtureAttributeSymbols, doNotParallelizeAttributeSymbol, resourceLockAttributeSymbol),
                    OperationKind.SimpleAssignment);

src/Analyzers/MSTest.Analyzers/CultureMutationUnderParallelizationAnalyzer.cs:73

  • CultureInfo.DefaultThreadCurrentCulture ??= value is a CoalesceAssignment, not a SimpleAssignment, and it mutates the process-wide default when currently null. R3 silently misses this setter form. Register and analyze coalesce assignments through a shared assignment-target path.
            context.RegisterOperationAction(
                context => AnalyzeAssignment(context, cultureInfoSymbol, threadSymbol, testMethodAttributeSymbol, fixtureAttributeSymbols, doNotParallelizeAttributeSymbol, resourceLockAttributeSymbol),
                OperationKind.SimpleAssignment);

src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs:120

  • R4 never checks whether this test/class is marked [DoNotParallelize], unlike R1-R3. It therefore reports a shared-path collision for code that is guaranteed to run in the sequential phase. Resolve the attribute symbol and apply IsOptedOutOfParallelization before reporting.
        context.ReportDiagnostic(invocation.CreateDiagnostic(Rule, offendingPath));

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:79

  • AssemblyInitialize is serialized before any test body proceeds (other workers wait for RunAssemblyInitializeAsync), and AssemblyCleanup runs after the assembly's tests finish. Including these attributes reports process-global mutations where no in-assembly concurrency exists, and no method-level resource lock can affect these lifecycle phases. Exclude assembly initialize/cleanup from the test-chunk fixture set.
        AddIfPresent(WellKnownTypeNames.MicrosoftVisualStudioTestToolsUnitTestingAssemblyInitializeAttribute);
        AddIfPresent(WellKnownTypeNames.MicrosoftVisualStudioTestToolsUnitTestingAssemblyCleanupAttribute);
  • Files reviewed: 43/43 changed files
  • Comments generated: 4
  • Review effort level: Medium

Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers/CultureMutationUnderParallelizationAnalyzer.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs Outdated
@github-actions

This comment has been minimized.

@Evangelink
Evangelink enabled auto-merge (squash) July 27, 2026 11:01
Evangelink and others added 2 commits July 27, 2026 13:02
…te, fixture-aware fix scope

- R3 now flags only CultureInfo.DefaultThreadCurrent[UI]Culture (process-wide static
  field). Thread.CurrentThread.Current* and ambient CurrentCulture/CurrentUICulture are
  AsyncLocal-backed on modern .NET (flow with ExecutionContext), so they neither corrupt
  siblings nor leak onto pooled threads and are intentionally not flagged.
- Firing gate: assembly [DoNotParallelize] disables all rules; otherwise fire when
  [assembly: Parallelize] is present OR the editorconfig knob mstest_parallel_safety_mode
  = always is set (scans every syntax tree, fixing the editorconfig blind spot).
- Fix scope: discovery reads resource locks only from the test class and test method, so
  the fixer places the lock on the class for class-scoped fixtures, on the method for test
  methods, and offers no fix for assembly/global fixtures.
- Handle compound (+=) and coalesce (??=) assignments as mutations.
- Expand R4 mutating filesystem method set (symlink/attributes/timestamps).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
- Culture: Thread.CurrentThread.Current* is now a negative (AsyncLocal-backed, not flagged);
  add a ??= regression positive for DefaultThreadCurrentCulture.
- R1 fixture fix now targets the enclosing class for [TestInitialize]; add [AssemblyInitialize]
  diagnostic-but-no-fix case (no effective lock target).
- R2: add compound-assignment (+=) regression positive.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
Copilot AI review requested due to automatic review settings July 27, 2026 11:02
Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs
Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs

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.

Review details

Comments suppressed due to low confidence (6)

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:343

  • A read-mode lock is currently treated as protection for a write. [ResourceLock(key, Mode = ResourceAccessMode.Read)] permits concurrent holders, so R1/R2 become silent even though the mutation is still racing. Check the Mode named argument and suppress only for the default/exclusive ReadWrite mode; the same rule must also be applied to HasAnyResourceLock for R3.
            if (attribute.ConstructorArguments.Length > 0
                && attribute.ConstructorArguments[0].Value is string declaredKey
                && string.Equals(declaredKey, resourceKey, StringComparison.Ordinal))
            {
                return true;

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:286

  • This checks a fixture method's own [ResourceLock], although discovery ignores resource locks on fixture methods and reads them only from the test class and actual test method (TypeEnumerator.cs:228-236). Thus [TestInitialize, ResourceLock(...)] incorrectly suppresses R1/R2 while providing no serialization. Skip method-level locks when the enclosing method is a fixture.
        if (DeclaresResourceLock(method, resourceLockAttributeSymbol, resourceKey))
        {

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:351

  • DoNotParallelizeAttribute is non-sealed, and MSTest's IsAttributeDefined<T> explicitly treats derived attributes as matches (ReflectionOperations.cs:103-125). This exact equality makes a custom [Sequential] : DoNotParallelizeAttribute test run sequentially at runtime but still receive all four diagnostics. Use the same inheritance-aware symbol check as test-method detection.
    private static bool HasAttribute(ISymbol symbol, INamedTypeSymbol attributeSymbol)
        => symbol.GetAttributes().Any(attribute => SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, attributeSymbol));

src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs:148

  • Classifying only by method name creates both false positives and false negatives. File.Open("fixture", FileMode.Open, FileAccess.Read) is read-only but is reported, while File.OpenHandle("shared", FileMode.Create, FileAccess.Write), File.Encrypt, and File.Decrypt mutate a fixed path but are omitted. Make the Open/OpenHandle decision from FileMode/FileAccess and include the unconditional mutators.
                or "Create" or "CreateText" or "CreateSymbolicLink"
                or "Copy" or "Move" or "Replace" or "Delete"
                or "OpenWrite" or "Open"
                or "SetAttributes" or "SetUnixFileMode"

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:316

  • This suppresses R3 for any method-level lock without checking whether the lock is effective or exclusive. A [ResourceLock("Culture", Mode = ResourceAccessMode.Read)] still allows concurrent culture writers, and the same attribute on a fixture method is ignored by discovery; both cases make the analyzer silent while the race remains. Restrict this to effective test/class locks in ReadWrite mode.
        if (HasAttribute(method, resourceLockAttributeSymbol))
        {
            return true;

src/Analyzers/MSTest.Analyzers/CurrentDirectoryMutationUnderParallelizationAnalyzer.cs:33

  • The PR's Rules table declares MSTEST0075 and MSTEST0076 as Warning, but both descriptors and AnalyzerReleases.Unshipped.md ship them as Info. Since this changes whether consumers see build warnings, please reconcile the PR contract with the implementation (the newly added severity policy appears to support Info).
        Category.Usage,
        DiagnosticSeverity.Info,
        isEnabledByDefault: true);
  • Files reviewed: 43/43 changed files
  • Comments generated: 5
  • Review effort level: Medium

Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers.CodeFixes/AddResourceLockFixer.cs
@github-actions

This comment has been minimized.

@Evangelink Evangelink added the state/needs-review Awaiting review from the team. label Jul 27, 2026
…re attributes, R4 path roles

- Only honor a method-level [DoNotParallelize] on a real test method: discovery ignores it on fixture methods, so a fixture mutation stays flagged unless the class/assembly opts out.
- AddResourceLockFixer now handles record test classes (TypeDeclarationSyntax, not ClassDeclarationSyntax).
- Attribute matching walks the inheritance chain (.Inherits) to mirror the adapter's reflective lookups, so subclasses of [Parallelize]/[DoNotParallelize]/[ResourceLock]/[TestMethod] are honored.
- R4 assigns a per-API path role: File.Copy flags only its destination, File.Move both positions, File.Replace destination+backup, mirroring which argument each API actually mutates.
- Expose each analyzer's Rule descriptor as public (matching DataRowShouldBeValidAnalyzer) so message-content assertions can reference it from the test assembly.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
…tests for the review fixes

- Convert positive cases from bare [|...|] markup to named {|#0:...|} spans plus .WithArguments, so every positive test now asserts the rendered offending API / resource key / path, not just the diagnostic's presence.
- Add regression tests for the production fixes: method-level vs fixture-level [DoNotParallelize], derived class-level [DoNotParallelize], record test class fix, and R4 Copy/Move/Replace path-role precision (constant source of Copy stays silent; constant destination/backup/move-source fire).
- Each new assertion was confirmed non-vacuous by inverting the corresponding production rule and observing the matching test fail.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
Copilot AI review requested due to automatic review settings July 27, 2026 12:09
@github-actions

This comment has been minimized.

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.

Review details

Comments suppressed due to low confidence (1)

src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs:211

  • File.Replace deletes sourceFileName after replacing the destination, so the source is mutated rather than merely read. With a constant source and variable destination/backup, this code currently misses the shared-path mutation.
                // Replace overwrites destinationFileName and creates destinationBackupFileName; sourceFileName is read.
                "Replace" => parameterName is "destinationFileName" or "destinationBackupFileName",
  • Files reviewed: 43/43 changed files
  • Comments generated: 9
  • Review effort level: Medium

Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs
Comment thread src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers.CodeFixes/AddResourceLockFixer.cs
Comment thread src/Analyzers/MSTest.Analyzers/Resources.resx Outdated
Comment thread src/Analyzers/MSTest.Analyzers.CodeFixes/AddResourceLockFixer.cs Outdated
Copilot AI review requested due to automatic review settings July 27, 2026 12:27
@github-actions

This comment has been minimized.

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.

Review details

Comments suppressed due to low confidence (4)

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:358

  • This treats a matching key as protection without checking ResourceLockAttribute.Mode. A mutation annotated with Mode = ResourceAccessMode.Read can run concurrently with another read holder, so MSTEST0074/0075 are suppressed for a still-racy mutation. Only an effective ReadWrite lock (default mode), including the strongest merged class/method declaration, should silence these rules.
            if (attribute.ConstructorArguments.Length > 0
                && attribute.ConstructorArguments[0].Value is string declaredKey
                && string.Equals(declaredKey, resourceKey, StringComparison.Ordinal))
            {
                return true;

src/Analyzers/MSTest.Analyzers/CultureMutationUnderParallelizationAnalyzer.cs:128

  • Any resource lock is not evidence that culture access is coordinated. For example, one test with [ResourceLock("Database")] and another with [ResourceLock("Culture")] can both mutate DefaultThreadCurrentCulture concurrently, yet this suppresses the diagnostic on both. Since there is no canonical culture key to match, keep reporting unless the test is non-parallelized, or introduce a canonical key that can be checked exactly.
        // No well-known culture resource key exists, so treat any declared [ResourceLock] as the author having
        // coordinated culture access and stay silent rather than guessing which custom key maps to culture.
        if (ParallelSafetyHelper.HasAnyResourceLock(testMethod, resourceLockAttributeSymbol))
        {
            return;

src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs:216

  • File.Replace consumes/deletes sourceFileName after replacing the destination; it is not a read-only source like File.Copy. Treating it as read misses a constant shared source that two parallel tests can race to remove. Include sourceFileName among the mutated path roles and add a regression test.
                // Replace overwrites destinationFileName and creates destinationBackupFileName; sourceFileName is read.
                "Replace" => parameterName is "destinationFileName" or "destinationBackupFileName",

src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs:132

  • The PR states that all four rules stay silent when a mutation has a [ResourceLock], but MSTEST0077 never resolves or checks ResourceLockAttribute; a protected fixed-path write still reports. Either define and implement the effective lock-matching semantics for this rule, or update the PR contract to exclude MSTEST0077 from that statement.
        // Opting out of parallelization (sequential phase) removes the collision risk entirely, so stay silent -
        // this matches R1/R2/R3 and keeps specimens like the [DoNotParallelize] environment-mutating tests quiet.
        if (ParallelSafetyHelper.IsOptedOutOfParallelization(testMethod, doNotParallelizeAttributeSymbol, testMethodAttributeSymbol))
        {
            return;
        }

        context.ReportDiagnostic(invocation.CreateDiagnostic(Rule, offendingPath));
  • Files reviewed: 43/43 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs Outdated
Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs
Round-4 review fixes for the parallel-safety analyzers.

1. Fixture-scope [ResourceLock] no longer suppresses MSTEST0074/0075/0076.
   TypeEnumerator.GetTestFromMethod merges the class locks with the locks read
   from the test method itself, and only ever while building a UnitTestElement
   for a discovered test method, so a [ResourceLock] on [TestInitialize] et al.
   has no runtime effect. HasResourceLockFor/HasAnyResourceLock now honor a
   method-level lock only on a real test method, mirroring the same narrowing
   already applied to IsOptedOutOfParallelization; the containing-type walk is
   unchanged, so a class-level lock still covers its fixtures.

2. Cover the shared firing gate, whose two nontrivial branches were untested
   even though all four rules depend on it: mstest_parallel_safety_mode =
   always without [assembly: Parallelize], the [assembly: DoNotParallelize]
   kill switch, and the precedence of the opt-out over the always opt-in.

Each new test was confirmed non-vacuous by inverting its production condition
(dropping the fixture guard, the always-mode branch, and the assembly opt-out
in turn) and observing exactly the matching tests fail. The class-level-lock
negative was additionally proven in the negative direction: removing the lock
from its snippet makes the diagnostic appear, so its silence is real
suppression rather than an analyzer that never ran.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
Copilot AI review requested due to automatic review settings July 27, 2026 12:49

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.

Review details

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

Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs Outdated
[AssemblyInitialize] and [AssemblyCleanup] cannot race a concurrent test, so
reporting a process-global mutation in either was a false positive.

Verified against the adapter rather than assumed: assembly initialize runs
under TestAssemblyInfo's SemaphoreSlim(1, 1) with a double-checked
IsAssemblyInitializeExecuted guard, and every worker awaits
RunAssemblyInitializeAsync before running its test, so no test body overlaps
it; assembly cleanup runs only after the last runnable test in the whole
assembly (ShouldRunEndOfAssemblyCleanup / _lastRunnableTestInWholeAssembly).
A mutation in either is ordinary global setup or teardown, not a race.

GetFixtureAttributeSymbols therefore drops the two assembly-scoped attributes.
Class- and test-scoped fixtures and the global per-test fixtures stay analyzed
because those genuinely can overlap a parallel test.

The existing assembly-initialize test flips from "diagnostic but no fix" to
no-diagnostic, and two tests are added: assembly cleanup stays silent, and
class initialize keeps firing - the latter pins the boundary of the exclusion
so it cannot silently widen.

All three were confirmed non-vacuous: re-adding the assembly attributes makes
exactly the two assembly negatives fail (the negative-direction check, proving
their silence is the exclusion rather than an analyzer that never ran), and
removing ClassInitialize from the analyzed set makes only the class-initialize
positive fail.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
Copilot AI review requested due to automatic review settings July 27, 2026 13:05

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.

Review details

Comments suppressed due to low confidence (1)

src/Analyzers/MSTest.Analyzers/SharedFileSystemPathInTestAnalyzer.cs:216

  • File.Replace deletes sourceFileName after replacing the destination, so a constant source is also a mutated shared path. Treating it as read-only misses calls such as File.Replace("shared.tmp", uniqueDestination, null). Include sourceFileName in this case and add a regression test.
                // Replace overwrites destinationFileName and creates destinationBackupFileName; sourceFileName is read.
                "Replace" => parameterName is "destinationFileName" or "destinationBackupFileName",
  • Files reviewed: 43/43 changed files
  • Comments generated: 2
  • Review effort level: Medium

…g branch

1. MSTEST0075 no longer registers OperationKind.CoalesceAssignment.
   Environment.CurrentDirectory is declared non-nullable (public static string
   CurrentDirectory) and throws rather than returning null on failure, so
   'Environment.CurrentDirectory ??= x' can never reach the setter and flagging
   it was a guaranteed false positive. The positive test becomes a
   no-diagnostic regression.

   This narrows, rather than reverses, the earlier review change that added the
   operation kind for parity with R3: R3 keeps handling '??=' because
   CultureInfo.DefaultThreadCurrentCulture is declared nullable and the
   coalescing form genuinely can write there. Verified rather than assumed - a
   probe shows 'string a = Environment.CurrentDirectory' produces no CS8600
   while 'CultureInfo b = CultureInfo.DefaultThreadCurrentCulture' does, so the
   two properties really do differ in declared nullability. The reason R2 now
   registers fewer operation kinds than R3 is recorded in a comment so the
   divergence is not "fixed" back later.

2. Cover IsAlwaysModeConfigured's per-syntax-tree fallback. The existing gate
   test used a .globalconfig, which only exercises GlobalOptions; the fallback
   exists specifically for ordinary [*.cs] .editorconfig sections and was
   untested.

Both confirmed non-vacuous, and the second check is discriminating: re-adding
CoalesceAssignment fails only the new coalesce negative, and disabling the
per-tree loop fails only the new .editorconfig test while the .globalconfig
test keeps passing - so the two gate tests demonstrably cover different
branches.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
Copilot AI review requested due to automatic review settings July 27, 2026 13:17

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.

Review details

Comments suppressed due to low confidence (2)

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:313

  • Runtime attribute lookup includes inherited method attributes (ReflectionOperations.cs:31-34), and ResourceLockAttribute is explicitly Inherited = true. If an override reapplies [TestMethod] but inherits its matching resource lock from the base method, DeclaresResourceLock checks only the override's direct attributes and MSTEST0074/0075 report despite the effective lock. Include overridden methods when resolving method-level locks.
        if (IsTestMethod(method, testMethodAttributeSymbol)
            && DeclaresResourceLock(method, resourceLockAttributeSymbol, resourceKey))
        {

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:347

  • This direct-only lookup also misses an inherited method-level [ResourceLock] on an override, although runtime discovery uses inherited attributes. For MSTEST0076 that means an effectively coordinated override is still diagnosed because the base method's lock is never seen. Apply the same OverriddenMethod traversal used for matching keyed locks.
        if (IsTestMethod(method, testMethodAttributeSymbol)
            && HasAttribute(method, resourceLockAttributeSymbol))
        {
  • Files reviewed: 43/43 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs
Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs Outdated
Runtime discovery reads member attributes through
ReflectionOperations.GetCustomAttributes, which passes inherit: true, so an
override inherits [DoNotParallelize] and [ResourceLock] from the method it
overrides. The analyzers inspected only the Roslyn symbol's own attributes and
so missed those opt-outs, reporting mutations the runtime had already
serialized or locked - a false positive, the direction these rules must never
take.

The method-level checks in IsOptedOutOfParallelization, HasResourceLockFor and
HasAnyResourceLock now walk the OverriddenMethod chain.

IsTestMethod deliberately does NOT walk it. TestMethodAttribute is declared
[AttributeUsage(..., Inherited = false)], so the runtime does not surface a
base method's [TestMethod] on an override; walking there would classify a plain
override as a test method when the runtime does not, which would itself be a
false positive. Verified per attribute rather than applied uniformly:
DoNotParallelizeAttribute leaves Inherited at its default of true,
ResourceLockAttribute sets Inherited = true explicitly, and
TestMethodAttribute sets Inherited = false. The reason for the asymmetry is
recorded on IsTestMethod so it is not "fixed" into consistency later.

Two regression tests cover an override inheriting each of the two inheritable
attributes; both confirmed non-vacuous by disabling the chain walk and
observing exactly those two fail.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 27ae56b0-f7b7-4863-8bca-62c8d7f47466
Copilot AI review requested due to automatic review settings July 27, 2026 13:30

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.

Review details

Comments suppressed due to low confidence (2)

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:322

  • A class-level [ResourceLock] does not cover a global per-test fixture: global fixtures are discovered into the assembly-wide list and run for every test, while class locks are read only when building tests for that class (TypeEnumerator.cs:213-239). This lookup therefore makes MSTEST0074/0075 silently accept an ineffective lock when the global fixture's declaring class happens to carry the matching key. Skip containing-type locks for global fixtures and keep reporting them without a fix.
        for (INamedTypeSymbol? type = method.ContainingType; type is not null; type = type.BaseType)
        {
            if (DeclaresResourceLock(type, resourceLockAttributeSymbol, resourceKey))
            {
                return true;
            }

src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs:355

  • The same global-fixture leak affects the culture rule here. A [ResourceLock] on the class that merely declares [GlobalTestInitialize] is never acquired by every test that invokes that assembly-wide fixture, but HasAnyResourceLock suppresses MSTEST0076 anyway. Do not consider containing-type locks for global fixtures.
        for (INamedTypeSymbol? type = method.ContainingType; type is not null; type = type.BaseType)
        {
            if (HasAttribute(type, resourceLockAttributeSymbol))
            {
                return true;
  • Files reviewed: 43/43 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread src/Analyzers/MSTest.Analyzers/Helpers/ParallelSafetyHelper.cs
@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test quality grade — PR #10248

62 test methods graded across 4 new files. All grade A (90–100). The suite is well-crafted: every diagnostic test verifies both the source location and the API name rendered in the diagnostic message (not just that the diagnostic fires), code-fix tests verify the exact transformed output, and every suppression guard has a corresponding no-diagnostic test that kills the remove-suppression mutation. Coverage includes VisualBasic variants, editorconfig firing gates, fixture-scope precision, derived attributes, record types, and inheritance-propagated locks.

Remaining 47 tests (all A 90–100)
GradeTestMutationNotesHow to improve
A (90–100) new CultureMutationUnderParallelizationAnalyzerTests.
WhenDoNotParallelizeDeclared_
NoDiagnostic
1/1 killed No-fire assertion kills the remove-DoNotParallelize-check mutation cleanly.
A (90–100) new CultureMutationUnderParallelizationAnalyzerTests.
WhenNonTestClass_
NoDiagnostic
1/1 killed No-fire assertion kills the remove-test-class-check mutation cleanly.
A (90–100) new CultureMutationUnderParallelizationAnalyzerTests.
WhenParallelizeNotDeclared_
NoDiagnostic
1/1 killed No-fire assertion kills the remove-parallelization-gate mutation cleanly.
A (90–100) new CultureMutationUnderParallelizationAnalyzerTests.
WhenReadingDefaultThreadCurrentCulture_
NoDiagnostic
1/1 killed No-fire assertion kills the flag-reads-too mutation; read vs. write distinction guarded.
A (90–100) new CultureMutationUnderParallelizationAnalyzerTests.
WhenResourceLockDeclared_
NoDiagnostic
1/1 killed No-fire assertion kills the remove-ResourceLock-suppression mutation cleanly.
A (90–100) new CultureMutationUnderParallelizationAnalyzerTests.
WhenTestMethodNullCoalesceAssignsDefaultThreadCurrentCulture_
Diagnostic
3/3 killed Verifies ??= operator fires; checks location and API name. Comment explains nullable distinction from CurrentDirectory.
A (90–100) new CultureMutationUnderParallelizationAnalyzerTests.
WhenTestMethodSetsCurrentCulture_
NoDiagnostic
1/1 killed No-fire assertion guards AsyncLocal-backed CurrentCulture from being over-flagged.
A (90–100) new CultureMutationUnderParallelizationAnalyzerTests.
WhenTestMethodSetsDefaultThreadCurrentCulture_
Diagnostic
3/3 killed Verifies diagnostic fires at location 0 with API name "CultureInfo.DefaultThreadCurrentCulture" in message.
A (90–100) new CultureMutationUnderParallelizationAnalyzerTests.
WhenTestMethodSetsDefaultThreadCurrentCulture_
VisualBasic_
Diagnostic
3/3 killed VB variant verifies location and API name; guards against language-specific registration gaps.
A (90–100) new CultureMutationUnderParallelizationAnalyzerTests.
WhenTestMethodSetsDefaultThreadCurrentUICulture_
Diagnostic
3/3 killed Verifies UIculture form fires with correct argument; distinct from culture form.
A (90–100) new CultureMutationUnderParallelizationAnalyzerTests.
WhenTestMethodSetsThreadCurrentCulture_
NoDiagnostic
1/1 killed No-fire guards Thread.CurrentThread.CurrentCulture from being over-flagged; comment explains AsyncLocal rationale.
A (90–100) new CurrentDirectoryMutationUnderParallelizationAnalyzerTests.
WhenDoNotParallelizeDeclared_
NoDiagnostic
1/1 killed No-fire assertion kills the remove-DoNotParallelize-suppression mutation.
A (90–100) new CurrentDirectoryMutationUnderParallelizationAnalyzerTests.
WhenNonTestClass_
NoDiagnostic
1/1 killed No-fire assertion kills the remove-test-class-check mutation.
A (90–100) new CurrentDirectoryMutationUnderParallelizationAnalyzerTests.
WhenParallelizeNotDeclared_
NoDiagnostic
1/1 killed No-fire assertion kills the remove-parallelization-gate mutation.
A (90–100) new CurrentDirectoryMutationUnderParallelizationAnalyzerTests.
WhenReadingCurrentDirectory_
NoDiagnostic
1/1 killed No-fire guards reads from being flagged; kills flag-reads-too mutation.
GradeTestMutationNotesHow to improve
A (90–100) new CurrentDirectoryMutationUnderParallelizationAnalyzerTests.
WhenResourceLockDeclared_
NoDiagnostic
1/1 killed No-fire kills the remove-ResourceLock-suppression mutation.
A (90–100) new CurrentDirectoryMutationUnderParallelizationAnalyzerTests.
WhenTestMethodCoalesceAssignsEnvironmentCurrentDirectory_
NoDiagnostic
1/1 killed No-fire kills the flag-??=-on-non-nullable mutation; comment explains non-nullable rationale vs. CultureInfo.
A (90–100) new CurrentDirectoryMutationUnderParallelizationAnalyzerTests.
WhenTestMethodCompoundAssignsEnvironmentCurrentDirectory_
Diagnostic
4/4 killed Code-fix test; verifies += fires with correct location, API name, and ResourceLock is added to method.
A (90–100) new CurrentDirectoryMutationUnderParallelizationAnalyzerTests.
WhenTestMethodSetsCurrentDirectory_
VisualBasic_
Diagnostic
3/3 killed VB variant verifies location and API name for Directory.SetCurrentDirectory.
A (90–100) new CurrentDirectoryMutationUnderParallelizationAnalyzerTests.
WhenTestMethodSetsCurrentDirectoryViaDirectory_
Diagnostic
4/4 killed Code-fix test; verifies location, API name in message, and ResourceLock(WellKnownResources.CurrentDirectory) added.
A (90–100) new CurrentDirectoryMutationUnderParallelizationAnalyzerTests.
WhenTestMethodSetsEnvironmentCurrentDirectory_
Diagnostic
4/4 killed Code-fix test; verifies Environment.CurrentDirectory assignment fires with correct location, API name, and fix.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenNonTestClass_
NoDiagnostic
1/1 killed No-fire kills the remove-test-class-check mutation.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenParallelizeNotDeclared_
NoDiagnostic
1/1 killed No-fire kills the remove-parallelization-gate mutation.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodCopiesToConstantDestination_
Diagnostic
3/3 killed Verifies File.Copy constant destination fires; parameter-position precision (source not flagged, dest is).
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodCopiesFromConstantSource_
NoDiagnostic
1/1 killed No-fire guards File.Copy source from being flagged; comment explains read-only source rationale.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodCreatesDirectoryWithConstantPath_
Diagnostic
3/3 killed Verifies Directory.CreateDirectory fires with correct location and path in message.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodMovesFromConstantSource_
Diagnostic
3/3 killed Verifies File.Move constant source fires (Move deletes source, distinguishing from Copy); path in message.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodOpensFileForReading_
NoDiagnostic
1/1 killed No-fire guards File.Open from being flagged; comment explains read-only handle exclusion rationale.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodReadsConstantPath_
NoDiagnostic
1/1 killed No-fire guards read-only APIs (ReadAllText, Exists) from being flagged.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodReplacesWithConstantBackup_
Diagnostic
3/3 killed Verifies File.Replace backup path fires; parameter-position precision (source not flagged, backup is).
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodUsesSharedTempCombine_
NoDiagnostic
1/1 killed No-fire guards Path.Combine from being flagged; comment explains construction vs. mutation distinction.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodUsesUniqueTempFileName_
NoDiagnostic
1/1 killed No-fire guards per-test unique temp paths from being flagged.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodUsesVariablePath_
NoDiagnostic
1/1 killed No-fire guards variable paths from being flagged; comment explains static analysis scope limitation.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodWritesAbsolutePathLiteral_
Diagnostic
3/3 killed Verifies Windows absolute path fires with correct location and offending path "C:\temp\shared.txt" in message.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodWritesAbsolutePathLiteral_
VisualBasic_
Diagnostic
3/3 killed VB variant verifies location and path in message for File.WriteAllText.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodWritesRelativePathLiteral_
Diagnostic
3/3 killed Verifies relative paths also fire; path "output.txt" in message.
A (90–100) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethodWritesUnixAbsolutePathLiteral_
Diagnostic
3/3 killed Verifies Unix absolute path "/tmp/shared.txt" fires with correct location and path in message.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenAlwaysModeConfiguredInEditorConfigSection_
Diagnostic
4/4 killed Verifies per-tree editorconfig path fires; guards the tree-scanning fallback in IsAlwaysModeConfigured.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenAlwaysModeConfiguredWithoutParallelize_
Diagnostic
4/4 killed Verifies globalconfig always-mode gate fires without Parallelize attribute; checks args and fix output.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenAssemblyDoNotParallelizeWithAlwaysMode_
NoDiagnostic
1/1 killed No-fire verifies assembly opt-out wins over always-mode; precedence between two firing gates guarded.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenAssemblyDoNotParallelizeWithParallelize_
NoDiagnostic
1/1 killed No-fire verifies assembly-level DoNotParallelize kill-switch wins over Parallelize attribute.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenClassLevelResourceLockCoversFixture_
NoDiagnostic
1/1 killed No-fire guards class-level ResourceLock from over-firing on correctly-coordinated fixture mutations.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenDerivedDoNotParallelizeOnClass_
NoDiagnostic
1/1 killed No-fire guards derived DoNotParallelizeAttribute subclasses from being ignored; inheritance honored.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenDoNotParallelizeDeclared_
NoDiagnostic
1/1 killed No-fire kills the remove-DoNotParallelize-suppression mutation.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenFixtureMethodHasMethodLevelDoNotParallelize_
Diagnostic
4/4 killed Code-fix test; verifies DoNotParallelize on a fixture method is ineffective; fix lifts lock to class.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenFixtureMethodHasMethodLevelResourceLock_
Diagnostic
4/4 killed Code-fix test; verifies fixture-method ResourceLock is ignored by discovery; fix lifts lock to class.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenNotTestMethod_
NoDiagnostic
1/1 killed No-fire guards plain helper methods in test classes from being flagged.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenNonTestClass_
NoDiagnostic
1/1 killed No-fire kills the remove-test-class-check mutation.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenOverrideInheritsDoNotParallelizeFromBaseMethod_
NoDiagnostic
1/1 killed No-fire guards inherited DoNotParallelize (Inherited=true) from being ignored on overrides.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenOverrideInheritsResourceLockFromBaseMethod_
NoDiagnostic
1/1 killed No-fire guards inherited ResourceLock (Inherited=true) from being ignored on overrides.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenParallelizeNotDeclared_
NoDiagnostic
1/1 killed No-fire kills the remove-parallelization-gate mutation.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenReadingEnvironmentVariable_
NoDiagnostic
1/1 killed No-fire guards GetEnvironmentVariable reads from being flagged.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenRecordTestClassFixture_
FixAddedToRecord
3/3 killed Code-fix test; verifies fixer places ResourceLock on record declarations, not only class declarations.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenResourceLockDeclared_
NoDiagnostic
1/1 killed No-fire kills the remove-ResourceLock-suppression mutation.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenTestMethodHasMethodLevelDoNotParallelize_
NoDiagnostic
1/1 killed No-fire verifies method-level DoNotParallelize on a test method opts it out of parallelization.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenTestMethodSetsConsoleOut_
Diagnostic
4/4 killed Code-fix test; verifies Console.SetOut fires with API name and resource key "Console" in message; fix adds lock.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenTestMethodSetsEnvironmentVariable_
Diagnostic
4/4 killed Code-fix test; verifies API name and resource key "EnvironmentVariables" both appear in message; fix adds lock.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenTestMethodSetsEnvironmentVariable_
VisualBasic_
Diagnostic
3/3 killed VB variant verifies location and both message arguments for Environment.SetEnvironmentVariable.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenTestMethodSetsEnvironmentVariableInAssemblyCleanup_
NoDiagnostic
1/1 killed No-fire guards AssemblyCleanup (serialized after last test) from being flagged.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenTestMethodSetsEnvironmentVariableInAssemblyInitialize_
NoDiagnostic
1/1 killed No-fire guards AssemblyInitialize (serialized before any test) from being flagged.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenTestMethodSetsEnvironmentVariableInClassInitialize_
Diagnostic
4/4 killed Code-fix test; verifies ClassInitialize fires (unlike AssemblyInitialize); fix lifts lock to class level.
A (90–100) new UndeclaredProcessGlobalStateMutationAnalyzerTests.
WhenTestMethodSetsEnvironmentVariableInTestInitialize_
Diagnostic
3/3 killed Code-fix test; verifies TestInitialize mutation fires and fix places lock on class (not method).

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 172.8 AIC · ⌖ 6.3 AIC · ⊞ 10.3K · [◷]( · )

@Evangelink
Evangelink merged commit 97b07c6 into main Jul 27, 2026
56 checks passed
@Evangelink
Evangelink deleted the dev/amauryleve/parallel-safety-analyzers branch July 27, 2026 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state/needs-review Awaiting review from the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants