[thomhurst]

Summary

A dynamic test added at run time with TestContext.AddDynamicTest whose method returns Task or ValueTask passed even when the method threw after its first await.

DynamicTest<T>.TestMethod is an Expression<Action<T>>, so TestRegistry.CreateRuntimeTestInvoker compiled an Action<T> and the returned task was dropped. The invoker now compiles the lambda body into a Func<object, object?> that returns the body's value (boxed, or null for void bodies), and awaits it when it is a Task or ValueTask. A ValueTask<T> body is converted to its Task<T> with AsTask first, since a boxed ValueTask<T> matches neither. The rewrite substitutes the parameter with an ExpressionVisitor instead of building an untyped lambda, so it adds no new RequiresDynamicCode usage. Native AOT keeps no reflection metadata for ValueTask<T>.AsTask by default, so a [DynamicDependency] on ValueTask<>.AsTask keeps the lookup working there (without it, CI's AOT jobs failed with ArgumentNullException from Expression.Call). Synchronous exceptions are no longer wrapped in TargetInvocationException, because the delegate is no longer invoked through reflection.

Also removes the #7204 regression test comment that worked around this bug.

Tests

• New tests/TUnit.TestProject/Bugs/_7249 adds five dynamic tests (async Task, ValueTask and ValueTask<int> each failing after await, async passing, sync passing). It has no EngineTest property because it contains intentional failures.
• New tests/TUnit.Engine.Tests/Bug7249AsyncRuntimeDynamicTests asserts 6 total, 3 passed, 3 failed with the three expected failing names.

Local runs (net10.0, Release):

• /*/TUnit.TestProject.Bugs._7249/*/*: 6 total, 3 failed (the expected ones), in both source-generated and --reflection modes. Without the fix: 5 total, 0 failed (reproduces the bug).
• /*/*DynamicTests/*/*: 29/29 passed in both modes.
• /*/TUnit.TestProject.Bugs._7204/*/*: 6/6 passed in both modes.
• Native AOT (win-x64, published like PublishTestProjectAOTModule): /*/TUnit.TestProject.Bugs._7249/*/* gives 6 total, 3 failed, the three expected names. Without the [DynamicDependency], a NativeAOT probe reproduced CI's ArgumentNullException (Parameter 'method').
• TUnit.Engine.Tests builds; not run locally per AGENTS.md, left to CI.

Closes #7249

Summary by CodeRabbit

• Bug Fixes
  • Fixed dynamic tests so failures that occur after awaiting Task, ValueTask, or ValueTask<T> are reported correctly.
• Tests
  • Added coverage for asynchronous and synchronous dynamic tests, including verification of reported pass and failure counts.

