[thomhurst]

Closes #7359.

TUnit.Engine.Tests took most of the CI time. This makes three changes.

1. Engine tests get a job of their own

RunEngineTestsModule depended on 11 other test modules, so it started last and ran alone for 3-7 minutes at the end of every modularpipeline job (run 40786972231: ubuntu 6.4 min, windows 6.6 min, macOS 3.3 min).

• The module now has its own EngineTests category and no DependsOn list.
• dotnet.yml ignores that category, and a new engine-tests matrix job in aot.yml runs it. That job builds only TUnit.Engine.Tests and the pipeline.
• UploadToNuGetModule drops its DependsOn<RunEngineTestsModule>: it already waits for aot.yml to pass on the commit before it publishes, and that workflow now includes these tests.

2. No --hangdump on child runs

Hangdump runs the test host under a test-host controller, so every child process started twice. Child runs are now bounded with --timeout 10m, as the AOT path already does. Trade-off: a hung child run is cancelled, but no dump is written. The parent run's hangdump only dumps the parent process.

3. TRX written in-process

--report-trx always uses a test-host controller, so it also doubled every child start. With an empty filter locally, a child run took 1.1 s with no extra flags and 1.9 s with --report-trx.

• TUnit.TestProject now has InProcessTrxReporter, registered through a TestingPlatformBuilderHook. It registers nothing unless TUNIT_ENGINE_TESTS_TRX_PATH is set.
• It enables the framework's TRX capability, so TUnit attaches the same Trx* properties it does for --report-trx. It then writes the TRX elements the engine tests read, using the outcome, counter and output rules of the TRX extension 2.5 (TrxReportEngine, TrxTestResultExtractor, testfx tag v4.5.2). The existing TRX parsing and assertions are unchanged.
• The harness reads each TRX from its known path. Before, FileSystemHelpers.FindFile walked the whole repository, including bin/obj, up to six times per child run. The AOT app is also looked up once instead of once per test.
• Limitation: if the test host crashes, the reporter writes no file, and the test fails with "wrote no trx file" instead of a TRX with crash info.
• The F#, VB and external-cancellation tests still use --report-trx. There are only a few of them.

Validation

• Every literal treenode filter in TUnit.Engine.Tests was run against TUnit.TestProject twice, once with --report-trx and once with the in-process reporter, and the two files were compared. The comparison covered the summary outcome, counters, test-definition class names, distinct test ids, each result's name and outcome, which output sections are present, the first message line, and all stdout lines as a multiset.

  • Reflection mode: 202 of 207 filters match. The other 5 are external-cancellation classes that wait for a signal, so both modes timed out.
  • Source-generated mode: 187 of 187 filters match (cancellation filters left out).

• Total child time across the matching filters, with 12 processes running at once:

  Mode	--report-trx	in-process	Saving
  Reflection	792 s	561 s	29%
  Source-generated	773 s	515 s	33%

  This measures the TRX change alone; removing hangdump and the repository walks saves more.

• TUnit.TestProject builds with -p:Aot=true, and the new file adds no IL warnings. The aot.yml publish will confirm.

• As AGENTS.md asks, TUnit.Engine.Tests itself was not run locally; CI runs it.

Note for branch protection

The new jobs are named Engine tests (<os>). If required status checks list the modularpipeline jobs, add these too, or a PR could merge with failing engine tests.

Summary by CodeRabbit

• Tests
  • Reflection-mode engine tests now run on Linux, Windows, and macOS, except for draft pull requests.
  • Test results are recorded and validated, while diagnostic logs and hang dumps are retained when test runs fail.
  • Engine tests run separately from other test suites and publishing tasks, with test execution parallelism capped based on available processors. Publishing waits for the separate engine-test workflow to complete.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-02T01:30:04.364840Z	2dc464a	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: dccdc1be-9b7e-4e5f-ac5c-e7f78cd9c62e

Commits

Reviewing files that changed from the base of the PR and between 7764bbe and 2dc464a.

Files selected for processing: 1

• tools/TUnit.Pipeline/Modules/UploadToNuGetModule.cs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review.

Walkthrough

Reflection-mode engine tests now run in a separate cross-platform workflow job. The test project adds an in-process TRX reporter, and the engine-test harness validates reports by path. The pipeline also updates engine-test category exclusions, dependencies, and parallelism.

