Skip to content

Convert ExitCodes from static class to enum#7876

Open
Evangelink wants to merge 1 commit intomainfrom
dev/amauryleve/convert-exitcodes-to-enum
Open

Convert ExitCodes from static class to enum#7876
Evangelink wants to merge 1 commit intomainfrom
dev/amauryleve/convert-exitcodes-to-enum

Conversation

@Evangelink
Copy link
Copy Markdown
Member

Summary

Converts the internal ExitCodes static class with public const int fields to an internal enum. This improves type safety, discoverability, and enables Enum.IsDefined checks for validating known MTP exit codes.

Breaking change analysis

No breaking change, because:

  1. const int values are inlined at compile time — extensions compiled against the old static class ExitCodes have the literal integer values baked into their IL. The consumer assembly has zero runtime reference to the ExitCodes type. Verified by disassembling a Consumer.dll — the string "ExitCodes" does not appear.

  2. Runtime verified — built an old consumer against static class ExitCodes, then swapped the library to enum ExitCodes without recompiling the consumer. It runs correctly.

  3. Out-of-repo IVT consumers don't use ExitCodes — checked MSTest.Engine, OpenTelemetry, AzureFoundry, Platform.AI, HotReload, CrashDump, Telemetry, VSTestBridge — none reference ExitCodes.

  4. Source-embedded consumers always recompile — extensions that DO use ExitCodes (Retry, TrxReport, HangDump, MSBuild) embed ExitCodes.cs via <Compile Include> and compile their own internal copy.

  5. Not in any PublicAPI.txtExitCodes is internal, so no public API surface change.

Changes

  • Converted ExitCodes from internal static class to internal enum (same numeric values)
  • Added (int) casts at all usage sites in source code
  • Added ExitCodes enum overloads to AcceptanceAssert.AssertExitCodeIs/AssertExitCodeIsNot for both TestHostResult and DotnetMuxerResult
  • Added explicit using Microsoft.Testing.Platform.Helpers to AcceptanceAssert.cs (shared across projects with different global usings)

Fixes #7807

Converts the internal ExitCodes static class with const int fields to an
internal enum. This improves type safety, discoverability, and enables
Enum.IsDefined checks for validating known MTP exit codes.

This is safe because:
- ExitCodes is internal and not in any PublicAPI.txt
- const int values are inlined at compile time, so extensions compiled
  against the old class have zero runtime dependency on the ExitCodes type
- All out-of-repo IVT consumers (MSTest.Engine, OpenTelemetry, etc.) do
  not reference ExitCodes at all
- Source-embedded consumers always recompile from the same source

Fixes #7807
Copilot AI review requested due to automatic review settings April 27, 2026 17:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Microsoft.Testing.Platform’s internal exit-code representation by converting ExitCodes from an internal static class of const int fields to an internal enum, and then updates call sites (product code + tests) to use the enum while still returning/consuming int where required (process exit codes, assertions, etc.).

Changes:

  • Converted ExitCodes from internal static class constants to an internal enum with the same numeric values.
  • Updated product code call sites to cast enum values to int where APIs require integers (e.g., return values and Environment.Exit).
  • Updated unit/integration tests and added enum-friendly overloads to AcceptanceAssert to reduce casting noise in tests.