[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: 1c4eeb49-ed1c-4e8e-a0f4-54b5637822b5

Commits

Reviewing files that changed from the base of the PR and between 7e579da and dc0ded5.

Files selected for processing: 4

• src/TUnit.Engine/Services/TestRegistry.cs
• tests/TUnit.Engine.Tests/Bug7249AsyncRuntimeDynamicTests.cs
• tests/TUnit.TestProject/Bugs/_7204/SharedInstanceAfterDisposalTests.cs
• tests/TUnit.TestProject/Bugs/_7249/AsyncRuntimeDynamicTests.cs

Files with no reviewable changes: 1

• tests/TUnit.TestProject/Bugs/_7204/SharedInstanceAfterDisposalTests.cs

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

Walkthrough

The runtime dynamic-test invoker now compiles delegates that return test results and awaits returned Task and ValueTask values. Regression tests cover passing and failing synchronous and asynchronous dynamic tests.

Changes: Runtime dynamic test invocation

Layer / File(s)	Summary
Compile and await runtime test results src/TUnit.Engine/Services/TestRegistry.cs	The runtime invoker compiles an object-returning delegate and awaits returned Task and ValueTask values. It also validates that the lambda has one parameter.
Verify asynchronous dynamic test results tests/TUnit.TestProject/Bugs/_7249/AsyncRuntimeDynamicTests.cs, tests/TUnit.Engine.Tests/Bug7249AsyncRuntimeDynamicTests.cs, tests/TUnit.TestProject/Bugs/_7204/SharedInstanceAfterDisposalTests.cs	The tests register passing and failing synchronous and asynchronous dynamic methods. The engine test checks that six results include three failures with the expected asynchronous test names. A comment about keeping a test method synchronous was removed; the method itself remains unchanged.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related PRs

• thomhurst/TUnit#7248: Its test changes document the discarded-task behavior addressed by this PR.
• thomhurst/TUnit#7296: It handles unobserved task faults in session results; this PR instead awaits the runtime dynamic-test task and reports its failure as a test result.

Merge Risk: Minimal · up to dc0de

No actionable issue is established that would prevent merging after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 33.33% 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 summarizes the main change: the engine now awaits results from async runtime dynamic tests.
Linked Issues check		Issue #7249 requires runtime tests added with AddDynamicTest to report failures raised after await. TestRegistry.CreateRuntimeTestInvoker now compiles the lambda body to return its value, awaits Task …
Out of Scope Changes check		The changes support issue #7249. The Native AOT dependency supports the ValueTask invocation path. The engine and test-project changes add regression coverage. Removing the #7204 comment removes a …

Full details: Linked Issues check

Explanation

Issue #7249 requires runtime tests added with AddDynamicTest to report failures raised after await. TestRegistry.CreateRuntimeTestInvoker now compiles the lambda body to return its value, awaits Task and ValueTask results, and converts ValueTask<T> to Task<T> before awaiting. The new regression tests cover failing Task, ValueTask, and ValueTask<T> methods, plus passing async and synchronous methods. The engine test checks the expected six-test result and failure names.

Full details: Out of Scope Changes check

Explanation

The changes support issue #7249. The Native AOT dependency supports the ValueTask<T> invocation path. The engine and test-project changes add regression coverage. Removing the #7204 comment removes a workaround note made obsolete by this fix. No unrelated changes are present in the reviewed diff.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% 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 3 functions across 3 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 tests a task at night,
It waits until the work is right.
A ValueTask gets watched as well,
And failed tests ring a clearer bell.
The sync tests hop along their way,
While async errors will not stray.}

[thomhurst]

@codex review

[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-02T08:40:15.202437Z	dc0ded5	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.

[github-actions]

Review

The change looks correct. I found nothing that needs fixing.

• Root cause: Expression<Action<T>> compiled to Action<T> drops the body's value. The new code re-types the body to Func<object, object?>, which keeps the Task or ValueTask so the invoker can await it. The compiler-generated lambda body keeps its Task type even under an Action<T> delegate type, so this works.
• Parameter substitution: ParameterReplacer also rewrites references inside nested lambdas, which is what you want. void bodies return null, and value-typed bodies are boxed with Convert.
• Dropped reflection: The invoker no longer goes through MethodInfo.Invoke. That removes the TargetInvocationException wrapping, and it is a small perf and AOT-annotation improvement too.
• Tests: They cover Task and ValueTask failing after an await, plus the passing async and sync cases. The Engine test has exact counts and names. Removing the old workaround comment from the Shared data-source instances are reused after disposal by tests registered later in the session #7204 test is appropriate.

Optional nits:

• Compile the invoker once per test rather than per invocation. It is already compiled once in CreateRuntimeTestInvoker, so this is fine as is.
• A source-generated TestMetadata test in AOT mode is out of scope here, since Compile() was already annotated as unsupported there.

LGTM.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: c5fa47735c

ℹ️ 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".

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no blocking issue remains.

Summary

Runtime dynamic tests now wait for returned async work instead of dropping it.

• Runtime-added async tests report failures after they await.

_{Reviews (3) · Last reviewed commit: "fix(engine): keep ValueTask<T>.AsTask me..." · Reviewed by Greptile}

[thomhurst]

@codex review

[github-actions]

Review

The fix is sound. Re-typing the lambda body to return a boxed value, and awaiting it when it is a Task or ValueTask, addresses the root cause: the Action<T> compile was dropping the task. I read the diff only and did not run the tests.

What I checked:

• ValueTask<T>: converting to Task<T> with AsTask is needed, because a boxed ValueTask<T> matches neither Task nor ValueTask. Calling AsTask on the closed type avoids runtime generic instantiation.
• ParameterReplacer: it also rewrites uses of the parameter inside nested lambdas in the body, and it avoids new RequiresDynamicCode usage.
• Exception behaviour: dropping the reflection Invoke removes the TargetInvocationException wrapping. That is a small behaviour change, and the PR calls it out.
• Tests: the new tests cover all three async return shapes, plus the passing async and sync cases. The Engine test asserts the exact failing names.

Minor, non-blocking:

• Compiling a Func<object, object?> per test is more work than before, but it replaces a reflection Invoke per call and still happens once per dynamic test. This is acceptable.
• If a test class is a struct, the Convert unboxes into a copy. That seems unlikely to matter for test classes.

LGTM.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: be285a940e

ℹ️ 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".

[github-actions]

Review

The fix looks correct and well-scoped. I found no blocking issues.

• Root cause: the Expression<Action<T>> lambda was compiled as an Action<T>, so the returned task was discarded. Re-typing the body to return a boxed value and awaiting it fixes that.
• ValueTask<T>: converting it with AsTask before boxing is the right call, because a boxed ValueTask<T> matches neither Task nor ValueTask. Using the closed type's own AsTask also avoids runtime generic instantiation, which keeps the AOT surface unchanged.
• ParameterReplacer: substituting the parameter in the body is safe, including for nested lambdas, since ExpressionVisitor recurses into them. Using a typed Func<object, object?> instead of reflection Invoke also removes the TargetInvocationException wrapping and a per-call reflection hop. That is a small performance gain and gives cleaner exception stacks.
• Tests: the TestProject cases cover Task, ValueTask and ValueTask<int> failing after an await, plus async and sync passing cases. The Engine test checks the exact failing names. Removing the workaround comment from the Shared data-source instances are reused after disposal by tests registered later in the session #7204 test is appropriate.

Non-blocking suggestions:

1. Add a test where the dynamic test method is on a class with a different instance type (for example a derived or generic class). This would exercise the Convert(instanceParameter, testClassParameter.Type) path.
2. ValueTask<T> is the only wrapper case handled specially. Custom awaitables were not handled before and still are not, which is acceptable. A short comment saying so would help future readers.

Overall this is a good fix. The CI TUnit.Engine.Tests run is the remaining verification.

[github-actions]

Review

The change looks correct and well-scoped.

• Root cause fixed at the right layer. Compiling the lambda body into a Func<object, object?> keeps the Task/ValueTask return value. This removes the reflection Invoke call, which is also a small hot-path win. Dropping TargetInvocationException wrapping is an improvement.
• ValueTask<T>. Converting with AsTask on the closed type avoids a boxed ValueTask<T> slipping past both the Task and ValueTask checks. The [DynamicDependency] for AOT is justified, and the PR description says CI's AOT jobs reproduced the failure without it.
• ParameterReplacer. This is cleaner than building an untyped lambda and adds no new RequiresDynamicCode usage.
• Tests. The TestProject cases cover all three async return shapes plus passing sync and async cases. The Engine test asserts the exact failing names, and the intentional failures are isolated without an EngineTest property, as required.

Minor, non-blocking suggestions:

1. The [RequiresUnreferencedCode] attribute has a comment between it and [DynamicDependency]. Grouping the attributes first and keeping the comments above them would read better.
2. If TestMethod ever supports non-single-parameter lambdas, the Parameters.Count != 1 guard gives a clear error. Good defensive choice.

Nothing blocking. LGTM.