Consolidate CI detection into core platform helper#8394
Conversation
Move CIEnvironmentDetectorForTelemetry from the Telemetry extension into Microsoft.Testing.Platform as CIEnvironmentDetector (instance-based with IEnvironment for testability, marked [Embedded] so the Telemetry extension source-links its own copy instead of relying on InternalsVisibleTo). Use the consolidated detector in TerminalOutputDevice instead of the inline TF_BUILD || GITHUB_ACTIONS check, closing the long-standing TODO that asked to reuse the telemetry CI detection (#5533). The terminal now switches to its CI-friendly SimpleAnsi mode for the broader set of CI providers detected by the standard .NET CI-detection list. Use --no-ansi if you prefer plain output. Also reword SimpleAnsiTerminal documentation to be CI-generic rather than AzDO-specific. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| bool noAnsi = _commandLineOptions.IsOptionSet(TerminalTestReporterCommandLineOptionsProvider.NoAnsiOption); | ||
|
|
||
| // TODO: Replace this with proper CI detection that we already have in telemetry. https://github.com/microsoft/testfx/issues/5533#issuecomment-2838893327 | ||
| bool inCI = string.Equals(_environment.GetEnvironmentVariable("TF_BUILD"), "true", StringComparison.OrdinalIgnoreCase) || string.Equals(_environment.GetEnvironmentVariable("GITHUB_ACTIONS"), "true", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Are you sure all other CI systems have support for simple ANSI? This can easily be a regression if other systems don't support that.
There was a problem hiding this comment.
Based on AI lookup there are few uncertainties but it seemed overall easier than just picking some and waiting.
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Review Summary — PR #8394: Consolidate CI detection into core platform helper
Overall this is a clean, well-scoped change that properly closes the long-standing TODO. The move of CIEnvironmentDetectorForTelemetry into Microsoft.Testing.Platform as an [Embedded] helper follows the established pattern for RoslynString and SingleConsumerUnboundedChannel, and the switch from LINQ .All(...) to an explicit loop is a minor-but-correct improvement (avoids a lambda allocation per outer loop iteration under netstandard).
Dimension verdict table
| # | Dimension | Status |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ Logic is equivalent to the original; bool.TryParse handles AzDO ("True") and GitHub Actions ("true") correctly |
| 2 | Threading & Concurrency | ✅ static readonly arrays are immutable; Instance is a lazily-constructed static field — safe |
| 3 | Security & IPC Contract Safety | ✅ Only reads env-vars; no new surface |
| 4 | Public API Surface | ✅ Class is internal sealed; no changes to PublicAPI.Unshipped.txt needed |
| 5 | Backward Compatibility | ✅ Behavioural change in TerminalOutputDevice is net-wider (more CI systems detected), not narrower |
| 6 | Performance | ✅ Called once during init in both callers; no hot-path impact |
| 7 | Cross-TFM Correctness | ✅ [Embedded] pattern compiles correctly across all TFMs |
| 8 | Localization | ✅ No user-facing strings added |
| 9 | Naming & Conventions | ✅ Consistent with existing CIEnvironmentDetector in TestFramework |
| 10 | Code Style | [ExcludeFromCodeCoverage] missing (see inline comment) |
| 11 | Documentation | ✅ XML-docs added; SimpleAnsiTerminal comment generalised appropriately |
| 12 | Test Coverage | i️ No new tests added; the refactored logic is functionally identical to the existing telemetry code, so existing tests cover it indirectly |
| 13 | Error Handling | ✅ N/A |
| 14 | Dependency Management | ✅ Source-linked via <Compile Include=...> instead of InternalsVisibleTo, consistent with pattern |
| 15 | Scope Discipline | ✅ Single coherent concern |
| 16 | Dead Code | Instance property unused by current callers (see inline comment) |
| 17 | Resource Management | ✅ N/A |
| 18 | IPC Contract | ✅ N/A |
| 19 | Telemetry Correctness | ✅ Behaviour preserved in AppInsightsProvider |
| 20 | init Accessor Rule |
✅ No init accessors |
| 21 | Analyzer / Roslyn Rules | ✅ [Embedded] correctly sourced from Microsoft.CodeAnalysis |
Two minor findings
-
[ExcludeFromCodeCoverage]missing —RoslynString.csand other embedded helpers carry this attribute. Without it the source-linked copy inMicrosoft.Testing.Extensions.Telemetrywill be included in coverage reports for that project. -
Instanceis unused — Both current callers pass their own injectedIEnvironment. IfInstanceis kept for future convenience callers, its intended audience should be documented; otherwise removing it reduces noise.
Neither finding blocks merging, but both are worth addressing before the PR lands.
Generated by Expert Code Review (on open) for issue #8394 · ● 5.9M
- Add [ExcludeFromCodeCoverage] to CIEnvironmentDetector to match the established pattern for embedded helpers (e.g., RoslynString). Without it, the source-linked copy in Microsoft.Testing.Extensions.Telemetry would be included in coverage measurements for that project. - Remove the unused static Instance property. Both current callers (TerminalOutputDevice and AppInsightsProvider) construct the detector directly with their injected IEnvironment, so the static field was dead code that also allocated a SystemEnvironment unconditionally at class initialization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/Platform/Microsoft.Testing.Platform/Helpers/CIEnvironmentDetector.cs:88
RoslynStringis referenced without a namespace import, butRoslynStringis declared innamespace Microsoft.Testing.Platform(notMicrosoft.Testing.Platform.Helpers). As written, this file won’t compile unless an unrelated global using exists. Addusing Microsoft.Testing.Platform;or fully-qualify the type (Microsoft.Testing.Platform.RoslynString).
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| foreach (string variable in variables) | ||
| { | ||
| if (RoslynString.IsNullOrEmpty(_environment.GetEnvironmentVariable(variable))) | ||
| { | ||
| allVariablesPresent = false; | ||
| break; | ||
| } | ||
| } |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the RoslynString lookup concern by fully qualifying the helper in \CIEnvironmentDetector\. I also reran \.\build.cmd\, and the repository build completed cleanly with 0 warnings and 0 errors. |
Summary
Closes a long-standing TODO (#5533) that asked the terminal output device to reuse the proper CI-detection list that telemetry already maintains, instead of the hand-coded
TF_BUILD || GITHUB_ACTIONScheck.CIEnvironmentDetectorForTelemetryfromMicrosoft.Testing.Extensions.TelemetryintoMicrosoft.Testing.PlatformasCIEnvironmentDetector(instance-based withIEnvironmentfor testability, mirroring the existingTestFrameworkcopy).[Embedded]and has the Telemetry extension source-link it via<Compile Include>, instead of relying on InternalsVisibleTo. Same pattern already in place forRoslynStringandSingleConsumerUnboundedChannelin that project.TerminalOutputDevicenow uses the consolidated detector.SimpleAnsiTerminaldoc comments to be CI-generic (still mentions AzDO as the example of a log viewer that resets color per line).Behavior change
The terminal now switches to its CI-friendly
SimpleAnsimode (colored output, no cursor moves) for the broader set of CI providers detected by the standard .NET CI-detection list — i.e., not justTF_BUILD/GITHUB_ACTIONSbut also AppVeyor, Travis, CircleCI, AWS CodeBuild, Jenkins, Google Cloud Build, TeamCity, JetBrains Space, and genericCI=true.Matches the spirit of
dotnet build --tldefaulting to off in CI, and most affected CIs render ANSI codes fine but don't support cursor moves anyway. Users who prefer plain output on a CI we now light up can pass--no-ansi.Validation
dotnet build src/Platform/Microsoft.Testing.Platform/Microsoft.Testing.Platform.csproj— succeedsdotnet build src/Platform/Microsoft.Testing.Extensions.Telemetry/Microsoft.Testing.Extensions.Telemetry.csproj— succeedsMicrosoft.Testing.Platform.UnitTests— 820 passed, 2 pre-existing skipsMicrosoft.Testing.Extensions.UnitTests— 266 passed, 2 pre-existing skips