fix(mocha-test-setup): Disambiguate JUnit test names by extending mocha's xunit reporter - #28207
Alex Villarreal (alexvy86) wants to merge 12 commits into
Conversation
…er for the client release group Swaps mocha's built-in xunit reporter for mocha-junit-reporter across the client release group (root pnpm workspace) to fix ambiguous test names in Azure DevOps Test Results. xunit puts the full describe-block path into the JUnit classname attribute and only the innermost it() title into name. ADO's PublishTestResults@2 JUnit parser displays name as the test title, so tests with the same leaf title under different suites are indistinguishable. mocha-junit-reporter reverses this: name becomes the fully-qualified suite path + title, classname becomes the leaf title. Verified locally with @fluidframework/counter's real test suite (214 tests): xunit produced 204/214 (95%) name collisions; mocha-junit-reporter produced zero. Trade-off is longer names (avg 90 chars, max 142 in this package, vs max 67 before). AB#4462 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (584 lines, 13 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
🟡 Changes recommended
Skipped tests need includePending: true, and all shared-helper consumers must have the reporter dependency available.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This pull request replaces Mocha’s xunit reporter with mocha-junit-reporter for client-release-group test reporting, producing unique Azure DevOps test names.
Changes:
- Updates shared and
core-interfacesreporter configuration. - Adds the reporter dependency across client packages and updates the lockfile.
- Preserves existing report paths and supports CJS/ESM runs.
File summaries
| File | Summary |
|---|---|
pnpm-lock.yaml |
Locks the new reporter and dependencies. |
packages/utils/tool-utils/package.json |
Adds the reporter devDependency. |
packages/utils/telemetry-utils/package.json |
Adds the reporter devDependency. |
packages/utils/odsp-doclib-utils/package.json |
Adds the reporter devDependency. |
packages/tools/fluid-runner/package.json |
Adds the reporter devDependency. |
packages/tools/devtools/devtools/package.json |
Adds the reporter devDependency. |
packages/tools/devtools/devtools-view/package.json |
Adds the reporter devDependency. |
packages/tools/devtools/devtools-core/package.json |
Adds the reporter devDependency. |
packages/test/test-version-utils/package.json |
Adds the reporter devDependency. |
packages/test/test-utils/package.json |
Adds the reporter devDependency. |
packages/test/test-end-to-end-tests/package.json |
Adds the reporter devDependency. |
packages/test/stochastic-test-utils/package.json |
Adds the reporter devDependency. |
packages/test/snapshots/package.json |
Adds the reporter devDependency. |
packages/test/mocha-test-setup/test-config.json |
Configures the new reporter; enable includePending: true to retain skipped tests (moderate, 1 vote). |
packages/test/mocha-test-setup/src/mocharcCommon.ts |
Selects the new reporter; ensure every helper consumer declares the dependency (moderate, 2 votes). |
packages/test/local-server-tests/package.json |
Adds the reporter devDependency. |
packages/test/local-server-stress-tests/package.json |
Adds the reporter devDependency. |
packages/test/functional-tests/package.json |
Adds the reporter devDependency. |
packages/service-clients/end-to-end-tests/odsp-client/package.json |
Adds the reporter devDependency. |
packages/service-clients/end-to-end-tests/azure-client/package.json |
Adds the reporter devDependency. |
packages/runtime/test-runtime-utils/package.json |
Adds the reporter devDependency. |
packages/runtime/runtime-utils/package.json |
Adds the reporter devDependency. |
packages/runtime/id-compressor/package.json |
Adds the reporter devDependency. |
packages/runtime/datastore/package.json |
Adds the reporter devDependency. |
packages/runtime/container-runtime/package.json |
Adds the reporter devDependency. |
packages/loader/driver-utils/package.json |
Adds the reporter devDependency. |
packages/loader/container-loader/package.json |
Adds the reporter devDependency. |
packages/framework/undo-redo/package.json |
Adds the reporter devDependency. |
packages/framework/type-factory/package.json |
Adds the reporter devDependency. |
packages/framework/tree-agent/package.json |
Adds the reporter devDependency. |
packages/framework/tree-agent-ses/package.json |
Adds the reporter devDependency. |
packages/framework/tree-agent-langchain/package.json |
Adds the reporter devDependency. |
packages/framework/synthesize/package.json |
Adds the reporter devDependency. |
packages/framework/request-handler/package.json |
Adds the reporter devDependency. |
packages/framework/react/package.json |
Adds the reporter devDependency. |
packages/framework/quill-react/package.json |
Adds the reporter devDependency. |
packages/framework/presence-runtime/package.json |
Adds the reporter devDependency. |
packages/framework/fluid-static/package.json |
Adds the reporter devDependency. |
packages/framework/dds-interceptions/package.json |
Adds the reporter devDependency. |
packages/framework/attributor/package.json |
Adds the reporter devDependency. |
packages/framework/aqueduct/package.json |
Adds the reporter devDependency. |
packages/drivers/routerlicious-urlResolver/package.json |
Adds the reporter devDependency. |
packages/drivers/routerlicious-driver/package.json |
Adds the reporter devDependency. |
packages/drivers/odsp-urlResolver/package.json |
Adds the reporter devDependency. |
packages/drivers/odsp-driver/package.json |
Adds the reporter devDependency. |
packages/drivers/local-driver/package.json |
Adds the reporter devDependency. |
packages/drivers/driver-base/package.json |
Adds the reporter devDependency. |
packages/dds/tree/package.json |
Adds the reporter devDependency. |
packages/dds/test-dds-utils/package.json |
Adds the reporter devDependency. |
packages/dds/task-manager/package.json |
Adds the reporter devDependency. |
packages/dds/shared-summary-block/package.json |
Adds the reporter devDependency. |
packages/dds/shared-object-base/package.json |
Adds the reporter devDependency. |
packages/dds/sequence/package.json |
Adds the reporter devDependency. |
packages/dds/register-collection/package.json |
Adds the reporter devDependency. |
packages/dds/pact-map/package.json |
Adds the reporter devDependency. |
packages/dds/ordered-collection/package.json |
Adds the reporter devDependency. |
packages/dds/merge-tree/package.json |
Adds the reporter devDependency. |
packages/dds/matrix/package.json |
Adds the reporter devDependency. |
packages/dds/map/package.json |
Adds the reporter devDependency. |
packages/dds/legacy-dds/package.json |
Adds the reporter devDependency. |
packages/dds/ink/package.json |
Adds the reporter devDependency. |
packages/dds/counter/package.json |
Adds the reporter devDependency. |
packages/dds/claims/package.json |
Adds the reporter devDependency. |
packages/dds/cell/package.json |
Adds the reporter devDependency. |
packages/common/core-utils/package.json |
Adds the reporter devDependency. |
packages/common/core-interfaces/test-config.json |
Configures the new reporter; enable includePending: true to retain skipped tests (moderate, 1 vote). |
packages/common/core-interfaces/package.json |
Adds the reporter devDependency. |
packages/common/core-interfaces/.mocharc.cjs |
Switches core-interfaces reporting to the new reporter. |
packages/common/client-utils/package.json |
Adds the reporter devDependency. |
experimental/PropertyDDS/packages/property-properties/package.json |
Adds the reporter devDependency. |
experimental/PropertyDDS/packages/property-dds/package.json |
Adds the reporter devDependency. |
experimental/PropertyDDS/packages/property-common/package.json |
Adds the reporter devDependency. |
experimental/PropertyDDS/packages/property-changeset/package.json |
Adds the reporter devDependency. |
experimental/dds/tree/package.json |
Adds the reporter devDependency. |
experimental/dds/sequence-deprecated/package.json |
Adds the reporter devDependency. |
experimental/dds/ot/sharejs/json1/package.json |
Adds the reporter devDependency. |
experimental/dds/ot/ot/package.json |
Adds the reporter devDependency. |
examples/utils/webpack-fluid-loader/package.json |
Adds the reporter devDependency. |
examples/utils/import-testing/package.json |
Adds the reporter devDependency. |
examples/external-data/package.json |
Adds the reporter devDependency. |
examples/data-objects/webflow/package.json |
Adds the reporter devDependency. |
examples/data-objects/table-document/package.json |
Adds the reporter devDependency. |
examples/benchmarks/tablebench/package.json |
Adds the reporter devDependency. |
examples/apps/tree-cli-app/package.json |
Adds the reporter devDependency. |
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
packages/common/core-interfaces/test-config.json:5
mocha-junit-reporteronly registers itspendinghandler whenincludePendingis true, so this configuration drops everyit.skip()/this.skip()case from the XML even though the previous Mochaxunitreporter emitted each as<skipped/>. Core-interfaces has skipped tests as well, so its ADO report will omit those test results. Please setincludePendingtotruein these reporter options.
"mochaJunitReporterReporterOptions": {
"mochaFile": "nyc/{id}junit-report.xml",
"testsuitesTitle": "{id}"
packages/test/mocha-test-setup/test-config.json:5
mocha-junit-reporteronly registers itspendinghandler whenincludePendingis true, so this configuration drops everyit.skip()/this.skip()case from the XML even though the previous Mochaxunitreporter emitted each as<skipped/>. Affected suites contain skipped tests, so the report will omit those ADO test results (and its test counts can no longer match the emitted cases). Please setincludePendingtotruein these reporter options.
- Files reviewed: 83/84 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🔭 PR Review Fleet ReportNote This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement. Verdict: 0 Exterminate, 0 Squash, 2 Investigate Findings
|
…reporter deps) - Set includePending: true in mocha-junit-reporter options for both test-config.json files so skipped/pending tests are still recorded in the JUnit output, matching prior xunit reporter behavior. - Add explicit mocha-junit-reporter devDependency to 10 packages that consume getFluidTestMochaConfig() but previously declared neither mocha-multi-reporters nor mocha-junit-reporter, for consistency with the rest of the workspace's explicit-dependency convention. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
[Agent-generated] Addressing the review feedback about includePending: without it, mocha-junit-reporter never registers a listener for Mocha's pending event, so skipped tests are silently dropped from the JUnit XML entirely — unlike the old xunit reporter, which always emitted a entry. Set includePending: true in both est-config.json files (mocha-test-setup and core-interfaces) in 64a1458 to restore that behavior. |
Azure DevOps's JUnit importer derives the 'Test file' grouping shown in the Tests tab from the report's physical file name when the file contains multiple <testsuite> elements, which mocha-junit-reporter always produces (one per describe block, unlike the single flat <testsuite> the old xunit reporter emitted). Since every package wrote to an identically-named 'junit-report.xml' (differentiated only by directory), Azure DevOps could not tell them apart and displayed every package's tests under an indistinguishable 'JUnit_junit-report.xml' entry. Embed the sanitized package name in the file name itself (e.g. fluidframework-counter-junit-report.xml) so each package's report is uniquely identifiable again. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Deep ReviewReviewed commit Readiness: 3/10 — GETTING STARTED Not ready for sign-off. The reporter choice and package-aware filename fix are supported by representative ESM/CJS runs and live Azure DevOps evidence, but the shared reporting contract lacks durable regression coverage across the client release group. Path to Ready
Context for Reviewers
For human reviewer
Review history (1 prior review)
|
Fixes AutomatedTestStorage/'Group by Test file' grouping in Azure DevOps, which is populated from the JUnit <testcase classname=...> attribute (see JunitResultReader.cs in azure-pipelines-agent). mocha-junit-reporter hardcodes classname to the test's own (non-fully-qualified) title, so ADO was grouping by test name instead of by the file the test lives in. Adds a small FluidJUnitReporter subclass (in mocha-test-setup, duplicated in core-interfaces to avoid a circular dependency) that overrides getTestcaseData to set classname to the test's spec file path, relative to the repo root. Wires it in via reporterEnabled instead of the bare mocha-junit-reporter module name. Since mocha-junit-reporter is now only a dependency of mocha-test-setup (and, separately, core-interfaces, which cannot depend on mocha-test-setup), removes the now-redundant mocha-junit-reporter devDependency from the ~88 consumer packages it was added to previously. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…an ADO Test Run naming mocha-junit-reporter emits one <testsuite> element per describe block, which Azure DevOps's JUnit importer (JunitResultReader.cs) treats as multi-suite: whenever a report file has more than one <testsuite>, ADO unconditionally overwrites the Test Run's display name with 'JUnit_<file name>', regardless of any <testsuite name=...> or file-naming scheme. The earlier per-package file-naming fix only made that fallback name unique per package, not clean - it could never restore a bare package name while mocha-junit-reporter kept emitting multiple suites per file. Replaced FluidJUnitReporter (a mocha-junit-reporter subclass) with FluidXunitReporter, a subclass of mocha's own built-in `xunit` reporter. `xunit` already emits a single flat <testsuite> per report file (matching the pre-migration behavior), so ADO never falls back to the generic name. The override fixes the two problems `xunit` did have: - `name` (the <testcase>'s ADO display title): xunit used the test's bare title, causing collisions between same-named tests in different describe blocks/files - the original motivation for this whole change. Now set to the fully qualified title (test.fullTitle()). - `classname` (used by ADO's "Test file" grouping): xunit used the fully qualified title here too, so that grouping showed test names instead of file names. Now set to the test's spec file path, relative to the repo root. This also means the mocha-junit-reporter dependency, the package-name-in- file-name workaround in mocharcCommon.ts/.mocharc.cjs, and the includePending option (xunit always includes pending tests) are no longer needed; reverted those back to their pre-migration shape. Verified end-to-end with real test runs in @fluidframework/counter and @fluidframework/core-interfaces: each report file now has exactly one <testsuite name="<package name>">, with fully qualified, collision-free testcase names and repo-relative classnames. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…duplicated reporter file - Add comments explaining why mocha-test-setup's xunit-reporter must be referenced by its full package-export-subpath name (mocha-multi-reporters requires() it by that exact specifier, and derives its reporter-options key by camelCasing it) - it can't be shortened to a friendly alias. - Rename core-interfaces' duplicated copy from xunit-reporter-classname.cjs to FluidMochaReporter.cjs (and its class from FluidXunitReporter to FluidMochaReporter): since core-interfaces references it via a relative file path rather than a package export, the file name is not constrained the same way, so a clearer name can be used there instead. - Add a comment noting that file is a deliberately duplicated copy of mocha-test-setup/src/xunitReporter.ts, to keep in sync for future changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adds mocha's own test infra to mocha-test-setup (previously had none): a .mocharc.cjs, test/test:mocha:esm scripts, and a new spec file covering FluidXunitReporter. Covers: - Regression test for a real incident where a bug in the reporter's test() override silently truncated the JUnit report mid-write with exit code 0. - Fully qualified, collision-free testcase name attributes. - Repo-root-relative classname attributes, including the fallback to fullTitle() when a test has no file (e.g. dynamically added tests). - Failed/skipped tests are still recorded, matching xunit's behavior. The same assertions are run twice: once against the reporter imported as ESM, and once loaded via the CommonJS wrapper (xunit-reporter-cjswrapper.cjs) that mocha-multi-reporters actually require()s for consumers running in CJS mode. findRepoRoot is now exported from xunitReporter.ts so the classname test can compute the expected value without duplicating that logic; it isn't part of any api-extractor-tracked public surface. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adding the package's own test script triggers the npm-package-json-clean-script repo policy check, which requires 'clean' to remove the nyc/ output directory mocha's JUnit reporting writes to. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Identical suite paths across files remain ambiguous, and the duplicated reporter lacks integration coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
packages/common/core-interfaces/FluidMochaReporter.cjs:71
- This duplicated implementation has the same remaining collision:
fullTitle()omits the source file, so matching describe/it chains in separate core-interfaces specs still have identical ADO display names. Include the repo-relative file path innametoo so the core-interfaces copy provides the promised disambiguation.
title: { value: test.fullTitle(), enumerable: true },
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Balanced
Verify mocha-test-setup's reporter through its package export subpath and exercise core-interfaces' duplicated reporter through the package's actual mocha-multi-reporters configuration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation and coverage are sound; only two non-blocking stale source links remain.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/common/core-interfaces/.mocharc.cjs:15
- This source link returns 404 because the project is hosted under
stanleyhlng, notstevemao. Update the URL so the implementation detail documented here remains verifiable.
packages/test/mocha-test-setup/src/mocharcCommon.ts:133 - This source link returns 404 because the project is hosted under
stanleyhlng, notstevemao. Update the URL so the implementation detail documented here remains verifiable.
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The intended behavior is covered by integration tests; the remaining comments are non-blocking dependency-hardening suggestions.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
Avoid relying on Mocha's private lib/reporters directory in the mirrored reporter implementations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The reporter behavior, configuration paths, CommonJS compatibility, and generated output are adequately covered without unresolved correctness issues.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Abram Sanderson (Abe27342)
left a comment
There was a problem hiding this comment.
implementation looks fine, lmk when you address the couple comments I left and I can approve
Add the test:mocha script expected by repo-wide CI for mocha-test-setup and parse generated XML reports with xml2js instead of matching XML strings by regex. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Bundle size comparisonBase commit: Pending — |
Description
Azure DevOps Test Results shows test names that are ambiguous or outright duplicated: mocha's built-in
xunitreporter puts the full describe-block path into the JUnitclassnameattribute and only the innermostit()title intoname. ADO'sPublishTestResults@2JUnit parser usesnameas the test's display title in the Tests tab (classnameis not shown as the title), so tests with the same leaf title under different suites are indistinguishable in the UI.This PR fixes that — and a related "Test file" grouping issue — with a small subclass of mocha's own
xunitreporter that overrides just thenameandclassnamefields on each<testcase>.An earlier iteration of this PR instead adopted the third-party
mocha-junit-reporterpackage. It was dropped: it emits one<testsuite>per describe block instead of one per file, which caused ADO to fall back to a generic, non-per-package Test Run name (see below), and it has no option to setclassnameto the spec file path, so it would have needed the same kind of subclassing anyway — with none ofxunit's single-suite guarantee.Evidence
Real example from
@fluidframework/counter's fuzz tests, captured from the actual report files produced bypnpm run test:mocha:esmbefore and after this change.Before — report XML:
Both belong to different fuzz suites, but
name— the field ADO displays — is identical.classname(fully qualified, but not shown as the title) is also the only field with any file/suite-path information — there's nothing at all pointing back to a source file.After — report XML:
nameis now the fully-qualified suite path, so it's unique, andclassnameis the actual spec file the test lives in.Across the whole
@fluidframework/countersuite (214 tests), 204/214 (95%) ofnamevalues collided with another test'snamebefore this change; after, there are zero collisions. The trade-off is longer names (avg 90 chars, max 142 chars in this package, vs. max 67 chars before) — an acceptable cost for uniquely identifiable results, and one already anticipated in AB#4462.Before — ADO:

