Add opt-in fully qualified test name reporting to Helix job monitor - #17104
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in mode for the Helix Job Monitor to report fully qualified test identities (Namespace.Type.Method) to Azure DevOps, addressing collisions/instability caused by framework-provided display names (notably MSTest method-only names and xUnit custom display names).
Changes:
- Introduces
UseFullyQualifiedTestNamewiring end-to-end (YAML template parameter → tool flag/env var → JobMonitor options → AzDO publisher parameters). - Adds
FullyQualifiedNamederivation to parsed test results and switches aggregation/identity to use it when opted in. - Adds
TestNameFormatterto keep titles readable while surfacing the FQN, plus unit tests covering formatter/aggregation/TRX derivation.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.DotNet.Helix/Sdk/Readme.md | Documents the new opt-in fully qualified test name reporting mode and how to enable it. |
| src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/TestNameFormatterTests.cs | Adds coverage for the new display-name formatting rules. |
| src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/ResultAggregatorTests.cs | Verifies new FQN derivation and opt-in grouping behavior vs legacy grouping. |
| src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/LocalTestResultsReaderTests.cs | Ensures TRX inputs with method-only names still produce an FQN. |
| src/Microsoft.DotNet.Helix/JobMonitor/Services/AzureDevOpsService.cs | Plumbs the option into AzDO publisher parameters during upload. |
| src/Microsoft.DotNet.Helix/JobMonitor/JobMonitorOptions.cs | Adds CLI/env-var configuration for the opt-in behavior. |
| src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/TestNameFormatter.cs | Implements title formatting that preserves FQN while retaining display-name-added info. |
| src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/ResultAggregator.cs | Adds FQN support to aggregated results and opt-in grouping by FQN. |
| src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/Model/TestResult.cs | Derives FullyQualifiedName from type+method with reasonable fallbacks. |
| src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/Model/AzureDevOpsReportingParameters.cs | Adds the opt-in flag to publisher configuration. |
| src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/Microsoft.DotNet.Helix.AzureDevOpsTestPublisher.csproj | Exposes internals to the test project for formatter testing. |
| src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/AzureDevOpsResultPublisher.cs | Uses FQN for automatedTestName and formatted titles when enabled. |
| eng/common/core-templates/job/helix-job-monitor.yml | Adds useFullyQualifiedTestName parameter and passes the tool flag when set. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 1
7ac7ea5 to
2150e74
Compare
Helix reported the framework display name as both the visible title and the AzDO automatedTestName. That produced unqualified/unstable identities after MSTest migration (dotnet/sdk#55123): MSTest reports only the method name (colliding across classes) and custom xUnit display names are not unique or stable. Add an opt-in mode that uses the fully qualified name (Namespace.Type.Method) as the stable automatedTestName and formats the visible title as FQN, 'FQN (args)' for parameterized rows, or 'FQN (display name)' for custom names. Grouping is done by FQN when enabled, fixing same-method-name cross-class collisions. Legacy behavior is preserved when disabled. Wired via --use-fully-qualified-test-name / HELIX_USE_FULLY_QUALIFIED_TEST_NAME and the helix-job-monitor.yml useFullyQualifiedTestName parameter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80168a43-3f68-4f13-9a3d-3efd74c731a2
2150e74 to
cd1d9ae
Compare
|
How does this relates to code in https://github.com/dotnet/arcade/tree/main/src/Microsoft.DotNet.Helix/Sdk/tools/azure-pipelines/reporter ? This is another chunk that seems to parse test results and use them somewhere.... The change looks good to me in spirit - new behavior guarded by opt-in switch. I don't touch this area often so I do not have full picture here. |
|
Good question — they're two implementations of the same "parse test result files → publish to AzDO" step, for two different execution models, and this PR only touches the newer one:
They are mutually exclusive by design: when Why this PR only changes the C# side:
Net: this is additive and opt-in on the new monitor path, and it brings that path's TRX handling in line with what the legacy Python reporter already did, plus the extra FQN-as- |
|
why would we not always set this given this is how it used to work with xunit before? is there any downside? |
|
For default xUnit it actually makes almost no difference — xUnit's default display name already is the FQN, so the monitor produces the same automatedTestName whether the switch is on or off. The switch only changes two cases: MSTest (whose testName is method-only, the #55123 bug) and any test using a custom DisplayName. I made it opt-in mainly because automatedTestName is the identity AzDO uses for test history / flakiness / failure grouping, so flipping it is a one-time history discontinuity for anything already reporting through the monitor, and because tests with a deliberate custom DisplayName would see their title change to FQN (display name) and grouping move from display-name to FQN. If we are fine with taking this small break then I am fine with either dropping the feature flag or switching it on by default (if we want some opt-out) |
|
I've been working on new HelixClient and trying to remove the Python dependency. I bump to this and added support for AzDo uploader recently. But I;'de be happy to take it out and keep the test runner simple. But I was not sure if this is used by BA/BI anyhow. Any thoughts on this @premun ? |
|
I'd appreciate to be able to dogfood on SDK, is there a way to dogfood without having this merged? Or could we merge as-is and do a follow-up to promote as default if everything is good? |
|
I'm fine taking since it is opt-in. We can sort out longer term strategy separately. |
|
I do think we should change the default. Reporting just the method name as the test name to AzDO is basically useless for a repo that has a lot of tests |
Let me validate it all works well on SDK and I'll do a follow-up PR. Does it sound right? |
|
yep thanks! |
Problem
The Helix job monitor reports each test to Azure DevOps using the framework-provided display name as both the visible title and the stable
automatedTestName. That is problematic (see dotnet/sdk#55123):Tests.ClassA.MyTestandTests.ClassB.MyTestboth appear asMyTest— and are even merged together because aggregation groups by the display name.[Fact(DisplayName = "...")]get an arbitrary, non-unique name that is unstable over time — a poor choice for a test identity.Change
Add an opt-in mode that reports fully qualified test names. When enabled, the monitor:
Namespace.Type.Method) as the stableautomatedTestName, keeping a consistent identity/history even when the display name changes;Namespace.Type.Methodwhen the display name is just the method name (common default),Namespace.Type.Method ("net10.0")for parameterized rows — keeps the argument list without duplicating the method name (handles the case where the FQN does not end with the display name because of the args part),Namespace.Type.Method (My custom name)when a custom display name adds information.No framework detection is required: the readers already extract type + method separately for trx/xUnit/JUnit, so the FQN is computed deterministically. Legacy behavior is fully preserved when the option is off (default), so existing pipelines are unaffected.
How to enable
Equivalent tool flag
--use-fully-qualified-test-nameor env varHELIX_USE_FULLY_QUALIFIED_TEST_NAME=true.Files
Model/TestResult.cs— derivedFullyQualifiedName.ResultAggregator.cs— FQN plumbed throughAggregatedResult;Aggregate(results, useFullyQualifiedName)groups by FQN when enabled (legacy overload kept).TestNameFormatter.cs(new) — theFQN/FQN (args)/FQN (display)formatting.AzureDevOpsResultPublisher.cs— applies the option toAutomatedTestNameand the title.Model/AzureDevOpsReportingParameters.cs,JobMonitorOptions.cs,Services/AzureDevOpsService.cs— option wiring (CLI + env).eng/common/core-templates/job/helix-job-monitor.yml—useFullyQualifiedTestNamepipeline parameter.src/Microsoft.DotNet.Helix/Sdk/Readme.md— documentation.Tests
New/updated:
TestNameFormatterTests,ResultAggregatorTests,LocalTestResultsReaderTests(MSTest trx FQN derivation). 14 passing, build clean (0 warnings/errors).Addresses the Helix side of dotnet/sdk#55123.