Xunit2 -> Xunit3 Migration - #5507
Closed
Mikael Weaver (mikaelweave) wants to merge 111 commits into
Closed
Conversation
- Add xunit.v3, xunit.v3.mtp-v2, xunit.v3.assert, xunit.v3.common, xunit.v3.extensibility.core (3.2.2) - Add Microsoft.Testing.Extensions.Telemetry, Microsoft.Testing.Platform.MSBuild (2.0.2) - Bump Microsoft.Testing.Extensions.CodeCoverage 18.1.0 -> 18.3.1 - Bump xunit.runner.visualstudio 3.0.2 -> 3.1.5 - Remove YTest.MTP.XUnit2, xunit, xunit.assert, xunit.extensibility.core, Xunit.SkippableFact Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Enable UseMicrosoftTestingPlatformRunner - Switch xunit.runner.json from Content to None + CopyToPublishDirectory=Never - Replace YTest.MTP.XUnit2 reference with MTP extensions (Telemetry, MSBuild) - Drop xunit.runner.visualstudio remove directive (now used as normal ref) - Suppress xUnit1031, xUnit1051, SA1208, SA1209, SA1518 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- CustomAnalysisRules.Test.ruleset: suppress xUnit1031, xUnit1051 and StyleCop SA1208/SA1209 - THIRDPARTYNOTICES.md: remove YTest.MTP.XUnit2 and Xunit.SkippableFact entries - xunit.runner.json: update schema URL to v3 configuration schema Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace xunit package with xunit.v3.mtp-v2, xunit.v3.common, and xunit.v3.extensibility.core. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- CustomXunitTestFramework now inherits the v3 XunitTestFramework (configFileName ctor), overrides CreateDiscoverer/CreateExecutor taking Assembly. - CustomXunitTestFrameworkDiscoverer uses v3 XunitTestAssembly + IXunitTestClass + ValueTask-based FindTestsForType/FindTestsForMethod with Func<ITestCase, ValueTask<bool>> callback; integrates new FixtureArgumentSet* types. - CustomXunitTestFrameworkExecutor is restructured around RunTestCases + custom XunitTestAssemblyRunner/CollectionRunner/ClassRunner/TestRunner types that propagate FixtureArgumentSet variants, and uses reflection to seed the v3 XunitTestAssembly AssemblyFixtureTypes lazy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
xUnit v3 needs its own IXunitTestClass/IXunitTestCollection/IXunitTestMethod implementations that carry the fixture-argument-set variants. - Add FixtureArgumentSetTestClass, FixtureArgumentSetTestCollection, FixtureArgumentSetTestMethod. - Add FixtureArgumentSetsAttribute (carries the Flags enum combinations + CollectionBehavior). - Remove obsolete TestClassWithFixtureArguments and TestClassWithFixtureArgumentsTypeInfo (v2 ITypeInfo-based). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- AssemblyFixtureAttribute, RetryFactAttribute, RetryFactDiscoverer, RetryTestCase, RetryTheoryAttribute, RetryTheoryDiscoverer updated for xUnit v3 API (Assembly-based ctors, ValueTask discovery, IXunitTestCase/IXunitTestCollection abstractions). - Add local Skip, SkippableFactAttribute, SkippableTheoryAttribute classes replacing the now-removed Xunit.SkippableFact package. Skip.If/IfNot throw SkipException.ForSkip(reason) which xUnit v3 recognizes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Swap 'xunit' PackageReference to 'xunit.v3.mtp-v2' across all 19 *UnitTests*.csproj and Microsoft.Health.Fhir.Tests.Common.csproj. - Remove 'Xunit.SkippableFact' PackageReference from Azure.UnitTests and the four *.Core.UnitTests projects (replaced by local Skip/SkippableFact types in Microsoft.Health.Extensions.Xunit). - Add ProjectReference to Microsoft.Health.Extensions.Xunit in Azure.UnitTests (needed for the local SkippableFact attribute). - Swap 'xunit.assert'/'xunit.extensibility.core' to their 'xunit.v3.*' equivalents in SchemaManager.UnitTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Swap 'xunit' PackageReference to 'xunit.v3.mtp-v2' and drop 'Xunit.SkippableFact' from the four *.Tests.Integration.csproj files (R4, R4B, R5, Stu3). The xunit.runner.json copy metadata is handled in the shared .projitems (coming in a later commit). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Swap 'xunit' PackageReference to 'xunit.v3.mtp-v2' and drop 'Xunit.SkippableFact' and 'System.IO.FileSystem.AccessControl' across all four *.Tests.E2E.csproj files (R4, R4B, R5, Stu3). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- TestFramework now uses typeof() syntax (v3 no longer takes typeName/assemblyName string pair). - AssemblyFixture moves to Xunit namespace (fully qualified to disambiguate from Microsoft.Health.Extensions.Xunit). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
xUnit v3 changes IAsyncLifetime.InitializeAsync/DisposeAsync from Task to ValueTask. Update all test fixtures and test classes that implement IAsyncLifetime, OnInitializedAsync, or OnDisposedAsync hooks accordingly. Also remove stale 'using Xunit.Abstractions;' where ITestOutputHelper moved to the Xunit namespace. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ojitems for MTP xUnit v3 MTP runner requires xunit.runner.json to be copied to the test output directory. Add the Shared.Tests.Integration xunit.runner.json and register it (plus the E2E one) in the shared .projitems with Link + CopyToPublishDirectory=Never so each downstream test project picks it up. Add v3 $schema URL. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
xUnit v3 parallelizes more aggressively by default. CustomQueriesUnitTests mutates shared static state (CustomQueriesHash) so add a CollectionDefinition with DisableParallelization=true and attach the test class to it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Tests ITestOutputHelper moved to the Xunit namespace in xUnit v3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When using Microsoft Testing Platform (xUnit v3) the DotNetCoreCLI@2 'test' command no longer emits TRX automatically. Pass --report-trx, disable built-in test-result publishing, and add an explicit PublishTestResults@2 step. Keep failTaskOnFailedTests=true for the non-coverage path and keep it false for the coverage path until testfx#7167 is fixed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The MTP runner does not accept the legacy VSTest '--filter FullyQualifiedName~X' expressions. Switch to '--filter-query' with trait expressions (DataStore + Category) for E2E and export jobs, and use '--filter-not-class'/'--filter-not-trait' for integration-test exclusions. Also pass 'publishTestResults: false' on the integration-test DotNetCoreCLI@2 tasks so the separate PublishTestResults@2 step owns TRX publishing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MTP requires xunit.runner.json to be copied into each test project's output directory. Add a PowerShell task that asserts the file is present for every integration-test project prior to test execution, and prints its contents to the build log for diagnosis. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove 'using Xunit.Abstractions;' (namespace removed in v3; ITestOutputHelper lives in Xunit) - Add 'using System.Threading.Tasks;' where IAsyncLifetime now returns ValueTask - Add 'using Xunit;' to files that lost the only Xunit reference via the Abstractions removal Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mikael Weaver (mikaelweave)
changed the title
Mikael Weaver (mikaelweave)
marked this pull request as ready for review
April 17, 2026 06:30
Mikael Weaver (mikaelweave)
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+170
to
+186
| foreach (var fixtureType in classFixtureTypes) | ||
| { | ||
| var constructor = fixtureType.GetConstructors() | ||
| .SingleOrDefault(ctor => !ctor.IsStatic && ctor.IsPublic); | ||
| if (constructor == null) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| foreach (var parameter in constructor.GetParameters()) | ||
| { | ||
| if (parameter.ParameterType.IsEnum) | ||
| { | ||
| fixtureParameterTypes.Add(parameter.ParameterType); | ||
| } | ||
| } | ||
| } |
The CustomXunitTestFrameworkDiscoverer was creating FixtureArgumentSetTestCollection with disableParallelization hard-coded to false, ignoring the [CollectionDefinition(DisableParallelization = true)] attribute on test classes like ReindexJobTests. This caused long-running integration tests that share a single SQL fixture to run concurrently and hang on resource contention. Now the discoverer reads CollectionDefinitionAttribute.DisableParallelization from the test class and propagates it to the test collection created for each fixture-argument variant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
xunit v3 defaults parallelizeTestCollections to true. Integration tests share SQL fixtures and hang when collections run concurrently (ReindexJobTests). Match the E2E config by setting parallelizeTestCollections: false. Reverts the speculative DisableParallelization plumbing in the custom discoverer (the custom discoverer creates one collection per method, so DisableParallelization on a single collection had no effect). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…xunit v2 behavior In xUnit v2, all methods of a test class belong to a single test collection and therefore run serially with a shared IClassFixture lifetime. The v3 rewrite of CustomXunitTestFrameworkDiscoverer accidentally created a new FixtureArgumentSetTestCollection for every (method, variant) combination, which caused xUnit v3's scheduler to run methods of the same class in parallel. This broke tests like ReindexJobTests which rely on per-class serialization and shared fixture state, causing intermittent hangs on the SQL integration test pipeline stages. Cache collection/class instances per (class, variant) inside FindTestsForType so methods of the same test class + variant share the same collection instance, matching v2 semantics. Revert the over-broad parallelizeTestCollections=false workaround from xunit.runner.json; cross-class parallelism is restored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…llelism In xunit v3 test collections are the unit of parallelism: methods within a collection run serially, collections run in parallel. The previous discoverer used SharedPerVariant as the default, which collapsed every class with the same fixture argument variant (for example [FhirStorageTestsFixtureArgumentSets(DataStore.SqlServer)]) into one collection. All SqlServer integration tests across all classes then ran serially, causing R4/R4B SQL test stages to exceed the 60 minute timeout. Always scope the collection identity to the test class so each (class, variant) pair is its own collection. This matches xunit v2 behavior: one IClassFixture per class, methods within a class run serially, and classes run in parallel. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Linux unit-test 'Publish unit test results' task was the only publish step still using failTaskOnFailedTests: true. MTP's --retry-failed-tests records the original (pre-retry) failure in the trx, so a retried-and-passed test still failed the publish step even though dotnet test succeeded. Match the workaround used by every other PublishTestResults task in the repo (coverage, e2e, sql, cosmos, export). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ute wrote it The check named TraitAttribute exactly, but xUnit propagates any ITraitAttribute from a collection definition to the classes that join it - TraitAttribute is merely the built-in spelling of one. A custom trait attribute on a definition would therefore tag its members while the guard saw nothing, which is the same silent leg-reshaping the guard exists to catch, arriving by a slightly different route. The discoverer this branch changes already reasons in terms of the interface; this now agrees with it. Verified by putting a custom ITraitAttribute on a definition another class joins: under the old exact-type test the assembly checks all passed, and under this one all three report the offender by name. The scan above is left matching TraitAttribute exactly on purpose - it reads a trait's name out of the first constructor argument, a shape only that attribute guarantees - and a comment now says so. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Membership was read only from the non-generic [Collection] attribute, and only when it carried a constructor argument. xUnit v3 adds [Collection<TDefinition>], which names the definition in its generic argument and takes no constructor argument at all - so it missed the guard twice over. On a branch whose whole purpose is moving this repository to v3, the spelling people are about to start writing was the one spelling that could reintroduce the incident unseen. Ask the interface question instead, then read membership from whichever of the three forms the class used: a name, a definition type, or a generic argument. Verified with a definition another class joins as [Collection<TDefinition>]: the assembly checks passed with the previous code and report the offender by name with this one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ayed An intercepting bus replays whatever it still holds when it is disposed, so a failure can never be lost by forgetting to report it. The cost is that silence is not something a caller can express by omission: leaving a bus alone publishes it anyway, a moment later. Deciding which attempt wins was therefore only half the job. The empty-summary path discarded the earlier attempt when the current one won, but not the current attempt when the earlier one won; the crash path did the same, and its comment said so - it expected the finally to replay the current attempt's messages, next to the ones just published for the earlier attempt. When the current attempt was holding an abstention, that is an ITestSkipped arriving immediately after an ITestFailed for the same test, which is the reordering the skip-after-failure path a few lines above exists to prevent. Disposal even announced it, logging an internal error while replaying. Both paths now go through one rule that publishes the winner and discards the loser, so the two cannot drift apart again. Pinned with the real bus: under the previous behaviour the test records ITestFailed followed by ITestSkipped, and disposal is asserted to have nothing left to fall back on. Reachable only if an exception escapes the runner after an abstention was held, which xUnit normally converts into a failure - hence a unit-level pin rather than a test asset. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reading a declaration's attributes constructs all of them, so a single attribute whose constructor throws costs that declaration every trait beside it - including the Category a CI leg selects on. The attribute that throws need not have anything to do with traits, which is what makes this easy to arrive at without meaning to. The fallback now names the attribute types from metadata and constructs them one at a time, so the sound ones survive the broken one. Reaching this path needs two faults at once: a trait attribute that throws, which is what sends the read down the fallback, and a second attribute that throws when constructed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…f them A declaration whose dimensions all name nothing was already rescued: the failure standing in for its tests is reported once per value each dimension's type declares, so a leg selecting positively on one of them still sees it. A declaration that names nothing in only some of its dimensions has the same empty product and the same missing tests, but was left carrying only the dimensions it did name - and a leg selecting positively on a silent dimension, which is how every E2E and export leg selects a data store, matched nothing and passed green and empty. The dimensions a declaration leaves open are now widened individually. Where the open position was meant to be inherited from the class, the class's own declaration is closed over separately and still contributes its combinations, so the only cost is a failure reported under a value the declaration did not name - loud, and traceable back to it. The alternative is silence. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…from The custom framework is what expands fixture argument sets into variants, and the variants are what carry the DataStore trait every E2E and export leg selects positively on. Losing the declaration throws nothing: the classes are discovered as ordinary xUnit tests, carry no data store trait, and are filtered out. A filtered-out class never constructs its fixture, so no connection is attempted and nothing errors - the leg runs the handful of classes that declare traits by hand, reports success, and some two thousand E2E tests are simply absent. Also traits the E2E-only copy of the assembly checks, which compiled into the E2E assemblies but was invisible to their filters, so a check added there would have run nowhere. The copy shared with the integration assemblies stays untraited on purpose - those legs select by excluding a store, and an exclusion drops a class that names both - and now says so. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A leg that filters positively on DataStore reports success when its filter matches nothing, so the count is the only thing separating everything passed from nothing ran. Each leg now asserts a floor on what discovery selects, well below its real count, before the run starts. The floor deliberately does not ride on --minimum-expected-tests: that option is applied again to the much smaller pass --retry-failed-tests starts, so any floor above the size of a retry pass turns a flake that recovered into a failed leg with exit 9. Measured on a probe: three passing tests and one that fails only on its first run exit 9 under --retry-failed-tests 3 --minimum-expected-tests 3. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Disposal replaying held messages is both the fail-safe for a caller that forgot them and the ordinary way the last surviving result reaches the bus, and it announced an internal error for both. Every run printed the line, so nobody had reason to believe it the one time it meant something. The deliberate handoff now says so, and the warning is left for the case it was written for. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…es from Three things that were only obvious from the outside: a failure arriving after an abstention has been deferred deliberately never sets LastFailureMessage, because the outcome that follows reports a failure anyway and capturing it would publish two results for one test; the E2E runner settings are not published, so the legs' parallel-none switch is the only thing keeping suites that share a server off each other; and the bus now writes diagnostics through an injectable sink so a test can read them without redirecting the console, which is process-wide and would have made those tests unsafe to run beside the rest. The extensions harness also joins the R4 and R5 solution filters, so the tests guarding this machinery are present in the solution a developer actually opens. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A leg that filters on a DataStore trait can only select a class that either carries that trait by hand or gets it from an argument-set attribute the framework expands into per-store variants. A class with neither is silently invisible: no leg selects it, every leg still exits zero, and nothing says so. Four E2E classes are in that state today, and were on main too, so this adds a guard rather than a behaviour change. The guard fails on an unexpected unreachable class and equally on an allow-list entry that has since become reachable, so the list cannot quietly rot into a blanket exemption. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The integration legs select by excluding the other store's trait. An exclusion is the mirror hazard of a positive filter: a class that gains a trait it did not have before drops out of the leg, and the leg still passes. They had no floor, so add the same pre-run discovery count the other legs use, with the identical filter arguments the run itself uses. The E2E floor parameter loses its default. A default is the one value that is certainly wrong for a leg whose author never considered the question, and it would apply silently. All five call sites already pass one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
One claimed a deferral can only originate from an abstention; ordinary deferred failures, the common retry path, set the same flag. The other described retry selection as applying to transient failures, which reads as a transience test that ShouldRetry does not perform. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Both scenarios need one collection to cancel the run while the other is mid-attempt, and both arranged that by sleeping: the canceller slept, the other slept longer, and the interleave was whatever the machine's speed made it. On a loaded agent the cancellation lands after the other side's window has closed, and the run then reports a failure about the scenario rather than about the product. That is a test suite lying about the code, which is the one thing this whole change exists to prevent. Each side now waits to be told, so the order holds at any speed. The waits are asynchronous because a blocking one holds a runner thread: with a blocking wait the collection being waited for did not get a thread until the waiter had already given up, which was measurably worse than the sleeps it replaced. The run-duration backstop moves from 20s to 60s. It only has to separate a cancelled run from one that sat through three 30s retry delays, so 60s still says exactly that while no longer failing a correct run that was merely slow to start a child process. Verified: the old timing fails deterministically when the canceller is made 8s late, the new handshake passes under the same condition, and the harness is green over eight consecutive runs including three with every core saturated. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Nine test assemblies declare an assembly fixture, name it in no constructor anywhere, and depend entirely on what its constructor does: install the FHIR model info provider. This change removed the local implementation that used to guarantee that and now leans on the framework to do it. Nothing anywhere asserted the guarantee, so if it ever stopped holding the provider would go uninstalled and the failure would surface far from the cause. A scenario now declares such a fixture the same way and a test reads whether it was constructed before that test ran, which is precisely the property the nine assemblies rely on. Verified by removing the declaration: the scenario then fails, and passes again once it is restored. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The retry extension every leg runs with treats an attempt that did not fail as a recovery. A test that fails and then skips - Assert.Skip, or a SkippableFact deciding at runtime that its environment is not there - is therefore reported as recovered: the final TRX records NotExecuted, the runner exits 0 and the leg is green. Reproduced here as a scenario: with a second retried test present to keep the run non-empty, an attempt of 2 failed followed by 1 passed + 1 skipped prints "Passed!" and exits 0. Nothing in the repository skips dynamically today, so this is a trap laid for the next person rather than a live hole, and it is invisible at review time because the skip and the failure are in different tests. Every leg now reconciles the attempts it kept: Assert-RetriedFailuresPassed reads the per-attempt TRX files the runner leaves under Retries/ and fails the step unless every test that failed an attempt is Passed in the final one. Results are keyed by (assembly, test name), because shared test files are compiled into several assemblies and the names alone collide. The rationale comment claiming the runner's exit code was the retry-aware verdict was rewritten in all seven blocks: it is that only for tests that end up failing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Dimensions are declared by position but bound to a fixture's constructor by enum type, and xUnit's fixture cache is keyed by type as well, so two dimensions of one type cannot be told apart. Before this change the second was dropped: the method expanded to a single variant named for the whole combination, so a declaration reading as four pairings ran once, with the first value used for both arguments, and reported as though it had covered all of them. That is a suite claiming coverage it does not have. On origin/main the merge built its dimensions with Dictionary.Add, so this threw. The rewrite made it a silent overwrite; this restores the refusal at discovery, where it is reported per method as a failing stand-in under each combination the method would have claimed, leaving its siblings alone. Verified by removing the check: the scenario then expands to one variant named "(Sql, Cosmos)" and the three new harness tests go red. Both attributes in the repository - HttpIntegrationFixtureArgumentSets and FhirStorageTestsFixtureArgumentSets - use distinct enum types and are unaffected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
One method carrying fixture argument sets exempts its whole class from the check that every test class is selected by some leg, because that is enough for the framework to expand the class. It expands only that method: a sibling declaring nothing of its own, in a class declaring nothing of its own, is discovered as an ordinary test case with no DataStore trait, and every leg selects positively on that trait. So one decorated method can hide any number of siblings that run nowhere, from the very check written to find tests that run nowhere. No class is in that state today. Verified by putting one there: dropping the class-level attribute from BulkUpdateTests leaves its eleven undecorated methods unreachable, the new check names all eleven, and the class-level check stays green - which is the blind spot itself. Only the E2E copy runs this. The integration legs filter by excluding a store, and an exclusion still selects a test carrying no trait at all, so the same scan there would report tests that do run. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The unit test leg names no projects: it runs whatever '**/*UnitTests/*.csproj' matches. A project that leaves the glob - renamed, moved, or its directory suffix changed - is not reported anywhere. The other nineteen run, all of them pass, and the leg is green with an assembly's tests missing. The floor the other legs use does not fit this shape. They count what a filter discovers; here the loss is a whole project that was never run at all, which no count of the tests that did run can see. --minimum-expected-tests cannot either: it is applied per assembly, so it says nothing about an absent one, and it has to stay at 1 because the runner re-applies it to the smaller pass --retry-failed-tests starts. Twenty projects match today. Adding more is fine; losing one fails the leg before the tests run. Verified both ways against the working tree. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The three helpers that turn a no-result run into something the runner can publish each fell through to doing nothing for an outcome they did not recognise, while the summary beside them counted that same outcome as a failure. The verdict comes from what is published, not from the summary, so another member of the enum would have been counted as failed and reported as nothing at all. Each helper now refuses an outcome it was not written for. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The check that every failed test passed on a later attempt keyed results by their display name, and display names are not identities: a theory whose rows differ only in the case of an argument produces two tests a case-insensitive lookup cannot separate, and this repository already has one. A row that passed then answered for a row that failed and skipped, which is the exact failure this check was written to catch, and the leg went green. Results are now keyed by the identifier the report gives each test, which the retry attempts reuse, together with the assembly it came from. A result with no identifier fails the leg rather than being dropped. The script is covered by tests for the first time, running the real script over reports shaped like the runner's own. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The leg's guard counted the projects its glob matched and failed when the count dropped. A count only notices projects leaving while none arrive: renaming one project and adding another in the same change leaves the count where it was, and the renamed project stops being tested with nothing to show for it. The expected projects are now listed by path and each one is checked, so a project that leaves the glob is named whatever else changed alongside it. Projects that are not listed still run and are reported, since adding tests should not fail a leg. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The discovery floor that notices a leg selecting nothing repeated the store filter that the test run used, so an edit to one copy would leave the floor guarding a selection no longer run. Each leg now defines its filter once and both steps read it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
An assertion over a collection says nothing when the collection is empty, and both of these could be emptied by the very regression they exist to catch: a fault reported as a test that ran, and options never read at all. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Mikael Weaver (mikaelweave)
changed the title
Contributor
Author
|
Superseded by stack #5760, which contains this PR's changes split into three reviewable PRs:
The split is content-preserving, and that is verifiable. The tip of the stack (#5759 at Why three layers and not moreThe migration cannot be split further along the obvious "mechanical syntax vs real change" line, because every mechanical edit is a v3 breaking-API change that will not compile on v2 (
The pipeline YAML rides with layer 2 for a related reason: Where to focus reviewLayer 1 is small on purpose: it isolates the highest-value behavioural fix in the whole migration. On Closing in favour of the stack. The branch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
[RetryFact]/[RetryTheory], and skippable tests — against v3's new discovery and execution APIs, which have no v2 equivalents.Changes overview
The PR touches 293 files (+12,738 / −968), but roughly two-thirds of that is new test code that adds no production behaviour:
src/Microsoft.Health.Extensions.Xunit/— the custom frameworkbuild/jobs/— pipeline YAML + guard scriptssrc/Microsoft.Health.Extensions.Xunit.UnitTests/— new harnesssrc/Microsoft.Health.Extensions.Xunit.TestAssets/— new scenarios.csproj/ props / solution filesWhere to actually look:
CustomXunitTestFrameworkDiscoverer.cs(+1,399),RetryTestCase.cs(+698) andCustomXunitTestFrameworkExecutor.cs(+341) hold nearly all the real logic — v2's discoverer/executor surface no longer exists, so these were rewritten rather than edited.build/jobs/scripts/Assert-RetriedFailuresPassed.ps1(+155) andAssert-UnitTestProjectsDiscovered.ps1(+102) are the new safety nets; the five YAML files mostly wire them in and switch each leg to the MTP runner.Why the file count is high without much to review: 98 of the 108 changed test files have 15 or fewer changed lines and 73 have 5 or fewer — package/using swaps and attribute renames. The two largest,
AssemblyChecker.csandAssemblyValidationsTester.cs, are new shared helpers that assert every test assembly is configured for v3, not per-test edits.Related issues
AB#189958
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)