Skip to content

Extract shared collection definitions and add unit test manifest guard - #5756

Closed
Mikael Weaver (mikaelweave) wants to merge 1 commit into
mainfrom
mikaelweave-xunit3-prep-collection-definitions
Closed

Extract shared collection definitions and add unit test manifest guard#5756
Mikael Weaver (mikaelweave) wants to merge 1 commit into
mainfrom
mikaelweave-xunit3-prep-collection-definitions

Conversation

@mikaelweave

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

Copy link
Copy Markdown
Contributor

Description

Preparatory refactor ahead of the xUnit v2 → v3 migration. Everything here is inert on xUnit v2 — it is split out so the migration itself reviews as a single mechanical change.

This is the bottom layer of a 3-PR stack:

Layer Contents Status
1 (this PR) Shared collection definitions + unit test project manifest guard ← you are here
2 Core migration: Extensions.Xunit rewrite, CPM/props switch, ~109 test sources, 27 csproj, Tests.Common guards, CI pipeline YAML #5758
3 Extensions.Xunit.UnitTests harness + .TestAssets follows

Layer 2 is irreducible: Directory.Packages.props deletes the xUnit v2 package versions globally under Central Package Management, Directory.Build.props fans the MTP references out to every project matching Contains('Test'), and Microsoft.Health.Extensions.Xunit binds a single xUnit major that every test project references. Those three jointly force one atomic flip, which is why this prep layer contains only changes that are already correct on v2. The CI pipeline YAML rides with layer 2 for a related reason: main already runs xUnit v2 under Microsoft Testing Platform via the YTest.MTP.XUnit2 shim, which accepts VSTest-style --filter, whereas xUnit v3's MTP requires --filter-query. Swapping the shim without flipping the YAML would leave every integration and E2E leg selecting nothing.

Collection definitions

Collection definitions are moved out of the test classes that declared them, onto dedicated trait-free definition classes:

  • ModelInfoProviderCollection — serialises the SqlServer unit test classes that read or write the process-global ModelInfoProvider. A class fixture only orders the classes that share it, so it cannot stop a third class overwriting the same static; one shared collection can.
  • CustomQueriesTestsCollection — serialises the CustomQueries tests, which share static state.
  • IndexAndReindexCollection — replaces the duplicate [CollectionDefinition] that ReindexTests and CustomSearchParamTests each declared for the same collection name. The definition deliberately carries no traits: a trait on a shared definition is inherited by every member of the collection, and a CI leg filtering that category out would silently drop members that only joined to be serialised.

Manifest guard

Adds build/jobs/scripts/Assert-UnitTestProjectsDiscovered.ps1 and its manifest. The unit test leg runs whatever **/*UnitTests/*.csproj matches, so a project that leaves the glob — renamed, moved, or its directory suffix quietly changed — stops being tested without anything failing. The script checks the glob against the manifest before the tests start.

It is not wired into the pipeline yet; the YAML that calls it lands with the migration. Its automated tests live in the harness project in layer 3, so this script ships here without them.

Related issues

Addresses AB#189958.

Testing

Verified by execution on the unmodified v2 tree:

  • Microsoft.Health.Fhir.SqlServer.UnitTests1053 passed / 0 failed, both with and without this change, confirming the collection extraction is a true no-op (identical totals rule out silently-dropped tests).
  • Assert-UnitTestProjectsDiscovered.ps1 — prints Matched 19 unit test project(s) against 19 expected, exit code 0. The manifest reads 19 here and goes to 20 in layer 3, when the harness project exists.
  • Microsoft.Health.Fhir.R4.Tests.E2E — compiles clean (0 warnings, 0 errors) with IndexAndReindexCollection.cs explicitly registered in Microsoft.Health.Fhir.Shared.Tests.E2E.projitems, so the new shared file genuinely compiles into consumers rather than being silently skipped.

Note: dotnet build of the E2E projects trips the repo's VerifyExactSdkVersion target locally because the installed SDK is 10.0.303 while global.json pins 10.0.302. That is a pre-existing local-environment condition unrelated to this change, so global.json was left untouched and the E2E verification was run via the Compile target instead.

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 (test-only preparatory refactor, no product code or behavior change)

…anifest guard

Preparatory refactor ahead of the xUnit v2 to v3 migration. Everything here is
inert on xUnit v2 and is split out so the migration itself reviews as a single
mechanical change.

Collection definitions are moved out of the test classes that declared them and
onto dedicated, trait-free definition classes:

- ModelInfoProviderCollection serialises the SqlServer unit test classes that
  read or write the process-global ModelInfoProvider. A class fixture only
  orders the classes that share it, so it cannot stop a third class overwriting
  the same static; one shared collection can.
- CustomQueriesTestsCollection serialises the CustomQueries tests, which share
  static state.
- IndexAndReindexCollection replaces the duplicate [CollectionDefinition] that
  ReindexTests and CustomSearchParamTests each declared for the same collection
  name. The definition deliberately carries no traits, because a trait declared
  on a shared definition would be inherited by every member of the collection
  and silently dropped by CI legs that filter that category out.

Also adds build/jobs/scripts/Assert-UnitTestProjectsDiscovered.ps1 and its
manifest. The unit test leg runs whatever '**/*UnitTests/*.csproj' matches, so a
project that leaves the glob stops being tested without anything failing. The
script checks the glob against the manifest before the tests start. It is not
wired into the pipeline yet; the YAML that calls it lands with the migration.

Verified inert: Microsoft.Health.Fhir.SqlServer.UnitTests reports 1053 passed /
0 failed both with and without this change, and the R4 E2E project compiles with
the new shared file registered in the .projitems.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 407889a1-346a-479d-9340-5d02c0278d8b
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.59%. Comparing base (14f5285) to head (638b6aa).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5756      +/-   ##
==========================================
- Coverage   78.61%   78.59%   -0.02%     
==========================================
  Files        1019     1019              
  Lines       37659    37659              
  Branches     5729     5729              
==========================================
- Hits        29606    29599       -7     
- Misses       6654     6659       +5     
- Partials     1399     1401       +2     

see 8 files with indirect coverage changes

🚀 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

Superseded by #5761.

That PR re-does this migration from scratch against main as a single change, and is a net reduction: 144 files, +1,467 / -1,967, net -500 lines. This stack grew the custom test framework; #5761 deletes the retry subsystem (676 lines), the Xunit.SkippableFact package and its 438 call sites, and the YTest.MTP.XUnit2 shim, taking src/Microsoft.Health.Extensions.Xunit/ from 1,421 to 1,045 lines.

Test selection is preserved at name level - every CI leg was enumerated against a v2 control and diffed as a name multiset (missing 0 / added 0), not compared by count.

Closing to keep review attention on #5761. The branch is not deleted, so this can be reopened if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants