Modernize ToolTask canonical error format test

[rainersigwald]

Bring ToolTaskCanChangeCanonicalErrorFormat up to current test conventions by using TestEnvironment and MockEngine(_output), and replace piecemeal error checks with grouped Shouldly assertions that produce clearer failures.

This test failed with no helpful error in #13480.

[Copilot]

Pull request overview

Updates ToolTaskCanChangeCanonicalErrorFormat to align with current unit test conventions in the MSBuild repo, improving reliability and diagnosability when the test fails.

Changes:

• Switches temp file setup/cleanup to TestEnvironment.Create(_output) and env.CreateFile(...).
• Uses MockEngine(_output) for structured event capture and output visibility in test logs.
• Replaces multiple piecemeal log substring checks with grouped Shouldly assertions over warning/error event fields.

[github-actions]

One simplification issue: engine.ShouldSatisfyAllConditions(...) groups a task.ExitCode assertion under the engine receiver, producing misleading failure messages. Split into three bare assertions. Everything else in the refactoring is clean.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• #13546 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #13546 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by Expert Code Review (on open) for issue #13546 · ● 12.7M

[github-actions]

Code Simplification — Minor issue

engine.ShouldSatisfyAllConditions(
    () => task.ExitCode.ShouldBe(-1),   // ← subject is task, not engine
    () => engine.Warnings.ShouldBe(1),
    () => engine.Errors.ShouldBe(1));

The receiver engine is misleading: one of the three assertions is about task.ExitCode, not the engine. When this assertion fails, Shouldly will report "engine should satisfy all conditions" while the failing lambda is actually checking the task — a confusing mismatch.

These three independent scalar assertions gain nothing from being grouped. The simpler and clearer form is:

task.ExitCode.ShouldBe(-1);
engine.Warnings.ShouldBe(1);
engine.Errors.ShouldBe(1);

Each failure message then names the correct subject. The rest of the refactoring (TestEnvironment, using-declaration, typed event assertions) is clean.

[github-actions]

NIT (Code Simplification): The receiver of ShouldSatisfyAllConditions is engine, but the first lambda asserts on task.ExitCode — a property of a different object. This is functionally correct (the extension method just runs all lambdas), but a reader scanning engine.ShouldSatisfyAllConditions(...) would expect all assertions to be about engine.

Consider either:

• Asserting task.ExitCode separately: task.ExitCode.ShouldBe(-1);
• Or grouping under a neutral receiver

Suggested change

	engine.ShouldSatisfyAllConditions(
	() => task.ExitCode.ShouldBe(-1),
	() => engine.Warnings.ShouldBe(1),
	task.ExitCode.ShouldBe(-1);
	engine.ShouldSatisfyAllConditions(
	() => engine.Warnings.ShouldBe(1),
	() => engine.Errors.ShouldBe(1));