Fix flaky DistributedFileLoggerParameters test with test isolation

[jankratochvilcz]

Problem

DistributedFileLoggerParameters intermittently fails in CI with:

LoggerException: Failed to write to log file "msbuild0.log". The process cannot access the file because it is being used by another process.

Both DistributedFileLoggerParameters and DistributedLoggerNullEmpty create msbuild0.log in the shared working directory. When tests run concurrently they contend on the same file. The old finally block also never cleaned up msbuild0.log itself.

Fix

• Use TestEnvironment with an isolated temp folder and SetCurrentDirectory so each test operates on its own files
• TestEnvironment.Dispose() handles all cleanup automatically, replacing the brittle manual try/finally
• Added ITestOutputHelper to the test class for proper test output capture

[Copilot]

Pull request overview

Addresses CI flakiness in FileLogger_Tests caused by multiple distributed-logger tests contending on the same log file in a shared working directory.

Changes:

• Added ITestOutputHelper injection to the test class for captured diagnostic output.
• Updated distributed-logger tests to use TestEnvironment + an isolated temp folder, and set the current directory per test to avoid log-file collisions.
• Removed brittle manual cleanup logic in favor of TestEnvironment-managed cleanup.

[Copilot]

In this modified test code, assertions are still using xUnit Assert.*. This repo's Build.UnitTests generally uses Shouldly for assertions; please convert the updated assertions in this method (e.g., around this line and the similar comparisons below) to Shouldly and add the corresponding using Shouldly; if needed.

[github-actions]

Review Summary

Overall: Good fix ✅ — the flakiness root cause is correctly diagnosed and addressed.

What's Good

• ✅ TestEnvironment + TransientTestFolder + SetCurrentDirectory is the idiomatic MSBuild test isolation pattern
• ✅ using declaration ensures deterministic cleanup even if the test throws
• ✅ Removes the brittle manual try/finally cleanup that missed msbuild0.log
• ✅ Both DistributedFileLoggerParameters and DistributedLoggerNullEmpty are isolated
• ✅ ITestOutputHelper properly injected for diagnostic output in CI

Suggestion (🟡 Important)

• The assertions use the Assert.Equal(0, string.Compare(...)) pattern which should be replaced with Shouldly (ShouldBe(..., StringCompareShould.IgnoreCase)) per repo testing conventions for modified code. This also provides better failure diagnostics — the current pattern only reports "Expected 0, got 1" on failure, hiding what the actual string difference was.

Generated by Expert Code Review (on open) for issue #13669 · ● 4.6M

[github-actions]

🟡 Convention (Important): Per repo testing conventions, modified code should use Shouldly assertions even when the rest of the file uses xUnit. The Assert.Equal(0, string.Compare(..., StringComparison.OrdinalIgnoreCase)) pattern is verbose and produces poor failure messages. Consider replacing with:

fileLogger.InternalFilelogger.Parameters.ShouldBe(
    "ForceNoAlign;ShowEventId;ShowCommandLine;logfile=msbuild0.log;",
    StringCompareShould.IgnoreCase);

This applies to all four assertions in this method. You'd need to add using Shouldly; at the top of the file.

The Shouldly version is more readable and on failure will show the actual vs expected value, rather than just "Expected: 0, Actual: 1" which makes debugging much harder.

[github-actions]

🟡 Assertion style: replace Assert.Equal(0, string.Compare(...)) with Shouldly

Per repo testing conventions, Shouldly assertions should be used in modified code. The current pattern is verbose and produces poor failure diagnostics. Replace all occurrences in this method with:

fileLogger.InternalFilelogger.Parameters.ShouldBe(
    "ForceNoAlign;ShowEventId;ShowCommandLine;logfile=msbuild0.log;",
    StringCompareShould.IgnoreCase);

This applies to all 4 assertion blocks in DistributedFileLoggerParameters.

[jankratochvilcz]

Addressed the review feedback in a9e6760:

• ✅ Converted all four Assert.Equal(0, string.Compare(..., StringComparison.OrdinalIgnoreCase)) blocks to str.ShouldBe(expected, StringCompareShould.IgnoreCase) per repo testing conventions.
• ✅ Added using Shouldly;.
• 🔵 The note about a missing using Xunit.Abstractions; does not apply — this project is on xUnit v3, where ITestOutputHelper lives in the Xunit namespace (already imported). The file built and the tests passed locally.

Verified locally: DistributedFileLoggerParameters and DistributedLoggerNullEmpty both pass.

@copilot please re-review.

[Copilot]

Addressed the review feedback in a9e6760:

• ✅ Converted all four Assert.Equal(0, string.Compare(..., StringComparison.OrdinalIgnoreCase)) blocks to `str.ShouldBe(expected, StringCompareShould.Ign...

Re-reviewed and verified. Commit a9e6760 already addresses the feedback correctly (Shouldly assertions are in place, ITestOutputHelper resolves via Xunit in this test project), and both affected tests pass locally with targeted execution: DistributedFileLoggerParameters and DistributedLoggerNullEmpty.