Skip to content

Add edge-case tests for SharedFileSystemPathInTestAnalyzer (MSTEST0077) - #10280

Merged
Evangelink merged 2 commits into
mainfrom
dev/amauryleve/legendary-train
Jul 28, 2026
Merged

Add edge-case tests for SharedFileSystemPathInTestAnalyzer (MSTEST0077)#10280
Evangelink merged 2 commits into
mainfrom
dev/amauryleve/legendary-train

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Fixes #10272

Two edge cases of SharedFileSystemPathInTestAnalyzer (MSTEST0077) were documented in code comments but not exercised by any test. This adds coverage for both.

Changes

  • WhenTestMethodOpensFileForWriting_Diagnostic — verifies File.OpenWrite("shared.log") inside a [TestMethod] under [assembly: Parallelize(Scope = ExecutionScope.MethodLevel)] fires the diagnostic. OpenWrite is listed in IsMutatingFileSystemMethod as the only unambiguously-mutating File.Open* variant (unlike the general File.Open, which can request a read-only handle), but nothing covered it.
  • WhenTestInitializeWritesConstantPath_Diagnostic — verifies a constant-path write inside a [TestInitialize] fixture method fires the diagnostic. ParallelSafetyHelper.GetEnclosingTestMethod accepts fixture attribute types, but no fixture-method test existed for this analyzer.

No production code changed.

Validation

Test run summary: Passed! (net8.0|x64)
  total: 18 / failed: 0 / succeeded: 18 / skipped: 0

The original work was produced by the Test Improver agentic workflow on branch test-assist/mstest0077-edge-cases-08394bb30b927b0a; the commit is cherry-picked here onto current main since GitHub Actions cannot open PRs in this repo.

Copilot AI review requested due to automatic review settings July 28, 2026 07:44

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 missing MSTEST0077 analyzer coverage for two documented edge cases.

Changes:

  • Tests diagnostics for File.OpenWrite with a constant path.
  • Tests diagnostics inside [TestInitialize] methods.
Show a summary per file
File Description
test/UnitTests/MSTest.Analyzers.UnitTests/SharedFileSystemPathInTestAnalyzerTests.cs Adds both analyzer regression tests.

Review details

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

@github-actions

This comment has been minimized.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note

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

Review Summary

Verdict: All clear

This PR adds two focused edge-case tests for SharedFileSystemPathInTestAnalyzer (MSTEST0077). Both new test methods follow the established patterns in the file exactly — same structure, same assertion style, same use of VerifyCS.VerifyAnalyzerAsync with explicit .WithLocation(0).WithArguments(...).

Dimension Status
Correctness ✅ Tests exercise documented but uncovered code paths (File.OpenWrite and [TestInitialize] fixtures)
Naming ✅ Follows When<Condition>_<Expected> convention
Style ✅ Consistent with existing tests in the file
Comments ✅ Useful context explaining why these cases matter
Public API ✅ N/A — test-only change
Performance ✅ N/A
Thread Safety ✅ N/A
Backward Compat ✅ No production changes

No actionable findings.

@Evangelink
Evangelink enabled auto-merge (squash) July 28, 2026 08:07
@Evangelink Evangelink added the state/needs-review Awaiting review from the team. label Jul 28, 2026
@github-actions

This comment has been minimized.

- WhenTestMethodOpensFileForWriting_Diagnostic: verifies File.OpenWrite with a
  constant path fires the diagnostic. The analyzer comments mention OpenWrite as
  the only unambiguously-writing File.Open* variant but no test covered it.
- WhenTestInitializeWritesConstantPath_Diagnostic: verifies the analyzer fires
  inside a [TestInitialize] fixture method, which runs before each test and can
  race under method-level parallelization. Fixture-method coverage was absent.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink
Evangelink force-pushed the dev/amauryleve/legendary-train branch from be71a4b to afa855e Compare July 28, 2026 08:21
Copilot AI review requested due to automatic review settings July 28, 2026 08:21
@Evangelink

Copy link
Copy Markdown
Member Author

Regenerate the .xlf files by running: dotnet msbuild src/Analyzers/MSTest.Analyzers/MSTest.Analyzers.csproj /t:UpdateXlf

The diagnosis was right about the symptom but the fix belongs on main, not here. Only Resources.fr.xlf was flagged, and the cause was a missing UTF-8 BOM on that one file (every other xlf in the folder had one). That was already corrected on main by the OneLocBuild check-in #10268.