Show a summary per file
File Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Services/TestApplicationResultTests.cs Updates assertions to compare against (int)ExitCodes.*.
test/UnitTests/Microsoft.Testing.Platform.UnitTests/ServerMode/ServerTests.cs Updates expected exit code assertion to (int)ExitCodes.TestSessionAborted.
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MSBuildTests.GenerateEntryPoint.cs Updates success exit code assertion to (int)ExitCodes.Success.
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/Helpers/AcceptanceAssert.cs Adds ExitCodes overloads for exit-code assertions and adds explicit using for shared compilation contexts.
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/ExecutionRequestCompleteTests.cs Updates exit code assertion to (int)ExitCodes.Success.
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/ConsoleTests.cs Updates exit code assertion to (int)ExitCodes.Success.
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/SdkTests.cs Updates invalid-command-line exit code assertions to (int)ExitCodes.InvalidCommandLine.
src/Platform/Microsoft.Testing.Platform/Services/TestApplicationResult.cs Updates exit-code computation to cast enum values to int.
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeClient.cs Casts ExitCodes.GenericFailure to int for _environment.Exit(...).
src/Platform/Microsoft.Testing.Platform/Hosts/ToolsTestHost.cs Casts invalid-command-line exit codes to int for return values.
src/Platform/Microsoft.Testing.Platform/Hosts/TestHostOchestratorHost.cs Casts aborted-session exit code to int.
src/Platform/Microsoft.Testing.Platform/Hosts/TestHostControllersTestHost.cs Casts multiple exit code assignments/comparisons to int.
src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.cs Casts invalid-command-line exit code passed into InformativeCommandLineHost.
src/Platform/Microsoft.Testing.Platform/Hosts/ServerTestHost.cs Casts Success/TestSessionAborted to int in conditional return.
src/Platform/Microsoft.Testing.Platform/Hosts/ConsoleTestHost.cs Casts aborted-session exit code to int and updates telemetry payload expression.
src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs Casts default/fallback exit codes to int.
src/Platform/Microsoft.Testing.Platform/Helpers/NonCooperativeParentProcessListener.cs Casts dependent-process exit code to int for _environment.Exit(...).
src/Platform/Microsoft.Testing.Platform/Helpers/ExitCodes.cs Core change: converts ExitCodes to an internal enum with the same numeric values.
src/Platform/Microsoft.Testing.Platform.MSBuild/Tasks/InvokeTestingPlatformTask.cs Casts comparisons against invalid-command-line exit code to int.
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs Casts comparisons against ExitCodes.Success to int.
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxCompareTool.cs Casts success/failure returns to int.
src/Platform/Microsoft.Testing.Extensions.Retry/RetryOrchestrator.cs Casts success/failure comparisons/returns to int.

Copilot's findings

  • Files reviewed: 22/22 changed files
  • Comments generated: 2

{ TelemetryProperties.RequestProperties.RequestExecuteStart, requestExecuteStart },
{ TelemetryProperties.RequestProperties.RequestExecuteStop, requestExecuteStop },
{ TelemetryProperties.HostProperties.ExitCodePropertyName, cancellationToken.IsCancellationRequested ? ExitCodes.TestSessionAborted : exitCode.ToString(CultureInfo.InvariantCulture) },
{ TelemetryProperties.HostProperties.ExitCodePropertyName, cancellationToken.IsCancellationRequested ? (int)ExitCodes.TestSessionAborted : exitCode.ToString(CultureInfo.InvariantCulture) },
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

Telemetry metric ExitCodePropertyName is currently emitted as an int when cancellation is requested, but as a string otherwise. This creates inconsistent telemetry schema (and differs from other hosts which always emit exitCode.ToString(...)). Consider always emitting a string (e.g., just use exitCode.ToString(CultureInfo.InvariantCulture) since exitCode is already set to the final value in the catch).

Suggested change
{ TelemetryProperties.HostProperties.ExitCodePropertyName, cancellationToken.IsCancellationRequested ? (int)ExitCodes.TestSessionAborted : exitCode.ToString(CultureInfo.InvariantCulture) },
{ TelemetryProperties.HostProperties.ExitCodePropertyName, exitCode.ToString(CultureInfo.InvariantCulture) },