And grouping by "Test file", where some test names were being used in the field that ADO parses as test file:
After — ADO:

And grouping by "Test file", now correct;

"Test Run" naming, unaffected: the report file still has exactly one
<testsuite name="<package name>">element per file — unchanged from before this PR — so ADO's Tests tab still shows a clean per-package Test Run name. (Root-caused against ADO's actual open-source JUnit importer,JunitResultReader.cs: the run name falls back to a genericJUnit_<file name>whenever a report file has more than one<testsuite>element, regardless of any name attributes or file-naming scheme — this is whatmocha-junit-reporterran into, and why it was dropped in favor of extendingxunit, which never emits more than one<testsuite>per file.)"Test file" grouping, fixed: ADO's Tests tab can also be grouped by "Test file" — this uses the JUnit
<testcase classname="...">attribute directly. The oldxunitreporter setclassnameto the fully-qualified describe-block path, never the spec file, so that grouping showed test names instead of file names.classnameis now the test's spec file path, relative to the repo root (e.g.packages/dds/counter/lib/test/counter.spec.js), so that grouping is meaningful.(Filed AB#83326 as a follow-up to separately investigate an unrelated "Test file" grouping oddity seen for the Playwright JUnit reporter, used in a few packages' end-to-end suites — unaffected by this PR, since it doesn't touch Playwright.)
Scope
This PR only touches the client release group (root pnpm workspace —
packages/**,examples/**,experimental/**), matching AB#4462's acceptance criteria ("build - client" pipeline).build-tools/,server/routerlicious/,common/build/eslint-config-fluid/, andtools/test-tools/are separate release groups/pipelines and are intentionally left on the old reporter pending a follow-up decision.Changes
@fluid-internal/mocha-test-setup: addedFluidXunitReporter(src/xunitReporter.ts, exported as@fluid-internal/mocha-test-setup/xunit-reporter), a subclass of mocha's built-inxunitreporter (used by nearly all client packages viagetFluidTestMochaConfig()) that overrides the<testcase>name(fully-qualified title) andclassname(repo-relative spec file path) attributes. No new dependency —mocha's own reporter is reused directly.@fluidframework/core-interfaces: same fix, applied directly since it can't depend onmocha-test-setup(circular dependency) — it carries its own copy of.mocharc.cjs/test-config.json/the reporter subclass (xunit-reporter-classname.cjs) for that reason.Verified end-to-end (build + real test run) in
@fluidframework/counterand@fluidframework/core-interfaces: each produces a single valid<testsuite name="<package name>">element per report file, with fully-qualified, collision-free testcase names and repo-relative classnames.AB#4462
Reviewer Guidance
The review process is outlined in the pull request guidelines.