Running /t:UpdateXlf locally on this branch produced no diff, which confirms the content was fine — so committing regenerated .xlf files here would have been a no-op at best and duplicated localization churn at worst.

Rebased onto main instead, which picks up #10268. Local verification after the rebase: MSTest.Analyzers builds clean (0 warnings, 0 errors, xlf check passes) and SharedFileSystemPathInTestAnalyzerTests is 18/18 green on net8.0.

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: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

🔴 Build failure analysis

All 6 build legs fail with the same 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

Root cause

This PR did not cause the failure. The sole changed file is test/UnitTests/MSTest.Analyzers.UnitTests/SharedFileSystemPathInTestAnalyzerTests.cs (no resource file touched).

The build was triggered on the merge commit of this PR against the PR base 8527187 ("Change release date for version 4.3.3", 2026-07-28 08:11 UTC). At that point the xlf was the version produced by the OneLocBuild commit 866ed18 (PR #10268, 07:40 UTC), which removed the UTF-8 BOM from Resources.fr.xlf. The XliffTasks regeneration target produces files with a BOM, so the comparison detects a difference and reports the file as out-of-date.

Timeline

UTC Commit Relevant change
2026-07-27 15:58 97b07c6 (#10248) Added MSTEST0074–0077 resources to Resources.resx + regenerated all xlfs
2026-07-28 07:40 866ed18 (#10268, OneLocBuild) Updated French translations — accidentally removed the UTF-8 BOM from Resources.fr.xlf
2026-07-28 08:11 8527187 (#10278) — PR base "Change release date" (no xlf change)
2026-07-28 08:21 19c6941fbuild merge SHA Merge of this PR into base ← build ran here
2026-07-28 08:26 0b55734 (#10235, RFC 020) Merged after the build; recreated Resources.fr.xlf as a new file with BOM — unblocks main

Because RFC 020 landed after the PR was queued, the build ran on a state where the BOM-less xlf was still present.

Impact

  • main is currently clean: RFC 020 already restored the file with a correct BOM.
  • This PR is blocked only by the stale build snapshot. Rebasing or re-triggering the build against current main will resolve it.

Recommended action

Re-trigger the PR build (or rebase onto current main). No code changes are needed in this PR.

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

@Evangelink

Copy link
Copy Markdown
Member Author

Correcting my earlier reply — the BOM was a red herring, and I have the real root cause now. Do not run /t:UpdateXlf on this file: it destroys a French translation.

Why my local verification was wrong

_UpdateXlf in Microsoft.DotNet.XliffTasks.targets is an incremental target:

<Target Name="_UpdateXlf" ... Inputs="@(XlfSource);$(_XlfSourceList)" Outputs="$(_UpdateXlfLastRun)">

With a warm artifacts/obj/.../MSTest.Analyzers.xlf/UpdateXlf.lastrun marker, it skips entirely — so both my build.cmd run and my /t:UpdateXlf run reported success without ever validating anything. Deleting the marker makes it actually run, and it then rewrites Resources.fr.xlf.

Actual root cause

Exactly one trans-unit is affected: GlobalTestFixtureShouldBeValidDescription. Its <source> matches Resources.resx byte-for-byte, and its <note> is unchanged. The problem is the target.

The OneLocBuild check-in #10268 (866ed183f) made two changes to fr.xlf: it added the missing UTF-8 BOM (fine), and it replaced an empty line in the French translation with the literal token {0}:

 – le type de retour doit être « void », « Task » ou « ValueTask ».
-
+{0}
 Le type déclarant ces méthodes doit également respecter les règles suivantes :

The English source has no format placeholders at all — that position is a blank paragraph separator. The French target now claims a {0} that does not exist in the source, so XliffTasks treats the translation as invalid and regenerates the unit as untranslated (state="new", target reset to the English text). That regenerated form differs from what is committed, which is the is out-of-date with 'Resources.resx' error.

So running /t:UpdateXlf and committing the result would discard the French translation for this string and replace it with English — 15 lines of translation lost.

This is not caused by this PR

main is red on its own with the identical error:

Build Commit Result
1529241 dd34333 ✅ succeeded
1529333 9d25639 ❌ failed
1529348 866ed18 (#10268) ❌ failed
1529393 0b55734 ❌ failed

This PR is main plus one commit that touches a single test file, so it cannot influence MSTest.Analyzers' resx/xlf. Every PR branched off current main will hit this.

Suggested fix (for main, not this PR)

Restore the blank line in the fr target of GlobalTestFixtureShouldBeValidDescription — i.e. drop the spurious {0}. That keeps the translation intact and makes the placeholder set match the source. It should land on main as a localization fix rather than being folded into an unrelated test-only PR.

Worth noting this looks like a recurring OneLocBuild failure mode in this repo — there is prior art in "Restore AreEquivalent xlf entries dropped by OneLocBuild on main", so a guard against the loc pipeline emitting placeholders absent from the source may be worth considering.

@Evangelink

Copy link
Copy Markdown
Member Author

Closing the loop on my previous comment: the fix already exists as #10281 ("Restore localization filters after #10268"), which reverts exactly the {0} back to a blank line in the fr target of GlobalTestFixtureShouldBeValidDescription, along with the same class of OneLocBuild damage in 9 other resource files.

So there is nothing to do on this PR. Once #10281 lands on main, I will rebase this branch and CI should go green. I am deliberately not duplicating that fix here — this PR stays a test-only change.

Copilot AI review requested due to automatic review settings July 28, 2026 12:32

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: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test quality grade — PR #10280

GradeTestMutationNotesHow to improve
B (80–89) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestInitialize_
WritesConstantPath_
Diagnostic
3/4 killed Verifies location and argument precisely; does not cover other fixture scopes ([ClassInitialize], [AssemblyInitialize]) nor non-parallel opt-out suppression. Add a companion test asserting no diagnostic fires when [DoNotParallelize] is present on the class, and consider a test for [ClassInitialize] to confirm cross-fixture coverage.
B (80–89) new SharedFileSystemPathInTestAnalyzerTests.
WhenTestMethod_
OpensFileForWriting_
Diagnostic
3/4 killed Verifies location and argument precisely for File.OpenWrite; the documented distinction ("unlike File.Open") is not validated — a mutation that also flags File.Open would survive. Add a no-diagnostic test for File.Open(path, FileMode.Open, FileAccess.Read) to pin the boundary the comment describes and kill the surviving mutation.

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

@Evangelink

Copy link
Copy Markdown
Member Author

On the grade-tests suggestions — I checked both against the code and am not acting on either here.

Add a no-diagnostic test for File.Open(path, FileMode.Open, FileAccess.Read) to pin the boundary the comment describes and kill the surviving mutation.

This test already exists. WhenTestMethodOpensFileForReading_NoDiagnostic sits ~250 lines above the new test in the same file and asserts exactly that:

using FileStream stream = File.Open("fixture.txt", FileMode.Open, FileAccess.Read);
...
await VerifyCS.VerifyAnalyzerAsync(code);   // no diagnostic

It even carries a comment spelling out the OpenWrite vs Open distinction. A mutation that also flagged File.Open would be killed by it, so the "surviving mutation" is not real.

Add a companion test asserting no diagnostic fires when [DoNotParallelize] is present on the class, and consider a test for [ClassInitialize].

Fair in principle, but low value here. The opt-out is not implemented per-analyzer — it lives in the shared ParallelSafetyHelper.IsOptedOutOfParallelization, which is already exercised through CultureMutationUnderParallelizationAnalyzerTests, CurrentDirectoryMutationUnderParallelizationAnalyzerTests, and UndeclaredProcessGlobalStateMutationAnalyzerTests. Adding an MSTEST0077 copy would re-test the same helper via a fourth entry point.

Same for [ClassInitialize]: GetEnclosingTestMethod accepts the fixture attributes as a set, so [TestInitialize] and [ClassInitialize] take an identical path — the new test already covers that branch.

Also worth noting the grades have moved A/A → A/A → B/B across three runs on unchanged code, so I'm treating the deltas as heuristic noise rather than a signal to keep appending tests. This PR closes a specifically-scoped issue (#10272: two documented-but-untested edge cases) and is already approved; broadening it would be scope creep. Happy for the [DoNotParallelize]-per-analyzer idea to be picked up separately if someone wants the redundancy.

@Evangelink
Evangelink merged commit 03a9683 into main Jul 28, 2026
26 checks passed
@Evangelink
Evangelink deleted the dev/amauryleve/legendary-train branch July 28, 2026 13:39
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.

[test-improver] Add edge-case tests for SharedFileSystemPathInTestAnalyzer (MSTEST0077)

3 participants