Changes: Engine Test CI and TRX Reporting

Layer / File(s)	Summary
In-process TRX reporter tests/TUnit.TestProject/EngineTestSupport/InProcessTrxReporter.cs, tests/TUnit.TestProject/TUnit.TestProject.csproj	A testing-platform hook registers the reporter when TUNIT_ENGINE_TESTS_TRX_PATH is set. The reporter collects test updates and writes TRX results.
Engine-test TRX paths and assertions tests/TUnit.Engine.Tests/InvokableTestBase.cs, tests/TUnit.Engine.Tests/TrxAsserter.cs, tests/TUnit.Engine.Tests/ListTestsFilterTests.cs	The harness supplies a generated TRX path to Reflection and AOT runs and validates results at that path. AOT executable discovery is cached. The Reflection run uses a 10-minute timeout.
Engine-test category and CI workflow tools/TUnit.Pipeline/PipelineCategories.cs, tools/TUnit.Pipeline/Modules/RunEngineTestsModule.cs, tools/TUnit.Pipeline/Modules/UploadToNuGetModule.cs, tools/TUnit.Pipeline/Modules/Abstract/EngineTestsModuleBase.cs, .github/workflows/aot.yml, .github/workflows/dotnet.yml	The pipeline adds the EngineTests category, changes module dependencies, and sets engine-test parallelism to four times the processor count. The AOT workflow runs the category in a separate job, while the modular pipeline excludes it.

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Other · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant GitHubActions
  participant RunEngineTestsModule
  participant InvokableTestBase
  participant TestHost
  participant InProcessTrxReporterHook
  participant TrxAsserter
  GitHubActions->>RunEngineTestsModule: Run EngineTests category
  RunEngineTestsModule->>InvokableTestBase: Execute engine-test run
  InvokableTestBase->>TestHost: Start process with TRX path
  TestHost->>InProcessTrxReporterHook: Register reporter when path is set
  InProcessTrxReporterHook->>TestHost: Write TRX results after the run
  InvokableTestBase->>TrxAsserter: Assert results at supplied path

Loading

Possibly related issues

• thomhurst/TUnit#7359: The changes implement the issue objectives to isolate engine tests in CI, use a timeout for child runs, and replace the child-run TRX reporting flow with an in-process reporter.

Possibly related PRs

• thomhurst/TUnit#7357: Both PRs change CI category allocation in .github/workflows/dotnet.yml.
• thomhurst/TUnit#7296: The in-process reporter consumes test-node updates and serializes results into the TRX file used by engine-test assertions.
• thomhurst/TUnit#7354: Both PRs change engine-test parallelism behavior.

Merge Risk: Minimal · up to 2dc46

No actionable merge-blocking risk remains.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely identifies the main changes: moving engine tests to a separate CI job and removing per-child test-host relaunch overhead.
Linked Issues check		Issue #7359 has three coding objectives. The workflow adds a separate EngineTests matrix job and excludes that category from modularpipeline. InvokableTestBase replaces child --hangdump use wi…
Out of Scope Changes check		The category changes, workflow dependencies, diagnostic uploads, child timeout, in-process reporter, known-path TRX handling, executable lookup cache, and parallelism setting support the CI objective …

Full details: Linked Issues check

Explanation

Issue #7359 has three coding objectives. The workflow adds a separate EngineTests matrix job and excludes that category from modularpipeline. InvokableTestBase replaces child --hangdump use with --timeout 10m. The test project adds an opt-in in-process TRX reporter, and the harness reads the reporter output from a known path while keeping existing TRX assertions. The reported validation covers the matching reflection and source-generated filters. The requirements are implemented at the reviewed head.

Full details: Out of Scope Changes check

Explanation

The category changes, workflow dependencies, diagnostic uploads, child timeout, in-process reporter, known-path TRX handling, executable lookup cache, and parallelism setting support the CI objective in issue #7359. No unrelated change is established by the reviewed scope.

Full details: Docstring Coverage

Explanation

Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 13 functions across 8 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Generate docstrings

• Commit to this branch
• Create a new PR

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit hops where test runs start,
TRX reports now play their part.
Across three runners, checks proceed,
With timeout bounds for runs in need.
The pipeline sorts each category,
And logs record the journey.}