Copilot uses AI. Check for mistakes.
Comment on lines +132 to +137
int exitCode = (int)ExitCodes.Success;
exitCode = exitCode == (int)ExitCodes.Success && _policiesService.IsMaxFailedTestsTriggered ? (int)ExitCodes.TestExecutionStoppedForMaxFailedTests : exitCode;
exitCode = exitCode == (int)ExitCodes.Success && _testAdapterTestSessionFailure ? (int)ExitCodes.TestAdapterTestSessionFailure : exitCode;
exitCode = exitCode == (int)ExitCodes.Success && _failedTestsCount > 0 ? (int)ExitCodes.AtLeastOneTestFailed : exitCode;
exitCode = exitCode == (int)ExitCodes.Success && _policiesService.IsAbortTriggered ? (int)ExitCodes.TestSessionAborted : exitCode;
exitCode = exitCode == (int)ExitCodes.Success && _totalRanTests == 0 ? (int)ExitCodes.ZeroTests : exitCode;
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

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

GetProcessExitCode() now has a lot of repeated (int)ExitCodes.Success casts and comparisons, which makes the exit-code decision logic harder to read and maintain. Consider keeping the working variable typed as ExitCodes (and comparing against ExitCodes.Success), then cast once at the end when returning the int process exit code.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-27
Repository: microsoft/testfx

Key Findings

The test changes in this PR are entirely mechanical — converting ExitCodes.xxx to (int)ExitCodes.xxx (or Assert.AreEqual((int)ExitCodes.xxx, ...)) as required by the const intenum conversion. All changes are correct and maintain the same test semantics.

One minor observation:

This PR adds typed AssertExitCodeIs(ExitCodes) / AssertExitCodeIsNot(ExitCodes) overloads on both TestHostResult and DotnetMuxerResult in AcceptanceAssert.cs. These overloads are superior to raw Assert.AreEqual because they invoke GenerateFailedAssertionMessage, which includes the full test host stdout/stderr in the failure output — invaluable for diagnosing integration test failures. However, three call sites on TestHostResult were not migrated to the new API:

  • test/IntegrationTests/MSTest.Acceptance.IntegrationTests/SdkTests.cs — lines 229, 286
  • test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/ExecutionRequestCompleteTests.cs — line 19

(The other changed sites — ConsoleTests.cs and ServerTests.cs — compare against raw int variables where the extension methods don't apply, so they're fine as-is.)

Recommendations

Update the three sites above to use testHostResult.AssertExitCodeIs(ExitCodes.xxx) for richer failure diagnostics in CI.

No isolation, flakiness, data-coverage, or false-positive/negative issues found.


Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪


testHostResult = await testHost.ExecuteAsync(command: invalidCommandLineArg, cancellationToken: TestContext.CancellationToken);
Assert.AreEqual(ExitCodes.InvalidCommandLine, testHostResult.ExitCode);
Assert.AreEqual((int)ExitCodes.InvalidCommandLine, testHostResult.ExitCode);
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.

[Assertion] This PR adds a typed AssertExitCodeIs(ExitCodes exitCode) overload on TestHostResult in AcceptanceAssert.cs that produces a richer failure message (via GenerateFailedAssertionMessage, which includes the full test host standard output/error). However, this site (and line 286 below, plus ExecutionRequestCompleteTests.cs line 19) still uses a raw Assert.AreEqual((int)ExitCodes.xxx, testHostResult.ExitCode), which on failure only reports the integer values with no host output context.

Impact: Harder to diagnose CI failures — the assertion failure message won't include the test host output that shows why the exit code was wrong.

Suggestion: Replace with the typed overload for a more informative failure message:

// instead of:
Assert.AreEqual((int)ExitCodes.InvalidCommandLine, testHostResult.ExitCode);
// use:
testHostResult.AssertExitCodeIs(ExitCodes.InvalidCommandLine);

/// We use positive exit codes for failure because POSIX/BASH exit codes are unsigned 8-bit integers.
/// On POSIX systems the standard exit code is 0 for success and any number from 1 to 255 for anything else.
/// </summary>
// TODO: Consider changing this to an enum, and rename to 'ExitCode' to follow enum naming convention.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Renaming wasn't addressed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Convert ExitCodes constants class to an enum

3 participants