Reduce dependence on CodeTaskFactory in tests

[jankratochvilcz]

This PR reduces the test suite's dependence on CodeTaskFactory by:

• Moving VerifyEnvironmentSavedBetweenCalls to use RoslynCodeTaskFactory instead
• Adding missing test coverage relative to CodeTaskFactory

Closes #248

[Copilot]

Pull request overview

This PR reduces reliance on CodeTaskFactory within the unit test suite by switching a previously FEATURE_CODETASKFACTORY-guarded test to RoslynCodeTaskFactory, and adds a new regression test covering a temp-directory edge case for Roslyn-based inline tasks.

Changes:

• Switched VerifyEnvironmentSavedBetweenCalls to use RoslynCodeTaskFactory and removed the FEATURE_CODETASKFACTORY preprocessor guard so the test runs more broadly.
• Added a new RoslynCodeTaskFactory test ensuring inline compilation succeeds when TMP/TMPDIR points to a non-existent directory.
• Updated RoslynCodeTaskFactory_Tests to use xUnit ITestOutputHelper injection for test infrastructure output capture.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Tasks.UnitTests/RoslynCodeTaskFactory_Tests.cs	Adds regression coverage for nonexistent temp directory and wires ITestOutputHelper into the test class.
src/Build.UnitTests/BackEnd/BuildManager_Tests.cs	Removes CodeTaskFactory usage by switching the environment-persistence test to RoslynCodeTaskFactory, enabling it beyond FEATURE_CODETASKFACTORY.

[github-actions]

Backwards Compatibility Vigilance — LGTM

All changes are confined to test files (*_Tests.cs). No production code, public API, targets/props, CLI switches, default properties, or warning/error emissions are modified. No ChangeWave consideration needed.

Generated by Expert Code Review (on open) for issue #13518 · ● 36.7M

[JanKrivanek]

/review

[github-actions]

✅ Expert Code Review (command) completed successfully!

Expert review of PR #13518 complete. The expert-reviewer sub-agent analyzed the PR across all 24 review dimensions and posted its findings directly to the PR. Summary: no blocking issues found; 1 moderate suggestion (also set TEMP env var for fuller Windows coverage in the new test); 2 NITs (use using declaration style, pass _output to existing TestEnvironment calls). Verdict: COMMENT (LGTM with suggestions).

[github-actions]

Clean test-only PR. Reviewed dimensions 7–12:

• String Comparison, API Surface, Target Authoring, Design, Cross-Platform: All LGTM. The $(MSBuildToolsPath)\... backslash on line 279 of BuildManager_Tests.cs is a pre-existing MSBuild-normalized pattern used across many unguarded tests (e.g., TaskHost_Tests.cs). The new test correctly uses forward slash and sets both TMP/TMPDIR for cross-platform coverage.
• Code Simplification: One nit — prefer using declaration over braced using for TestEnvironment per project conventions.

Removing the FEATURE_CODETASKFACTORY guard is safe since RoslynCodeTaskFactory works on all target frameworks (the guard was only needed for the old CodeTaskFactory which requires .NET Framework).

Generated by Expert Code Review (command) for issue #13518 · ● 26M

[github-actions]

[NIT] Naming Precision — method name doesn't follow the codebase convention

The dominant naming pattern in this file is MethodOrScenario_Scenario_ExpectedResult or a descriptive verb phrase. The nearby tests in particular show: RoslynCodeTaskFactory_ReuseCompilation, RoslynCodeTaskFactory_UsingAPI, RoslynCodeTaskFactoryWithoutCS1702Warning.

RoslynCodeTaskFactoryTempDirectoryDoesntExist describes the setup (the temp directory doesn't exist) but doesn't convey the expected outcome (compilation succeeds / build succeeds). A reader scanning test results can't tell from the name alone whether this is a positive or negative test.

Recommendation: Rename to make the scenario and expected outcome explicit, e.g.:

public void RoslynCodeTaskFactory_NonexistentTempDirectory_BuildSucceeds()

[github-actions]

[MODERATE] Test Coverage & Completeness

The test sets TMP/TMPDIR to a nonexistent path but never verifies that Path.GetTempPath() actually reflects that path before the build runs. The analogous CodeTaskFactory test (CodeTaskFactoryTests.BuildTaskSimpleCodeFactoryTempDirectoryDoesntExist) includes two guard assertions for exactly this reason (lines 1208–1217 in that file):

// 1. Pre: confirm baseline — TMP == GetTempPath() before the override
Assert.Equal(
    FileUtilities.EnsureTrailingSlash(Path.GetTempPath()),
    FileUtilities.EnsureTrailingSlash(realTempPath));

env.SetEnvironmentVariable("TMP", newTempPath);
env.SetEnvironmentVariable("TMPDIR", newTempPath);

// 2. Post: confirm the override is in effect BEFORE the build executes
Assert.Equal(
    FileUtilities.EnsureTrailingSlash(newTempPath),
    FileUtilities.EnsureTrailingSlash(Path.GetTempPath()));

MockLogger mockLogger = Helpers.BuildProjectWithNewOMExpectSuccess(projectFileContents);

Why this matters: On a Windows CI machine where TEMP is set but TMP is absent (or vice-versa), GetTempPathW falls back through TMP → TEMP → USERPROFILE → Windows dir. If TMP is currently unset, writing to it has no effect on Path.GetTempPath(), the real temp directory is used throughout, and the test passes trivially — without ever exercising the nonexistent-directory code path it was written to protect.

Recommendation: Mirror the pre/post assertions from BuildTaskSimpleCodeFactoryTempDirectoryDoesntExist. At a minimum, add the postcondition check between the env.SetEnvironmentVariable calls and the Helpers.BuildProjectWithNewOMExpectSuccess call so the test self-validates that the override is active.

[jankratochvilcz]

Makes sense, please bring this to parity @copilot

[github-actions]

Test Coverage gap — the test may silently pass for the wrong reason on some CI machines.

The analogous CodeTaskFactoryTests.BuildTaskSimpleCodeFactoryTempDirectoryDoesntExist (line 1207-1217) includes two guard assertions that are missing here:

1. Before setting the env vars, it verifies Path.GetTempPath() actually matches the old TMP value (proving TMP is the variable the runtime reads).
2. After setting TMP to the new path, it verifies Path.GetTempPath() now returns newTempPath.

Without assertion (2), if a CI machine's runtime resolves the temp path from a different variable (e.g., TEMP on Windows takes precedence over TMP, or the .NET runtime caches GetTempPath()), then Path.GetTempPath() still returns a valid, existing directory and the build succeeds trivially — the nonexistent-temp-dir scenario is never actually exercised.

Consider adding after line 1558:

// Verify the runtime actually sees the new temp path.
FileUtilities.EnsureTrailingSlash(Path.GetTempPath())
    .ShouldBe(FileUtilities.EnsureTrailingSlash(newTempPath));

Also: setting TMPDIR on Linux is good, but you may also want to set TEMP on Windows (which takes precedence over TMP in GetTempPathW) to ensure full coverage across platforms.

[jankratochvilcz]

Please do this @copilot