[github-actions]

Review of #7364: the design is sound. It removes the serial tail job and the double process start per child run. I did not run the engine tests (CI only, per AGENTS.md).

Points to consider

1. NuGet publish gate. UploadToNuGetModule no longer depends on RunEngineTestsModule, so a failing engine test is stopped only by aot.yml needing to pass on the commit. Please confirm the engine-tests job is in the set the publisher waits for, across all matrix OSes (fail-fast is false). A short comment in UploadToNuGetModule pointing at aot.yml would stop the gate being loosened later.

2. Lost hang diagnostics. --hangdump is replaced by --timeout 10m, so hung child runs are cancelled without a dump. The engine-tests job still uploads hangdump files, which child runs will no longer produce. Consider an opt-in switch to re-enable --hangdump when investigating.

3. TRX writer drift. InProcessTrxReporter (about 400 lines) copies TrxReportEngine and TrxTestResultExtractor rules from testfx 2.5. After a package upgrade the engine tests could keep passing against the copy while real --report-trx output diverges. I suggest one small test that runs once with the real --report-trx and compares the elements the engine tests read against the in-process output.

4. Crash reporting. The file is written in AfterRunAsync, so a crashed host now yields a missing file instead of a TRX entry. The error message covers it, but this is a behaviour change.

5. Cleanup. TrxAsserter.AssertTrx and its testMode, command and assertionExpression parameters may have no callers left. AssertTrxAtPath still calls ReadTrxWithRetry, while its comment says there is nothing to wait for. Drop one or fix the comment. Also confirm TestResults/engine-tests is gitignored or cleaned.

No blockers. Item 1 is the one to confirm before merge. Good CI win overall.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The latest change appears safe to merge and fixes the missing test waits before package publishing.

Summary

The PR moves reflection-mode engine tests into their own CI jobs and writes child-run TRX files without starting a second test-host process.

• Engine tests run in their own job on three operating systems.
• Engine-test child runs use time limits instead of per-child hang dumps.
• Engine-test child runs write TRX results inside the test process.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  Suites["11 test suites"] --> Upload["UploadToNuGetModule"]
  Reflection["Reflection engine tests"] --> AOTWorkflow["aot.yml succeeds"]
  Native["AOT tests and package checks"] --> AOTWorkflow
  AOTWorkflow --> Wait["WaitForAotWorkflowAsync"]
  Wait --> Upload
  Packages["Build and check packages"] --> Upload

Loading

_{Reviews (3) · Last reviewed commit: "ci: keep the NuGet release waiting for t..." · Reviewed by Greptile}

[github-actions]

Review of PR #7364 (engine tests in their own job, no child hangdump, in-process TRX)

I read the pipeline, workflow and InProcessTrxReporter changes. The direction is sound: engine tests no longer run alone at the end of the modularpipeline job, and child-process startup cost drops. I didn't run anything locally (per AGENTS.md, TUnit.Engine.Tests is left to CI). Nothing blocking, but a few concerns.

Concerns

1. A publish gate now depends on a separate workflow. UploadToNuGetModule drops DependsOn<RunEngineTestsModule> and relies on aot.yml having passed on the commit. That is a convention, not an enforced dependency. If aot.yml is skipped (path filter, draft PR), nothing checks that the engine tests ran. Please confirm the publish path fails closed when aot.yml has no successful run for the commit.

2. InProcessTrxReporter copies TRX extension logic. It re-implements outcome mapping, counters, output and ID rules from TRX extension 2.5, about 400 lines pinned to a testfx tag. It can drift from upstream, and the real --report-trx path is no longer exercised by any engine test. Suggestion: keep one or two engine tests on the real --report-trx, or add a self-check that runs one child both ways and compares the parsed results.

3. A crash looks like a harness failure. The reporter cannot report a crashed test host, so the file is simply missing. Please make TrxAsserter/InvokableTestBase throw a clear message (path, child exit code, output) when the file is absent, instead of a bare FileNotFoundException.

4. Dropping --hangdump on child runs. Acknowledged in the description. A hung child is now cancelled by --timeout 10m with no dump. Consider making the dump opt-in through an env var so it can be switched on when investigating a flake.

5. Small items in the reporter. Check that AfterRunAsync runs after all TestNodeUpdateMessages are consumed, as the TRX extension does with its own finishing handler. storage on UnitTest is set to outputPath, which differs from real TRX (harmless). The .tmp plus File.Move write is good, but confirm each child gets a unique path.

Positives

• The EngineTests category and the dedicated matrix job are a clean separation, and --timeout 10m matches the AOT path.
• The reporter documents why it exists and what it deliberately doesn't do, and TUNIT_ENGINE_TESTS_TRX_PATH keeps it inert for other runs.
• Reading each TRX from a known path instead of walking the repository with FindFile is a good win on its own.

Overall a reasonable CI improvement. I'd like items 1 and 2 addressed or explicitly accepted before merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7764bbe877

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thomhurst]

Dispositions for the two Claude reviews:

• Publish gate (item 1 in both reviews): fixed in 2dc464a. The Greptile and Codex threads were right that dropping DependsOn<RunEngineTestsModule> also dropped the 11 suites it depended on from the release gate. UploadToNuGetModule now depends on them directly. The engine tests stay gated by WaitForAotWorkflowAsync, which fails closed when aot.yml has no successful run for the commit (see its header comment), and a new comment says not to drop that wait.
• No --hangdump on child runs / opt-in switch: follow-up, not blocking. The trade-off is stated in the PR description, and --timeout 10m still bounds hung child runs.
• TRX writer drift: follow-up, not blocking. The F#, VB and external-cancellation tests still use the real --report-trx, so that path is still exercised. The PR's validation also compared both writers across every literal filter.
• Crash shows as a missing file: the harness already fails with "wrote no trx file" (see the PR description), not a bare FileNotFoundException.
• Cleanup nits (AssertTrx callers, ReadTrxWithRetry comment): nit, not blocking.

The modularpipeline (ubuntu-latest) failure on the previous head was the known Aspire flake #7282, unrelated to this PR.

[github-actions]

Review of #7364: engine tests in own job, no hangdump, in-process TRX

I read the diff, InProcessTrxReporter, the harness changes and the new engine-tests job. I didn't run anything or check CI results. I found no blocking correctness bugs. The gain is real: it removes the 3-7 minute serial tail of every modularpipeline job and the double host start per child run.

What looks good

• The reporter is a no-op unless TUNIT_ENGINE_TESTS_TRX_PATH is set, and it writes via a .tmp file and a move, so readers never see a partial file.
• The Lazy AOT app lookup and the per-run unique TRX path replace repo walks.
• UploadToNuGetModule keeps the aot.yml wait, and the comment says plainly that it is the only release gate for the engine tests.

Concerns and suggestions

1. Hand-copied TRX semantics will drift. The outcome, counter and output rules are copied from TrxReport 2.5 (testfx v4.5.2). If that package is bumped, nothing flags the copy as stale. A cheap guard is one parity test that runs a small filter with the real --report-trx and with the in-process reporter, then compares the attributes the engine tests read (outcome, counters, ErrorInfo, StdOut).

2. Crashed and hung runs lose their evidence.

   • The reporter can't write a TRX if the host dies, and --hangdump is gone. Please confirm AssertTrxAtPath reports the child's exit code, stdout and stderr when the file is missing, rather than a bare FileNotFoundException.
   • engine-tests still uploads */hangdump, but child runs no longer produce any, so that step may be dead weight.
   • --timeout 10m per child is twice the old --hangdump-timeout 5m, so a hang takes longer to cancel.

3. The release gate is a single indirect dependency. Please confirm WaitForAotWorkflowAsync checks the engine-tests job's conclusion and not just that an aot.yml run exists. If aot.yml later gets a paths filter or skip condition, the engine tests would silently stop gating releases. A small test or a required-job-name list in code would protect that.

4. Drafts run no engine tests. The engine-tests job skips draft PRs and dotnet.yml now ignores the EngineTests category. This is probably intended, but it is worth stating in the PR description.

5. Minor. The engine-tests job repeats the Dev Drive, long-paths and diagnostics boilerplate from the aot job, so a composite action would keep them in sync. The Write method in the reporter is long, and splitting it into results, definitions and summary writers would make the counters easier to compare with upstream.

Overall this is a solid CI win and the reporter is careful. I'd want the parity test in (1) and confirmation of (2) and (3) before merge.

🤖 Generated with Claude Code