Fix Helix SDK test isolation: current-directory race in XHarness tests

[Copilot]

CreateXHarnessAppleWorkItemsTests.ArchivePayloadIsOverwritten fails intermittently: LocalTestResultsReaderTests mutates Environment.CurrentDirectory while XHarness tests resolve relative paths through TaskEnvironment.Fallback, which reads the process current directory on every lookup. When the interleaving is unlucky, PrepareWorkItem resolves apps/System.Foo.app to a path the MockFileSystem never registered, the task logs an error, and InvokeExecute returns false.

Test-only change. Parallel execution is preserved — no nonparallel collection, sleeps, retries, or production behavior changes.

Changes

• Dropped the global mutation (LocalTestResultsReaderTests): the test already passes a fully qualified path to the reader, so the set/restore of Environment.CurrentDirectory was pure global-state churn.

• Explicit task environment (CreateXHarnessApple/AndroidWorkItemsTests): each test instance gets its own project directory, so setup registration and production resolution anchor to the same place regardless of interleaving.

  _task = new CreateXHarnessAppleWorkItems()
  {
      BuildEngine = _buildEngine,
      TaskEnvironment = TaskEnvironment.CreateWithProjectDirectoryAndEnvironment(
          Path.Combine(Path.GetTempPath(), "xharness-apple-tests", Guid.NewGuid().ToString("N"))),
  };

• Failure diagnostics: added MockBuildEngine.ErrorSummary and threaded it through the success assertions, so an unexpected task failure reports the logged error instead of only Expected boolean to be True, but found False.

• Regression coverage: RelativeAppBundlePathsResolveAgainstTheProjectDirectory / RelativeApkPathsResolveAgainstTheProjectDirectory assert relative payload paths land under TaskEnvironment.ProjectDirectory and reach the archive manager and PayloadArchive metadata unchanged.

To double check:

• The right tests are in and the right validation has happened. Guidance: https://github.com/dotnet/arcade/blob/main/Documentation/Validation.md

• Fixes Fix Helix SDK test isolation: current-directory race in ArchivePayloadIsOverwritten #17570

[Copilot]

Copilot wasn't able to review any files in this pull request.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

No unresolved issues were identified that would block approval.

Review details

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

[Copilot]

🔵 Needs a closer look

Two moderate filesystem-semantics issues remain unresolved.

Review details

Suppressed comments (5)

src/Common/Microsoft.Arcade.Test.Common/MockFileSystem.cs:70

• When an existing file is opened with FileMode.Create, this now constructs MockFileStream from the old contents instead of truncating it. A caller that writes a shorter replacement from position 0 leaves stale trailing bytes in Files (for example, ProvisioningProfileProvider uses FileMode.Create), so the mock no longer matches real FileStream semantics. Preserve create/truncate behavior when initializing the writable stream.

        => access == FileAccess.Read && FileExists(path)
            ? new MemoryStream(Encoding.UTF8.GetBytes(Files[path]))
            : new MockFileStream(this, path);

src/Microsoft.DotNet.Helix/JobMonitor/TestResults/LocalTestResultsReader.cs:306

• File.OpenRead used here before this change opens the file with FileShare.Read, but FileSystem.GetFileStream delegates to new FileStream(path, mode, access), whose default is FileShare.None. That makes the production reader acquire an exclusive read handle, so a second reader of the same result file can now fail with an IOException; preserve the previous sharing semantics in the filesystem abstraction (for example, use FileShare.Read for read-only opens).

        => XmlReader.Create(_fileSystem.GetFileStream(filePath, FileMode.Open, FileAccess.Read), new XmlReaderSettings

src/Microsoft.DotNet.Helix/JobMonitor/TestResults/LocalTestResultsReader.cs:26

• All reader tests now construct the new three-argument overload with MockFileSystem, so the default constructor used by TestResultProcessor (which creates the real FileSystem) is no longer exercised. A regression in the default IFileSystem wiring or actual FileStream behavior could therefore pass this suite; retain at least one test that uses the normal constructor with a real temporary result file.

        : this(logger, attachmentMode, new FileSystem())

src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/CreateXHarnessAndroidWorkItemsTests.cs:149

• This only checks that PayloadArchive starts with the project directory. It would pass if the task emitted a different archive filename or otherwise changed the metadata while staying under that directory, so it does not verify the stated unchanged payload metadata contract. Assert the exact archive path derived from the relative APK.

        _task.WorkItems.Single().GetMetadata("PayloadArchive").Should().StartWith(projectDirectory);

src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/CreateXHarnessAppleWorkItemsTests.cs:174

• This only checks that PayloadArchive starts with the project directory. It would pass if the task emitted a different archive filename or otherwise changed the metadata while staying under that directory, so it does not verify the stated unchanged payload metadata contract. Assert the exact archive path derived from the relative app bundle.

        _task.WorkItems.Single().GetMetadata("PayloadArchive").Should().StartWith(projectDirectory);

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite