Skip to content

Migrate xUnit v2 tests to xUnit v3 on Microsoft Testing Platform - #5762

Open
Mikael Weaver (mikaelweave) wants to merge 21 commits into
mainfrom
mikaelweave-xunit3-minimal-migration
Open

Migrate xUnit v2 tests to xUnit v3 on Microsoft Testing Platform#5762
Mikael Weaver (mikaelweave) wants to merge 21 commits into
mainfrom
mikaelweave-xunit3-minimal-migration

Conversation

@mikaelweave

@mikaelweave Mikael Weaver (mikaelweave) commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Description

Migrate the test suite from xUnit v2 to xUnit v3 while continuing to use Microsoft Testing Platform.

  • Keep the existing datastore and format test variants.
  • Keep the existing CI test partitions and retry behavior.
  • Replace v2 test filters with equivalent xUnit v3 filters.
  • Remove obsolete custom retry and runner code.
  • Keep only the custom fixture support required by this repository.

This change does not alter FHIR product behavior or the SQL schema.

Related issues

Addresses AB#189958.

Testing

  • The PR pipeline passed in build 51284.
  • R4 and R5 SQL E2E result sets matched the migration baseline with no missing or added tests across 2,136 R4 and 1,922 R5 results.
  • R4 Integration discovery matched main with no missing tests for the full, SQL, and Cosmos selections.
  • CodeQL passed.

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
  • When changing or adding behavior, if your code modifies the system design or changes design assumptions, please create and include an ADR.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Skip

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… Retry sites

- Swap all 28 test csprojs to xunit.v3 (drop Microsoft.NET.Test.Sdk, xunit.runner.visualstudio, Xunit.SkippableFact)

- 438 SkippableFact sites -> Fact/Theory + Assert.SkipWhen/SkipUnless; 32 reason-less sites use shared SkipReasons.Unspecified

- 26 RetryFact/RetryTheory -> Fact/Theory; 3 RetryAsync helper sites unwrapped

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…TestFramework attrs, and port CI filters to xunit.v3 query grammar

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…rcing xunit.v3.core from framework, suppress xUnit1051 in test ruleset

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…tform usings; suppress xUnit1031

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…s.props

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ility

The xunit.v3 meta-package binds xunit.v3.mtp-v1 (MTP 1.9.1), which is
incompatible with the MTP 2.0.2 extension stack (Retry/TrxReport) that the
CI legs invoke via --filter-query/--retry-failed-tests/--report-trx. Reference
xunit.v3.mtp-v2 directly (pulls MTP 2.0.2 transitively) and set
UseMicrosoftTestingPlatformRunner=true so the standalone test exe is an MTP
host, restoring the entrypoint behavior the deleted YTest.MTP.XUnit2 shim
provided.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 87a059be-5733-43cf-8cf4-7ab5cf57f26f
…t, drop dead xunit.v3 pin

- tools/ABTestRunner/Invoke-ABTest.ps1: emit MTP --filter-query /[(...)] instead of
  VSTest --filter (the latter exits 5 'Unknown option' on an xunit.v3 MTP host);
  each predicate keeps its original polarity.
- tools/MultiInstanceRunner/README.md: same --filter -> -- --filter-query conversion; net9.0 -> net10.0.
- FixtureArgumentSetTestMethod.cs: correct the XML-doc claim that base serialization round-trips
  merged traits (it does not); explain why that is safe and name the one affected consumer.
- Directory.Packages.props: remove the now-unreferenced xunit.v3 meta-package pin
  (all 28 projects use xunit.v3.mtp-v2; 0 consumers remain).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 87a059be-5733-43cf-8cf4-7ab5cf57f26f
Fix A/B: rebuild fault cases from base XunitTestMethod and attach a
conservative union of raw class/method flag traits, so the reporter no
longer re-enters uniqueID/traits reflection (which re-threw out of
FindTestsForType, silently dropping the class, exit 0) and so fault
cases carry traits and are selected under positive filter-query legs
(E2E/export), not only negative ones.

Fix C: use IsDefined at the attribute-presence check (no instantiation)
and retrieve each method's attribute inside its per-method try, so a
throwing method attribute isolates as a loud error case instead of
taking down the whole class or vanishing untraited.

Fix D: correct the ComputeVariants comment to describe per-dimension
REPLACE (method flag replaces class flag for that dimension), which
matches v2 and is A/B-verified; drop the false 'never adds variants
beyond the class cross product' claim.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 87a059be-5733-43cf-8cf4-7ab5cf57f26f
When a method's FixtureArgumentSets attribute constructor itself throws,
its flag values are unknowable, so CollectRawFlags catches and the fault
case is emitted untraited - invisible to positive trait filters but still
visible unfiltered and under negative legs. Comment the swallow so it is
not "simplified" away by a future reader. Comment-only; no behaviour change.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 87a059be-5733-43cf-8cf4-7ab5cf57f26f
…itless store-named classes)

The two negative integration legs used --filter-query "/[(DataStore!=X)]".
In xunit.v3 a trait "!=" means "differs OR absent", so store-named but
DataStore-traitless classes (e.g. SqlServerImporterIntegrationTests) that v2
excluded by FullyQualifiedName!~ survived and were double-scheduled onto the
opposite store's leg (+43 cosmos, +7 sql). Those classes hard-fail at fixture
construction without their backend, so this would have reddened CI post-merge.

A query-filter equivalent is impossible in runner 3.2.2 (any explicit path
segment zeroes the trait predicate; multiple --filter-query args OR). Switch
the two negative legs to simple filters, which AND across filter types:
  --filter-not-trait "DataStore=X" --filter-not-class "*X*"
Verified on the R4 Integration assembly: found 291 (cosmos) / 439 (sql),
exact v2 membership (missing 0 / added 0). Both clauses load-bearing
(not-class alone 596; not-trait alone 334/446). Positive E2E/export legs
are unchanged. Comment added at each site to prevent harmonising back.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 87a059be-5733-43cf-8cf4-7ab5cf57f26f
Removing the custom test-retry helper left three immediately-invoked
`await ((Func<Task>)(async () => { ... }))()` wrappers that allocate a
delegate solely to invoke it in place -- dead scaffolding with no effect
on control flow. Inline the three bodies (2 in BasicSearchTests, 1 in the
Skip'd QueueClientTests) so net complexity keeps going down. Body-only
change: test names and discovery membership are unchanged on all four CI
legs. Whole-test retry remains covered by the pipeline's
`--retry-failed-tests 3`.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 87a059be-5733-43cf-8cf4-7ab5cf57f26f
Comment thread src/Microsoft.Health.Extensions.Xunit/FlagCodec.cs Fixed
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@da355bc). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5762   +/-   ##
=======================================
  Coverage        ?   78.64%           
=======================================
  Files           ?     1017           
  Lines           ?    36981           
  Branches        ?     5638           
=======================================
  Hits            ?    29085           
  Misses          ?     6479           
  Partials        ?     1417           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mikaelweave

Copy link
Copy Markdown
Contributor Author

Status: full pipeline is green — one owner action left

ADO build 51080 completed succeeded: 42 / 42 jobs, 22 / 22 stages, 0 failed, 0 skipped.
GitHub checks: 47 pass / 0 fail / 1 pending.

Every stage this change actually touches has now executed for real — all five E2E legs on both data
stores across R4 / R5 / STU3, all five reindex legs, all three bulk-update legs, and all five
integration legs. Their executed counts match two independent v2 control builds test-for-test and
skip-for-skip; see the new “Live CI evidence” section in the PR description for the 18-leg
table, including the three legs that differ and why the controls prove the migration is not the cause.


The one thing I cannot do for you: Check Metadata

This is the only non-passing check, and it is not a test or build failure. The workflow requires a
DevOps work item reference in the PR description:

if (body.toLowerCase().includes('ab#') == false && ...)
  errors += '- FHIR Team: A DevOps workitem is required. Use AB#123 syntax or link to DevOps item.'

Action: add AB#<id> to the PR description with the real work item number. I deliberately did not
invent one — a fabricated ID would link this PR to the wrong work item, and that is worse than a red
check. This is a one-line edit and the check goes green.

(Note: the run against this head recorded startup_failure, so the check currently reads pending
rather than fail. Re-running it after the AB# is added will resolve both.)


Why this PR replaced #5761

#5761 was raised from a fork. On this repository a cross-fork PR does not get the secrets needed by
Setup Test Environment, that stage failed, and it cascaded downstream. Both PRs run the same
22-stage pipeline — but on the fork build (51075)
only 7 stages ran, 1 failed and 14 were skipped, so not one test leg ever executed
(26 GitHub checks, 7 of them failing). On this PR, all 22 stages ran and all 22 succeeded (48 checks,
0 failing). Since the entire question this change raises is "do the test legs still select and run the
same tests?"
, #5761 could not be evaluated on its merits.

This PR runs from a branch on microsoft/fhir-server itself, which is why the matrix above exists.
#5761 is closed and superseded; the head commit is identical apart from the retry-residue cleanup
(d3e65a87b).


Suggested reading order for reviewers

The description is long because the claims are quantified, but it is structured for skimming — every
section past the summary is collapsed. If you only read three things:

  1. “Live CI evidence” — the 18-leg table above. This is the primary gate.
  2. “Net growth ≠ review surface” — which files actually need human eyes (six, not 144).
  3. “Integration-leg exclusion filters — a regression found late, and fixed” — the one place the
    mechanical port went wrong, how it was caught, and the proof it is fixed.

Net change is −509 lines across 144 files: this migration deletes custom test infrastructure
rather than adding to it.

Restore bounded collection parallelism, simplify fixture context handling, and dispose test-owned service providers without including the separate xUnit regression projects.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: e0e0f572-5e9c-4713-8034-59ffcfba63cd
Leave collection scheduling to xUnit's default assembly-runner fan-out.
The custom runner still injects fixture arguments.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0e0f572-5e9c-4713-8034-59ffcfba63cd
Remove the dummy NotTest and xunit.v3.assert reference from the helper library.
Revert agent model pins and the superpowers gitignore tweak from this migration PR.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0e0f572-5e9c-4713-8034-59ffcfba63cd
@mikaelweave
Mikael Weaver (mikaelweave) marked this pull request as draft September 4, 2026 03:11
Resolve reindex and storage lifecycle changes using xUnit v3 ValueTask signatures.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0e0f572-5e9c-4713-8034-59ffcfba63cd
Expose variant traits through the existing class metadata interface so native methods and deferred theory rows inherit them. Remove the custom method wrapper, its private-field reflection, and duplicate flag serialization.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0e0f572-5e9c-4713-8034-59ffcfba63cd
Use xUnit v3's supported uniqueID constructor argument instead of writing its private class identity field. Preserve the same identities and native serialization.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0e0f572-5e9c-4713-8034-59ffcfba63cd
Use the native assembly context, enum serialization, and constructor argument lookup while preserving fixture variants and collection exception isolation.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0e0f572-5e9c-4713-8034-59ffcfba63cd
Publish an execution error when method enumeration yields no recoverable methods, preserving native serialization, class traits, and method-specific fault names. Dispose the skipped heartbeat test's cancellation source.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e0e0f572-5e9c-4713-8034-59ffcfba63cd
@mikaelweave
Mikael Weaver (mikaelweave) marked this pull request as ready for review September 11, 2026 21:05
@mikaelweave Mikael Weaver (mikaelweave) added the Open source This change is only relevant to the OSS code or release. label Sep 11, 2026
…nimal-migration

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 87a059be-5733-43cf-8cf4-7ab5cf57f26f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Open source This change is only relevant to the OSS code or release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants