Skip to content

Add test dependencies: [DependsOn] attribute and testconfig.json declarations - #10260

Merged
Evangelink merged 28 commits into
mainfrom
dev/amauryleve/test-ordering-dependencies
Jul 28, 2026
Merged

Add test dependencies: [DependsOn] attribute and testconfig.json declarations#10260
Evangelink merged 28 commits into
mainfrom
dev/amauryleve/test-ordering-dependencies

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Adds test dependencies to MSTest: a test can declare that it runs after one or more other tests, either with a new [DependsOn] attribute or through testconfig.json.

The declarations form a directed acyclic graph, not a flat list. That is the point of the design: when several tests share a prerequisite they all become runnable at the same moment, and the in-assembly scheduler runs them concurrently. A flat ordering attribute would serialize them.

Implements RFC 022. Addresses long-standing requests #25 and #3162.

API

[TestMethod]
public void CreateCart() { }

// Fan-out: both wait for CreateCart, then may run in parallel with each other.
[TestMethod, DependsOn(nameof(CreateCart))] public void AddItem() { }
[TestMethod, DependsOn(nameof(CreateCart))] public void ApplyCoupon() { }

// Fan-in: waits for both.
[TestMethod, DependsOn(nameof(AddItem)), DependsOn(nameof(ApplyCoupon))]
public void PlaceOrder() { }

// Runs even though its prerequisite failed - the audit/cleanup escape hatch.
[TestMethod, DependsOn(nameof(PlaceOrder), ProceedOnFailure = true)]
public void WriteAuditRecord() { }

Overloads cover a method of the same class, every test of another class (typeof(C)), and a specific method of another class (typeof(C), nameof(C.M)). Repeating the attribute is how a test declares several prerequisites — consistent with every other AllowMultiple attribute in MSTest.

The same graph can be declared outside the test source, for orchestrating tests somebody else owns or keeping a pipeline visible in one reviewable place:

{
  "mstest": { "execution": { "dependencies": {
    "chains": [ [ "Contoso.Tests.SetupTests.CreateDatabase",
                  "Contoso.Tests.ImportTests.ImportCatalog" ] ],
    "nodes": [ { "test": "Contoso.Tests.ReportTests.WriteAudit",
                 "dependsOn": [ "Contoso.Tests.CheckoutTests.PlaceOrder",
                                "Contoso.Tests.ImportTests.*" ],
                 "proceedOnFailure": true } ]
  } } }
}

chains is the flat case, nodes the tree case. This part is Microsoft.Testing.Platform only, deliberately: testconfig.json is not supplied to the VSTest entry points, which pass configuration: null. A separate RunSettings-reachable file format was prototyped and dropped — it meant a second syntax, parser and set of diagnostics for the same concept, to serve a host that is being superseded. The attribute works on both hosts.

Semantics

Prerequisite doesn't pass Dependent is skipped, transitively, with a message naming the prerequisite. Not failed — one root cause reads as one failure surrounded by labelled skips. Matches TestNG, TUnit, pytest-dependency, Playwright.
ProceedOnFailure Per-edge opt-out. Merges conservatively: a dependent proceeds only when every edge says it may.
Cycle Configuration error. Reported before anything runs; the tests in the cycle are failed with the cycle path; everything else still runs.
Reference matches nothing Warned and ignored, so --filter and running one test from an IDE keep working.
[DoNotParallelize] prerequisite Dependents are demoted (transitively) into the sequential phase, since that phase runs last.
Data-driven prerequisite Edge targets all rows; the dependent waits for every row and is skipped if any row fails. Per-row matching is documented as not supported.

TestDependencyGraph.Build returns null when no test declares a dependency, so runs that don't use the feature stay on exactly the existing code path.

Review findings fixed in this PR

Two review rounds (correctness, then repo conventions/API) found four issues, all fixed here:

  1. Deadlock (High). An exception in a chunk skipped both the dependent-release and the completion count, so every other worker blocked forever on a semaphore permit that would never be issued and Task.WhenAll never returned — one faulty chunk hung the whole run, and the error handler written for exactly that case was unreachable. Bookkeeping now runs in a finally. The regression test was verified to genuinely hang against the unfixed code.

  2. Nondeterministic silent test loss (High). A cycle that existed only in the class-level projection dropped every chunk edge, on the assumption that the run-time gate would still enforce order. It can't: the gate cannot distinguish "hasn't run yet" from "didn't pass", so dependents were skipped nondeterministically — varying with worker count and thread timing — while the run still exited 0. Those tests are now demoted to the sequential phase, where the topological order holds exactly; only their parallelism is lost, and only for the classes involved.

  3. Class-level self-cycle (Medium). [DependsOn(nameof(Setup))] applied to a class expanded onto Setup itself, failing it as a cycle and cascading a skip over the whole class. The generated self-edge is dropped at discovery; a self-reference written on a method is still kept, since there the user really did name the test they were annotating.

  4. Message severity/conflation (Moderate). The class-level projection notice described a recovered downgrade but was reported as an error, and — because every Errors entry is joined into the message stamped on tests in a real cycle — it contaminated unrelated failures. It is now a warning.

Testing

  • 27 unit tests — 19 graph (edge resolution, fan-out independence, real and projection-only cycles, demotion, unmatched references, ProceedOnFailure merging, ordering determinism, encode/decode) + 8 attribute.
  • 1 scheduler regression test asserting the run completes rather than hangs when a chunk throws.
  • 12 acceptance tests across net462/net8.0/net10.0. The assets record the millisecond each test body entered and left, so the ordering guarantee and the overlap of independent branches are asserted against real timings — the overlap is what distinguishes a DAG from a flat order, so it is checked directly rather than assumed.

Full build.cmd -c Release -pack clean. 5049 + 5810 + 280 existing unit tests pass.

Notes for reviewers

  • No analyzer ships with this feature. Deferred deliberately and tracked in Add analyzer and telemetry support for [DependsOn] #10259, along with the WellKnownTypeNames registration, usage telemetry, and NativeAOT source-generation support — all of which [ResourceLock] shares to varying degrees.
  • One commit re-sorts pre-existing entries in MSTest.TestAdapter/InternalAPI/InternalAPI.Unshipped.txt; that is hygiene, unrelated to the feature, and can be ignored while reviewing.
  • The RFC documents why test dependencies are a poor fit for unit tests and should be reserved for integration/E2E suites, including JUnit 5's reasoned refusal to support them.

Copilot AI added 6 commits July 26, 2026 22:36
Introduce test dependencies to MSTest: a test can declare that it runs after
one or more other tests, either with the new [DependsOn] attribute or through
an XML dependency chain file referenced from runsettings/testconfig.

The declarations form a directed acyclic graph rather than a flat order, which
is the point of the design: tests that share a prerequisite become runnable at
the same moment and the in-assembly scheduler runs them concurrently. The flat
ordering alternative would serialize them.

- Skip, don't fail: a test whose prerequisite did not pass is reported as
  skipped (transitively), so one root cause reads as one failure surrounded by
  labelled skips. ProceedOnFailure is the escape hatch for audit/cleanup tests.
- Cycles are detected before anything runs; the tests in the cycle fail with the
  cycle path and the rest of the run proceeds.
- Test-level edges are projected onto scheduling chunks (class- or method-level).
  A cycle that exists only in the class-level projection is reported with advice
  to use MethodLevel instead of deadlocking.
- A parallelizable test depending on a [DoNotParallelize] test is demoted into
  the sequential phase, transitively, since that phase runs last.
- A dependency matching no test in the run warns and is ignored, so --filter and
  running a single test from an IDE keep working.

TestDependencyGraph.Build returns null when no test declares a dependency, so
runs that do not use the feature stay on exactly the existing code path.

Covered by 18 graph unit tests, 8 attribute unit tests, and acceptance tests
that assert both the ordering and the overlap of independent branches from
recorded timings, across net462/net8.0/net10.0. See RFC 022.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
Replace the separate XML dependency chain file with declarations inline in
testconfig.json under mstest:execution:dependencies.

testconfig.json is supplied only on the Microsoft.Testing.Platform path (the
VSTest entry points pass configuration: null), so configured dependencies are
now an MTP capability. A separate RunSettings-reachable file format was the
alternative; it would have meant a second syntax, parser and set of diagnostics
for the same concept in order to serve a host that is being superseded. The
[DependsOn] attribute still works on both hosts.

The section supports 'chains' (each entry waits for the previous one) and
'nodes' (one test naming several prerequisites, with proceedOnFailure), which
together cover the flat and tree cases the XML format covered.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
The asset's chain is First -> Second -> Third with Verify and Audit hanging off
Second, and Second fails. Third is therefore skipped too - it is downstream of
Second in the chain - so the run is 1 failed, 2 skipped, 2 succeeded, not 1
skipped. Assert the skip reason and that neither skipped test executed its body.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
1. Deadlock when a chunk throws. The worker consumed a permit and dequeued a
   chunk, then an exception from the chunk skipped both the dependent release
   and the completion count - so dependents never became ready, the shutdown
   permits were never issued, and every other worker blocked forever on
   WaitAsync while Task.WhenAll waited for them. One faulty chunk hung the whole
   run, and the error handler written for exactly that case was unreachable.
   The bookkeeping now runs in a finally. Regression test asserts the run
   completes rather than hangs (it deadlocks without the fix).

2. A cycle that existed only in the class-level projection dropped every chunk
   edge, leaving the affected tests unordered. That was worse than losing the
   parallelism: the run-time gate cannot distinguish 'has not run yet' from
   'did not pass', so dependents were skipped nondeterministically - varying
   with worker count and thread timing - while the run still reported success.
   Those tests are now moved into the sequential phase, where the topological
   order is honoured exactly; only their parallelism is lost, and the error
   says how to get it back.

3. A class-level [DependsOn(nameof(Setup))] is expanded onto every method of the
   class, including Setup, which made Setup depend on itself. The graph reported
   a cycle the user never wrote, failed Setup, and cascaded a skip over the whole
   class. That generated self-edge is now dropped at discovery; a self reference
   written directly on a method is still kept, since there the user really did
   name the test they were annotating.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
- The class-level projection notice moves from Errors to Warnings. It describes
  a recovered downgrade - the declared order is still honoured - so reporting it
  at error severity was a poor signal, and because every entry in Errors is
  joined into the failure message stamped on tests caught in a *real* cycle, it
  also contaminated unrelated failures.
- RFC 022 still documented the pre-fix behaviour ('the projected chunk edges are
  dropped'); rewrite the section for demotion, restate the deadlock-freedom
  argument in terms of it, and correct the now-answered unresolved question.
- Purge 'dependency chain file' from public XML docs and test constants; the
  format has been testconfig.json since 91c6b9e.
- Correct the [DoNotParallelize] rationale on the acceptance class: the assets
  are not shared between its test methods. The attribute is still right, for the
  real reason - the overlap assertions need a quiet machine.
- TryParse follows bool + out; resx formatting; RFC test count.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
Copilot AI review requested due to automatic review settings July 27, 2026 11:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds DAG-based MSTest dependencies through [DependsOn] and MTP testconfig.json declarations.

Changes:

  • Adds dependency APIs, configuration parsing, metadata transport, and diagnostics.
  • Introduces graph scheduling, failure propagation, cycle handling, and sequential demotion.
  • Adds API manifests, localization resources, documentation, and extensive tests.
Show a summary per file
File Description
src/TestFramework/TestFramework/Attributes/Lifecycle/DependsOnAttribute.cs Defines the public attribute.
src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt Tracks the new public API.
src/TestFramework/TestFramework/Resources/FrameworkMessages.resx Adds attribute validation text.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.cs.xlf Adds Czech localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.de.xlf Adds German localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.es.xlf Adds Spanish localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.fr.xlf Adds French localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.it.xlf Adds Italian localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ja.xlf Adds Japanese localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ko.xlf Adds Korean localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pl.xlf Adds Polish localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pt-BR.xlf Adds Portuguese localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ru.xlf Adds Russian localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.tr.xlf Adds Turkish localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hans.xlf Adds Simplified Chinese localization source.
src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hant.xlf Adds Traditional Chinese localization source.
src/Adapter/MSTestAdapter.PlatformServices/ObjectModel/TestDependencyInfo.cs Models and encodes dependency edges.
src/Adapter/MSTestAdapter.PlatformServices/ObjectModel/UnitTestElement.cs Carries discovered dependencies.
src/Adapter/MSTestAdapter.PlatformServices/Discovery/TypeEnumerator.cs Reads and merges dependency attributes.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestDependencyDeclaration.cs Applies configuration declarations.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestDependencyGraph.cs Resolves and schedules the DAG.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestDependencyCoordinator.cs Tracks prerequisite outcomes.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Runner.cs Gates and skips dependent tests.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Parallelization.cs Executes dependency-aware chunks.
src/Adapter/MSTestAdapter.PlatformServices/MSTestSettings.cs Stores configured dependencies.
src/Adapter/MSTestAdapter.PlatformServices/MSTestSettings.Configuration.cs Parses dependency configuration.
src/Adapter/MSTestAdapter.PlatformServices/Resources/Resource.resx Adds scheduler diagnostics.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.cs.xlf Adds Czech diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.de.xlf Adds German diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.es.xlf Adds Spanish diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.fr.xlf Adds French diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.it.xlf Adds Italian diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.ja.xlf Adds Japanese diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.ko.xlf Adds Korean diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.pl.xlf Adds Polish diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.pt-BR.xlf Adds Portuguese diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.ru.xlf Adds Russian diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.tr.xlf Adds Turkish diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.zh-Hans.xlf Adds Simplified Chinese diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/Resources/xlf/Resource.zh-Hant.xlf Adds Traditional Chinese diagnostic sources.
src/Adapter/MSTestAdapter.PlatformServices/InternalAPI/InternalAPI.Unshipped.txt Tracks new internal APIs.
src/Adapter/MSTest.TestAdapter/AdapterTestProperties.cs Registers dependency transport metadata.
src/Adapter/MSTest.TestAdapter/Extensions/UnitTestElementExtensions.cs Serializes dependencies to test cases.
src/Adapter/MSTest.TestAdapter/Extensions/TestCaseExtensions.cs Restores dependencies during execution.
src/Adapter/MSTest.TestAdapter/InternalAPI/InternalAPI.Unshipped.txt Tracks adapter internal APIs.
src/Adapter/MSTest.TestAdapter/InternalAPI/uwp/InternalAPI.Unshipped.txt Tracks UWP transport API.
test/UnitTests/TestFramework.UnitTests/Attributes/DependsOnAttributeTests.cs Tests the public attribute contract.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Discovery/TypeEnumeratorTests.cs Tests class-level self-edge handling.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestDependencyGraphTests.cs Tests graph resolution and ordering.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestExecutionManagerTests.cs Tests scheduler exception handling.
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestDependencyExecutionTests.cs Exercises dependencies end to end.
docs/testconfig.schema.json Documents configuration structure.
docs/RFCs/022-Test-Dependencies.md Specifies the feature design.
docs/Changelog.md Announces the feature.

Review details

  • Files reviewed: 54/54 changed files
  • Comments generated: 3
  • Review effort level: Medium

Comment thread src/Adapter/MSTestAdapter.PlatformServices/Discovery/TypeEnumerator.cs Outdated
Comment thread src/TestFramework/TestFramework/Attributes/Lifecycle/DependsOnAttribute.cs Outdated
@Evangelink
Evangelink marked this pull request as ready for review July 27, 2026 15:57
@Evangelink
Evangelink enabled auto-merge (squash) July 27, 2026 15:57
@Evangelink Evangelink added the state/needs-review Awaiting review from the team. label Jul 27, 2026
- Merging the same prerequisite declared twice kept the *permissive*
  ProceedOnFailure, so a class-level opt-out silently overrode a method-level
  default and ran a test whose precondition had failed. That contradicted the
  rule already applied across distinct prerequisites (ResolveEdges) and stated
  in the attribute docs and RFC. Merge conservatively; regression test covers
  the class-plus-method case.
- An empty chain stores nothing under its index, and IConfiguration's indexer
  cannot tell an absent key from a null one, so a stray [] looked like the end
  of the outer array and silently discarded every chain after it. The schema now
  requires at least two entries per chain, and the runtime warns and continues
  past a single empty element instead of truncating.
- The attribute's own XML docs still told consumers a class-level projection
  cycle fails the run; it has demoted to the sequential phase since e913940.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
Copilot AI review requested due to automatic review settings July 27, 2026 16:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 54/54 changed files
  • Comments generated: 2
  • Review effort level: Medium

Comment thread src/Adapter/MSTestAdapter.PlatformServices/Execution/TestDependencyGraph.cs Outdated
@github-actions

This comment has been minimized.

Two remaining conflations in the dependency diagnostics, both making a message
claim more than it should:

- Every broken test was failed with string.Join(graph.Errors), so an assembly
  containing two unrelated cycles stamped both cycle paths onto every failure.
  FindCycles now attributes each cycle's description to that cycle's own members
  and BrokenTests carries it per test, so a failure names the cycle the test is
  actually in. This is the same conflation already removed for the projection
  warning - it was only fixed on one side.

- Kahn's algorithm cannot settle a chunk that is on a cycle *or* merely
  downstream of one, and the class-level warning named the whole unsettled set
  as classes that 'depend on each other' - false for the downstream ones.
  HasChunkCycle now reports the two separately: everything blocked is still
  demoted out of the parallel phase, but only the chunks genuinely in the cycle
  are named.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
Copilot AI review requested due to automatic review settings July 27, 2026 16:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 54/54 changed files
  • Comments generated: 3
  • Review effort level: Medium

Comment thread src/Adapter/MSTestAdapter.PlatformServices/InternalAPI/InternalAPI.Unshipped.txt Outdated
@github-actions

This comment has been minimized.

The test asserted only that no diagnostic was produced, which would still hold
if the graph came back with the wrong chunks - its name claims the declarations
schedule cleanly under MethodLevel, so it should verify that. Now asserts one
chunk per test, an empty sequential phase, and the actual chunk edges.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
Copilot AI review requested due to automatic review settings July 27, 2026 16:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Comments suppressed due to low confidence (2)

src/Adapter/MSTestAdapter.PlatformServices/InternalAPI/InternalAPI.Unshipped.txt:36

  • This baseline still describes the old BrokenTests return type and omits the new BrokenTest nested API and MSTestSettings.DeclaredDependencies. PlatformServices tracks internal members (for example, InternalAPI.Shipped.txt:326-346 tracks MSTestSettings), so the API analyzers will report both stale and undeclared APIs. Update the baseline to match the current source.
Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution.TestDependencyGraph.BrokenTests.get -> System.Collections.Generic.IReadOnlyList<Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel.UnitTestElement!>!

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestDependencyGraph.cs:675

  • Removing blocked vertices with no blocked dependents does not isolate cycle members when two cycles are connected. For class edges A↔B, C→B, D→C, and D↔E, every vertex has a blocked dependent, so C survives even though it is only a bridge and cannot reach itself; the warning incorrectly names it as cyclic. Compute actual strongly connected components for the diagnostic while continuing to demote all blocked chunks.
    /// <summary>
    /// Narrows the blocked chunks down to those actually on a cycle, by repeatedly discarding any chunk that
    /// no other blocked chunk depends on. Such a chunk can reach the cycle but nothing returns to it, so it is
    /// downstream of the cycle rather than part of it; what survives is exactly the chunks that can reach
    /// themselves.
  • Files reviewed: 54/54 changed files
  • Comments generated: 3
  • Review effort level: Medium

Comment thread src/Adapter/MSTestAdapter.PlatformServices/Execution/TestDependencyGraph.cs Outdated
Comment thread src/Adapter/MSTestAdapter.PlatformServices/MSTestSettings.Configuration.cs Outdated
Comment thread src/Adapter/MSTestAdapter.PlatformServices/Resources/Resource.resx Outdated
@github-actions

This comment has been minimized.

…ransport

- A configured dependent of 'Ns.Class.*' expands onto every test of the class,
  including the prerequisite when it lives there too, so 'Ns.Class.* dependsOn
  Ns.Class.Setup' made Setup depend on itself - reported as a cycle, failing
  Setup and skipping the class. Discovery already dropped this generated
  self-edge for a class-level [DependsOn]; the configuration path did not.
- InternalAPI.Unshipped.txt still declared BrokenTests as returning
  IReadOnlyList<UnitTestElement> and omitted the nested BrokenTest type. This
  does not currently fail the build (verified), but the file exists to be an
  accurate record of the internal surface.
- Nothing exercised the VSTest TestProperty transport for dependencies: the
  acceptance assets run on the native MTP path, so a registration or conversion
  regression would silently disable [DependsOn] under VSTest. The new round-trip
  test clears LocalExtensionData first, because ToTestCase caches the element
  there and returning that cached instance would make the test pass without the
  encoded property being read at all - which is what the equivalent ResourceLock
  tests do today. Verified it fails when the decode is removed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
…keeping

ClassCleanupManager's countdown is built from every *selected* test, and end-of-assembly
cleanup is gated on every class reaching zero
(ShouldRunEndOfAssemblyCleanup => _remainingTestCountsByClass.IsEmpty). Tests whose outcome
this feature decides without running them - skipped because a prerequisite did not pass, or
failed because they are in a cycle - never reached UnitTestRunner, so they never decremented
that countdown. The class therefore never completed: its [ClassCleanup] was silently lost,
and with it the whole assembly's [AssemblyCleanup]. Verified: a class whose dependent is
skipped ended the run with ClassCleanupCount == 0, even though the prerequisite had run and
so [ClassInitialize] had executed.

This is the same situation the [TestFilterProvider] feature already solved for dropped tests
- selected, counted, never run - so rather than grow a second implementation, both paths now
call the existing bookkeeping through a new UnitTestRunner.NotifyTestNotRunAsync entry point.
FinishFilteredOutTestAsync is renamed FinishTestThatDidNotRunAsync to match its now-shared
purpose.

Any results produced by cleanup that these calls trigger (only non-empty when a cleanup
fails) are appended to the test's reported results, mirroring the filter path.

Both regression tests were verified to fail without the fix.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
Copilot AI review requested due to automatic review settings July 28, 2026 08:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Comments suppressed due to low confidence (3)

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestDependencyExecutionTests.cs:15

  • This summary still says the recorded timestamps make branch overlap checkable, but overlap is now deliberately verified by the bounded rendezvous; timestamps only verify prerequisite ordering. Align the summary with the non-flaky mechanism described immediately below.
    src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Runner.cs:212
  • When a dependency-skipped test triggers a failing class/assembly cleanup, cleanupResults carries AssociatedUnitTestElement pointing to the last runnable test, but the native MTP recorder ignores that property and converts every result using the test argument passed here. The cleanup failure is therefore published against the skipped dependent (potentially replacing its skipped outcome) instead of the test selected by the cleanup bookkeeping. Dispatch cleanup results using their associated element or teach the native recorder to honor the association, as the VSTest converter already does.
        await SendTestResultsAsync(
            test,
            [TestTools.UnitTesting.TestResult.CreateIgnoredResult(reason), .. cleanupResults],
            now,
            now,
            _testResultRecorder).ConfigureAwait(false);

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestDependencyGraph.cs:309

  • A valid whole-class dependency on a class containing only the dependent reaches this branch after its sole self-match is intentionally removed. matches is nonempty, but added is zero, so the runtime incorrectly warns that the target “matches no test.” Reserve this diagnostic for a genuinely empty lookup; a self-only whole-class dependency is simply a no-op.
                if (added == 0)
                {
                    warnings.Add(string.Format(
                        CultureInfo.CurrentCulture,
                        Resource.DependsOnTargetNotFound,
                        tests[i].TestMethod.FullyQualifiedName,
                        dependency.DescribeTarget()));
                    continue;
                }
  • Files reviewed: 59/59 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

ProceedOnFailure_CanBeSet asserted only the true case, so a property that ignored its
setter and always returned true would have passed. Assert the default first.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 96a6be10-1c72-4887-b7a3-8a93e19805f0
Copilot AI review requested due to automatic review settings July 28, 2026 08:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Comments suppressed due to low confidence (2)

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Parallelization.cs:183

  • This pre-sort runs even when TestDependencyGraph.Build will return null, so enabling OrderTestsByNameInClass now changes ordinary parallel runs that do not use dependencies. In particular, MethodLevel chunks are globally queued by type/method name here, whereas previously the setting sorted only within each chunk in ExecuteTestsWithTestRunnerAsync; this breaks the stated no-dependency fast-path invariant. Apply configured edges first, detect whether a graph is needed, and only pre-sort the graph input.
        if (MSTestSettings.CurrentSettings.OrderTestsByNameInClass && !MSTestSettings.CurrentSettings.RandomizeTestOrder)
        {
            testsToRun =
            [
                .. testsToRun
                    .OrderBy(t => t.TestMethod.HasManagedMethodAndTypeProperties ? t.TestMethod.ManagedTypeName : null, StringComparer.Ordinal)
                    .ThenBy(t => t.TestMethod.HasManagedMethodAndTypeProperties ? t.TestMethod.ManagedMethodName : null, StringComparer.Ordinal),
            ];

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Runner.cs:212

  • When this notification triggers a failing class/assembly cleanup, the returned result has AssociatedUnitTestElement set to the last runnable test, but this call submits every result using the skipped dependent. VSTest corrects that association in TestResultExtensions, while MtpTestResultRecorder ignores it and builds the node from the supplied element, so native MTP reports both the skip and cleanup failure against the skipped test instead of the test that owns the cleanup result. Make the MTP recording path honor AssociatedUnitTestElement (and cover this dependency-skip case).
        await SendTestResultsAsync(
            test,
            [TestTools.UnitTesting.TestResult.CreateIgnoredResult(reason), .. cleanupResults],
            now,
            now,
            _testResultRecorder).ConfigureAwait(false);
  • Files reviewed: 59/59 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Evangelink

Copy link
Copy Markdown
Member Author

Correcting one point in the automated analysis above, since it contradicts the earlier build-failure comment and could lead to the wrong fix landing here:

src/Analyzers/MSTest.Analyzers/Resources.resx has been modified on this branch (likely pulled in via a merge from main)

It hasn't. This branch touches zero files under src/Analyzers, and its Resources.resx is byte-identical to the merge-base. There is no merge from main on the branch either — AzDO builds refs/pull/10260/merge, so it evaluates main's state, not the branch's.

main is red on its own, independently of any PR:

Build Branch Commit Result
1529241 refs/heads/main dd34333 ✅ succeeded
1529333 refs/heads/main 9d25639 ❌ failed
1529348 refs/heads/main 866ed18 ❌ failed
1529393 refs/heads/main 0b55734 ❌ failed

All three failures are the same Resources.fr.xlf is out-of-date error from MSTest.Analyzers.

The specific drift, for whoever picks this up: Resources.resx and Resources.fr.xlf contain the same 211 keys with identical source text and notes, but in different order — first divergence at index 3 (AssemblyInitializeShouldBeValidDescription in the resx vs AssemblyFixtureProviderNotSupportedWithNativeAotDescription in the xlf). XliffTasks requires the xlf ordering to track the resx, which is why a content-level diff looks clean.

So the fix is dotnet msbuild src/Analyzers/MSTest.Analyzers/MSTest.Analyzers.csproj /t:UpdateXlf committed to main, as the earlier analysis said — not to this branch. Deliberately not doing it here: it's maintenance in a project this PR doesn't touch, it would muddy an approved feature diff, and it would conflict with the real fix when it lands. Once main is green, a re-run here should pass; this PR's own code was fully green cross-platform on 45fee98f4 before main broke.

Copilot AI review requested due to automatic review settings July 28, 2026 10:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Comments suppressed due to low confidence (2)

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TestDependencyExecutionTests.cs:15

  • This summary is stale: timestamps now verify ordering only; branch overlap is established by the bounded rendezvous described below. Saying the recorded milliseconds make both claims checkable contradicts the test and could encourage reintroducing the timing-based overlap assertion that was removed for flakiness.
    src/TestFramework/TestFramework/Attributes/Lifecycle/DependsOnAttribute.cs:200
  • This rationale incorrectly describes ProceedOnFailure as opting out of waiting, although ordering is always enforced as stated above. It only opts the dependent into running after a prerequisite has completed unsuccessfully; please phrase it in terms of failure handling so the public API documentation does not contradict itself.
  • Files reviewed: 59/59 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@github-actions

This comment has been minimized.

@Evangelink

Copy link
Copy Markdown
Member Author

Correcting myself on the mechanism, and adding a caution for whoever fixes main.

My earlier claim about ordering was wrong. I said the resx and Resources.fr.xlf held the same 211 keys in a different order, and that this was what XliffTasks objected to. The key-order difference is real, but it is not what the tooling is flagging: I ran the sanctioned command locally on the current branch (which now includes the merge of main), and it does not reorder anything.

What dotnet msbuild src/Analyzers/MSTest.Analyzers/MSTest.Analyzers.csproj /t:UpdateXlf actually produces is a single-unit change — and it is one worth looking at before committing:

   <trans-unit id="GlobalTestFixtureShouldBeValidDescription">
-    <target state="translated">Les méthodes marquées par « [GlobalTestInitialize] » ou « [GlobalTestCleanup] » …
+    <target state="new">Methods marked with '[GlobalTestInitialize]' or '[GlobalTestCleanup]' should follow …

It discards the existing French translation, reverting state="translated" to state="new" with the English source text. So the naive "run UpdateXlf and commit" would turn a translated string back into untranslated English — a localization regression rather than a fix. (For what it's worth, the resx <value> and the xlf <source> and <note> for that unit are byte-identical, so the staleness is not a source-text drift.)

Flagging it because the fix belongs on main and whoever does it should decide deliberately whether to accept that target reset or re-run localization, rather than have it slip in as a side effect.

Two other updates:

  • main was merged into this branch (6403cb0), so the branch is current. .\build.cmd -c Release succeeds locally and the feature's own changes survived the merge intact — including the adjacent RFC 020 filter-provider work from RFC 020: Add composable test execution filter providers #10235, which touches the same UnitTestRunner filter path this PR's cleanup fix builds on.
  • The main breakage is unchanged: refs/heads/main builds 1529333, 1529348 and 1529393 all failed with no PR involved, and no commit since has touched the analyzer resx/xlf.

@github-actions

Copy link
Copy Markdown
Contributor

🔴 Build Failure Analysis

All 6 build legs (Linux/macOS/Windows × Release/Debug) fail with the same error:

'xlf/Resources.fr.xlf' is out-of-date with 'Resources.resx'.
Project: src/Analyzers/MSTest.Analyzers/MSTest.Analyzers.csproj

Root cause

src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlf is stale relative to Resources.resx. This PR does not touch MSTest.Analyzers at all, which means the stale XLF was already present on the base branch before this PR was opened (or was introduced by a recent merge from main into the branch).

Fix

Regenerate the XLF file by running:

dotnet msbuild src/Analyzers/MSTest.Analyzers/MSTest.Analyzers.csproj /t:UpdateXlf

Then commit src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlf (and any other XLF files that were also flagged as out-of-date during that run).

Per repo guidelines: never manually edit .xlf files — always use /t:UpdateXlf to regenerate them.

🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · sonnet46 82 AIC · ⌖ 5.26 AIC · ⊞ 8K · [◷]( · )

Copilot AI review requested due to automatic review settings July 28, 2026 11:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 59/59 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@github-actions

Copy link
Copy Markdown
Contributor

🧪 Test quality grade — PR #10260

No new or modified test methods were identified in the changed regions
of this PR. Nothing to grade.

Re-run with /grade-tests.

🤖 Automated content by GitHub Copilot. Generated by the Grade Tests on PR (on open / sync) workflow. · sonnet46 44.8 AIC · ⌖ 4 AIC · ⊞ 10.3K · [◷]( · )

@Evangelink
Evangelink merged commit a206ac1 into main Jul 28, 2026
27 checks passed
@Evangelink
Evangelink deleted the dev/amauryleve/test-ordering-dependencies branch July 28, 2026 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state/needs-review Awaiting review from the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants