Deduplicate CIEnvironmentDetector across TestFramework and Platform#9008
Deduplicate CIEnvironmentDetector across TestFramework and Platform#9008Evangelink wants to merge 1 commit into
Conversation
Fixes #8985. The CIEnvironmentDetector class was duplicated nearly verbatim in src/TestFramework/TestFramework/Internal/ and src/Platform/Microsoft.Testing.Platform/Helpers/, with the same arrays of CI environment variable names and the same IsCIEnvironment() logic in each copy. The TestFramework copy was added later than the Platform copy and the two had already begun to drift (XML doc shape, attribute usage). Make the Platform copy the single source of truth and link it into MSTest.TestFramework (the same pattern already used for RoslynHashCode and as Microsoft.Testing.Extensions.Telemetry already does for this file). The TestFramework project sets a TESTFRAMEWORK_CI_DETECTOR preprocessor symbol which toggles the small, real differences via #if guards: namespace, [Embedded]/[ExcludeFromCodeCoverage] attributes, public vs. internal ctor, the static Instance helper, and RoslynString.IsNullOrEmpty vs. string.IsNullOrEmpty. Also update eng/vendored-files.json to drop the now-removed testframework-ci-environment-detector entry and document the new linked-file relationship on the platform entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR removes a duplicated CIEnvironmentDetector implementation by making src/Platform/Microsoft.Testing.Platform/Helpers/CIEnvironmentDetector.cs the canonical source and linking it into MSTest.TestFramework, using #if TESTFRAMEWORK_CI_DETECTOR to toggle the intentional TestFramework-vs-Platform differences.
Changes:
- Linked the Platform
CIEnvironmentDetector.csintoMSTest.TestFrameworkand added theTESTFRAMEWORK_CI_DETECTORcompilation constant. - Removed the in-tree duplicated
src/TestFramework/TestFramework/Internal/CIEnvironmentDetector.cs. - Updated the canonical Platform
CIEnvironmentDetectorto support both variants via conditional compilation. - Updated
eng/vendored-files.jsonto reflect the dedup/linking relationship (but see comment about the define name).
Show a summary per file
| File | Description |
|---|---|
| src/TestFramework/TestFramework/TestFramework.csproj | Links the canonical detector file and defines TESTFRAMEWORK_CI_DETECTOR for the TestFramework-specific variant. |
| src/TestFramework/TestFramework/Internal/CIEnvironmentDetector.cs | Removes the duplicated TestFramework copy. |
| src/Platform/Microsoft.Testing.Platform/Helpers/CIEnvironmentDetector.cs | Becomes the single source of truth with #if TESTFRAMEWORK_CI_DETECTOR toggles for namespace/attributes/ctor/Instance/IsNullOrEmpty behavior. |
| eng/vendored-files.json | Updates vendored-file metadata to reflect the new single-source/linking setup. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 1
| "id": "platform-ci-environment-detector", | ||
| "local_path": "src/Platform/Microsoft.Testing.Platform/Helpers/CIEnvironmentDetector.cs", | ||
| "notes": "Adapted from dotnet CLI telemetry CI detection. Maintained per https://learn.microsoft.com/dotnet/core/tools/telemetry#continuous-integration-detection.", | ||
| "notes": "Adapted from dotnet CLI telemetry CI detection. Maintained per https://learn.microsoft.com/dotnet/core/tools/telemetry#continuous-integration-detection. This file is the single source of truth: it is linked into MSTest.TestFramework via src/TestFramework/TestFramework/TestFramework.csproj and toggled via the IS_CORE_MTP define.", |
Evangelink
left a comment
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
| # | Dimension | Verdict |
|---|---|---|
| 17 | Documentation Accuracy | 🟢 1 NIT |
✅ 21/22 dimensions clean.
- Documentation —
vendored-files.jsonnotes say "IS_CORE_MTP define" but the toggle define isTESTFRAMEWORK_CI_DETECTOR
Design notes (non-blocking):
The single-source-of-truth approach via <Compile Link=...> + TESTFRAMEWORK_CI_DETECTOR is well-established in this codebase (the RoslynHashCode.cs link uses the same pattern). The six #if blocks are easy to follow and each one is necessary. The DefineConstants PropertyGroup placement after the ItemGroup in TestFramework.csproj is valid MSBuild — properties are evaluated before the compiler is invoked regardless of document order.
Functional equivalence is preserved: string.IsNullOrEmpty (TestFramework branch) and RoslynString.IsNullOrEmpty (Platform branch) both delegate to string.IsNullOrEmpty — no behavioral delta. The EnvironmentWrapper.Instance reference in the Instance property resolves to the correct type in each compilation context because both build the file under their own namespace.
🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review (on PR ready) workflow.{ai_credits_suffix} · ◷
| "id": "platform-ci-environment-detector", | ||
| "local_path": "src/Platform/Microsoft.Testing.Platform/Helpers/CIEnvironmentDetector.cs", | ||
| "notes": "Adapted from dotnet CLI telemetry CI detection. Maintained per https://learn.microsoft.com/dotnet/core/tools/telemetry#continuous-integration-detection.", | ||
| "notes": "Adapted from dotnet CLI telemetry CI detection. Maintained per https://learn.microsoft.com/dotnet/core/tools/telemetry#continuous-integration-detection. This file is the single source of truth: it is linked into MSTest.TestFramework via src/TestFramework/TestFramework/TestFramework.csproj and toggled via the IS_CORE_MTP define.", |
There was a problem hiding this comment.
[NIT] Documentation Accuracy
The notes field says "toggled via the IS_CORE_MTP define" but the actual define that gates the TestFramework variant is TESTFRAMEWORK_CI_DETECTOR. IS_CORE_MTP is a separate, unrelated symbol used only inside Microsoft.Testing.Platform.csproj to guard PlatformResources.cs.
A future maintainer searching for #if IS_CORE_MTP to understand how this file is toggled will come up empty.
Recommendation: Change IS_CORE_MTP → TESTFRAMEWORK_CI_DETECTOR in this notes string to match the actual define in TestFramework.csproj and the comment at the top of the linked source file.
Fixes #8985.
Summary
The CIEnvironmentDetector class was duplicated nearly verbatim in src/TestFramework/TestFramework/Internal/ and src/Platform/Microsoft.Testing.Platform/Helpers/, with the same arrays of CI environment variable names and the same IsCIEnvironment() logic in each copy. The two had already begun to drift (XML doc shape, attribute usage), and any future CI provider addition would have required two parallel edits.
Changes
#if TESTFRAMEWORK_CI_DETECTORblocks toggle the real, intentional differences between the Platform and TestFramework variants:Microsoft.Testing.Platform.HelpersvsMicrosoft.VisualStudio.TestTools.UnitTesting)[Embedded]/[ExcludeFromCodeCoverage]attributes (Platform only)publicvsinternal /* for testing */constructorInstancehelper (TestFramework only, consumed byCIConditionAttribute)RoslynString.IsNullOrEmpty(Platform — banned-symbol convention) vsstring.IsNullOrEmpty(TestFramework)src/TestFramework/TestFramework/Internal/CIEnvironmentDetector.csdeleted.src/TestFramework/TestFramework/TestFramework.csprojnow links the canonical file from Platform via<Compile Include="…/Platform/…/CIEnvironmentDetector.cs" Link="Internal/CIEnvironmentDetector.cs" />and definesTESTFRAMEWORK_CI_DETECTOR. This mirrors the existing pattern already used forRoslynHashCode.csin the same csproj, and matches whatMicrosoft.Testing.Extensions.Telemetry.csprojalready does for this file.eng/vendored-files.jsonupdated: removed the now-redundanttestframework-ci-environment-detectorentry and expanded theplatform-ci-environment-detectornote to document the new linked-file relationship.Net diff:
4 files changed, 43 insertions(+), 133 deletions(-).Verification
dotnet buildforMicrosoft.Testing.Platform,Microsoft.Testing.Extensions.Telemetry, andMSTest.TestFramework: 0 warnings / 0 errors acrossnetstandard2.0,net462,net8.0,net9.0.TestFramework.UnitTestsonnet8.0(entire suite): 1189 / 1189 passed, including all 17CIEnvironmentDetectorTestsand all 4CIConditionAttributeTests.