feat: add async Member() overloads for Task-returning member selectors#5475
feat: add async Member() overloads for Task-returning member selectors#5475
Conversation
There was a problem hiding this comment.
Code Review
Overall this is a well-structured addition that fills a real gap — users shouldn't need to break async member assertions into separate statements. The hoist is also a solid performance fix. A few concerns worth addressing:
Design: not supported
The project's own perf guidelines say to prefer for potentially-sync operations. Code like this is common in TUnit-adjacent code:
public ValueTask<string> ReadAsync() => ...There are no Member(x => x.ReadAsync(), ...) overloads for ValueTask<TMember>. Adding them would be straightforward — same pattern, just change Task<TMember> to ValueTask<TMember> and the mapper lambda becomes return (await compiledSelector(obj)).AsTask() or simply await the ValueTask directly. Omitting this creates a first-class feature gap on day one.
Bug: GetMemberPath drops chained method calls
For x => x.GetFoo().ReadAsync(), the current implementation:
- Sees
MethodCallExpression(ReadAsync), adds"ReadAsync()", sets body =MethodCallExpression(GetFoo) - The
while (body is MemberExpression)loop exits immediately —GetFoo()is lost - Path becomes
"ReadAsync()"instead of"GetFoo().ReadAsync()"
The fix is to extend the loop to handle nested method calls:
while (body is MemberExpression or MethodCallExpression)
{
if (body is MethodCallExpression mc)
{
parts.Insert(0, $"{mc.Method.Name}()");
body = mc.Object;
}
else if (body is MemberExpression me)
{
parts.Insert(0, me.Member.Name);
body = me.Expression;
}
}Design: Recurring code duplication (pre-existing, but this PR adds 3 more copies)
The 12-line pendingAssertion combine-and-return block appears 12 times (9 sync + 3 async) unchanged:
if (pendingAssertion != null && combinerType != null)
{
Assertion<object?> combinedAssertion = combinerType == CombinerType.And
? new CombinedAndAssertion<TObject>(...)
: new CombinedOrAssertion<TObject>(...);
return new MemberAssertionResult<TObject>(parentContext, combinedAssertion);
}
return new MemberAssertionResult<TObject>(parentContext, erasedAssertion);This is a good time to extract a private helper:
private static MemberAssertionResult<TObject> BuildMemberResult<TObject>(
AssertionContext<TObject> context,
Assertion<object?>? pendingAssertion,
CombinerType? combinerType,
Assertion<object?> erasedAssertion)
{
if (pendingAssertion != null && combinerType != null)
{
var combined = combinerType == CombinerType.And
? (Assertion<object?>)new CombinedAndAssertion<TObject>(context, pendingAssertion, erasedAssertion)
: new CombinedOrAssertion<TObject>(context, pendingAssertion, erasedAssertion);
return new MemberAssertionResult<TObject>(context, combined);
}
return new MemberAssertionResult<TObject>(context, erasedAssertion);
}This reduces the risk of divergence if the combining logic ever changes.
Minor: Third overload's doc comment is misleading
/// Note: This overload exists for backward compatibility.This is brand-new async API — there's nothing backward-compatible about it. The comment should say something like "catch-all overload for complex assertion chains where return type inference fails" instead.
Test: Fragile error-message assertion
await Assert.That(exception!.Message).Contains("Both conditions failed");Tying a test to a specific error message string is fragile. If the message wording changes, this test breaks for a non-functional reason. Consider just asserting the exception is non-null, or verify a more structural property of the failure.
The happy-path and chaining tests are thorough and well-organized. The compile hoist is clean and correctly placed. The core async delegation to the existing async overload on is the right approach. Addressing the gap and the chained-method bug would make this production-ready.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
TIP This summary will be updated as you push new changes. Give us feedback
There was a problem hiding this comment.
Code Review
Overall this is a well-structured addition that fills a real gap — users shouldn't need to break async member assertions into separate statements. The Compile() hoist is also a solid performance fix. A few concerns worth addressing:
Design: ValueTask not supported
The project's own perf guidelines say to prefer ValueTask for potentially-sync operations. There are no Member(x => x.ReadAsync(), ...) overloads for ValueTask<TMember>. Adding them would be straightforward — same pattern, just change Task<TMember> to ValueTask<TMember> and await it directly in the mapper. Omitting this creates a first-class feature gap on day one.
Bug: GetMemberPath drops chained method calls
For x => x.GetFoo().ReadAsync(), the current implementation:
- Sees MethodCallExpression(ReadAsync), adds "ReadAsync()", sets body = MethodCallExpression(GetFoo)
- The
while (body is MemberExpression)loop exits immediately — GetFoo() is lost - Path becomes "ReadAsync()" instead of "GetFoo().ReadAsync()"
The fix is to extend the loop to handle nested method calls:
while (body is MemberExpression or MethodCallExpression)
{
if (body is MethodCallExpression mc)
{
parts.Insert(0, $"{mc.Method.Name}()");
body = mc.Object;
}
else if (body is MemberExpression me)
{
parts.Insert(0, me.Member.Name);
body = me.Expression;
}
}Design: Recurring code duplication (pre-existing, but this PR adds 3 more copies)
The combine-and-return block appears 12 times (9 sync + 3 async) unchanged. This is a good time to extract a private helper:
private static MemberAssertionResult<TObject> BuildMemberResult<TObject>(
AssertionContext<TObject> context,
Assertion<object?>? pendingAssertion,
CombinerType? combinerType,
Assertion<object?> erasedAssertion)
{
if (pendingAssertion != null && combinerType != null)
{
var combined = combinerType == CombinerType.And
? (Assertion<object?>)new CombinedAndAssertion<TObject>(context, pendingAssertion, erasedAssertion)
: new CombinedOrAssertion<TObject>(context, pendingAssertion, erasedAssertion);
return new MemberAssertionResult<TObject>(context, combined);
}
return new MemberAssertionResult<TObject>(context, erasedAssertion);
}This reduces divergence risk if the combining logic ever changes.
Minor: Third overload doc comment is misleading
The comment says "This overload exists for backward compatibility" but this is brand-new async API with no backward compatibility concern. Should be "catch-all overload for complex assertion chains where return type inference fails."
Test: Fragile error-message assertion
await Assert.That(exception!.Message).Contains("Both conditions failed");Tying a test to a specific error message string is fragile. If the wording changes, this test fails for a non-functional reason.
The happy-path and chaining tests are thorough and well-organized. The Compile() hoist is clean and correctly placed. Delegating to the existing async Map overload on AssertionContext is the right approach. Addressing the ValueTask gap and the GetMemberPath chained-method bug would make this production-ready.
There was a problem hiding this comment.
Follow-up Review
The minor cleanup commit (renaming compiledSelector → compiled, removing section separators) is a nice tidy-up, but none of the substantive issues flagged in the previous review have been addressed. Summarising what still needs attention:
Still open: Bug in GetMemberPath for chained method calls
The current code is:
if (body is MethodCallExpression methodCall)
{
parts.Add($"{methodCall.Method.Name}()");
body = methodCall.Object;
}
while (body is MemberExpression memberExpr) { ... }For x => x.GetFoo().ReadAsync(), after the if block sets body = methodCall.Object (which is MethodCallExpression(GetFoo)), the while loop exits immediately because GetFoo() is a MethodCallExpression, not a MemberExpression. The resulting path is "ReadAsync()" rather than "GetFoo().ReadAsync()". The fix was outlined in the previous review — merge the if into the while:
while (body is MemberExpression or MethodCallExpression)
{
if (body is MethodCallExpression mc)
{
parts.Insert(0, $"{mc.Method.Name}()");
body = mc.Object;
}
else if (body is MemberExpression me)
{
parts.Insert(0, me.Member.Name);
body = me.Expression;
}
}Still open: ValueTask<TMember> overloads missing
No ValueTask<TMember> overloads were added. This is a functional gap: any async member that returns ValueTask (common for performance-sensitive APIs, and recommended in this codebase's own guidelines) cannot use the new fluent syntax. Same pattern as the Task overloads — just await the ValueTask directly.
Still open: Misleading doc comment on third overload
/// Note: This overload exists for backward compatibility.This is brand-new API; there is no prior version to be compatible with. The comment should reflect what it actually does: a catch-all overload for complex assertion chains where return-type inference would otherwise fail.
Still open: Fragile error-message assertion
await Assert.That(exception!.Message).Contains("Both conditions failed");Asserting on a hard-coded substring of an error message makes the test brittle to rewording. Assert on the exception type and the fact that the assertion failed, not on the specific text.
Still open: Combine-and-return duplication
The 6-line if (pendingAssertion != null)… block still appears 12 times. With 3 new async overloads this PR adds 3 more copies. A private helper BuildMemberResult (proposed in the previous review) would prevent this from growing further.
The core feature — awaiting Task<TMember> and delegating to the existing assertion pipeline — is correct and the compile-hoist is sound. Once the path-parsing bug, ValueTask support, and the minor comment/test issues are addressed, this will be in good shape.
There was a problem hiding this comment.
Third Review Pass
The third commit (781cc9e4) only adds the missing public API snapshot — a needed fix but it doesn't address the substantive issues raised in the two prior reviews. All five open items remain unresolved.
Still open: GetMemberPath bug for chained method calls
The current code after the PR:
if (body is MethodCallExpression methodCall)
{
parts.Add($"{methodCall.Method.Name}()");
body = methodCall.Object;
}
while (body is MemberExpression memberExpr) { ... }For x => x.GetFoo().ReadAsync(), body = methodCall.Object yields another MethodCallExpression. The while loop immediately exits, dropping GetFoo() from the path. The fix (merge the if into the while) has been outlined twice — this is a correctness bug that will silently produce wrong error messages.
Still open: ValueTask<TMember> overloads missing
No ValueTask overloads were added. This codebase recommends ValueTask for potentially-sync paths. Any member returning ValueTask<T> cannot use the new fluent async syntax at all. Since the pattern is identical (just await the ValueTask), adding parity overloads is low-effort and avoids a permanent first-class feature gap.
Still open: Misleading doc comment on third overload
/// Note: This overload exists for backward compatibility.This is brand-new API added in this very PR. The comment is factually wrong. Should describe what it actually does — catch-all when the compiler cannot resolve the return type.
Still open: Fragile error-message assertion
await Assert.That(exception!.Message).Contains("Both conditions failed");Hard-coding error message substrings creates fragile tests. The test should verify that an assertion exception was thrown, not that a specific string is present in the message. Text can be rewarded without a behaviour change, but this test will break.
Still open: Combine-and-return block duplicated 12 times
Three new async overloads added 3 more copies of the same 6-line pattern. A private helper BuildMemberResult<TObject> would avoid future divergence. This is not blocking, but each PR that adds Member() overloads without addressing it makes the refactor harder.
The core feature and compile-hoist are solid. The path-parsing bug and ValueTask gap are the two items most worth resolving before merge. The rest are lower priority but all were raised in the first review and have had two commits to be addressed.
There was a problem hiding this comment.
Fourth Review Pass
Good progress in the latest commit (4df8fe7a). Three of the five previously raised issues have been resolved. Two remain open.
Resolved since last review
GetMemberPath chained method call bug — Fixed. The while loop now correctly handles both MemberExpression and MethodCallExpression in sequence. Expressions like x => x.GetFoo().ReadAsync() will now produce the correct path.
ValueTask overloads — Added. All three variants (TTransformed, TMember, and object catch-all) are now present, giving parity with the Task overloads.
Misleading doc comment — Fixed. The comment now correctly describes the overload as a catch-all, not a backward-compatibility shim.
Still open: Fragile error-message assertion
The test file was not updated. This assertion is still present:
await Assert.That(exception!.Message).Contains("Both conditions failed");Binding a test to a specific substring of an error message makes it fragile — any wording change breaks the test for a non-functional reason. The test should verify that an AssertionException was thrown without asserting on the specific message content, or use a more structural assertion.
Still open: Combine-and-return block duplicated 15 times
The six-line pattern now appears 15 times (9 sync + 3 Task + 3 ValueTask) after this PR — three more copies than the last review. A private helper would prevent this from compounding further with each overload set added:
private static MemberAssertionResult<TObject> BuildMemberResult<TObject>(
AssertionContext<TObject> context,
Assertion<object?>? pendingAssertion,
CombinerType? combinerType,
Assertion<object?> erasedAssertion)
{
if (pendingAssertion != null && combinerType != null)
{
var combined = combinerType == CombinerType.And
? (Assertion<object?>)new CombinedAndAssertion<TObject>(context, pendingAssertion, erasedAssertion)
: new CombinedOrAssertion<TObject>(context, pendingAssertion, erasedAssertion);
return new MemberAssertionResult<TObject>(context, combined);
}
return new MemberAssertionResult<TObject>(context, erasedAssertion);
}The core feature is solid and the fixes to the expression parser and ValueTask parity are the right choices. The duplication concern is the more important of the two remaining items — the helper extraction is low-risk and prevents the pattern from proliferating with future overloads.
There was a problem hiding this comment.
Fifth Review Pass
Good iteration — four of the five previously raised issues have now been resolved.
Resolved since last review
All four items raised in the fourth review have been addressed:
- ** chained method call bug** — Fixed correctly. The loop handles both and .
- ** overloads** — Added with full parity (TTransformed, TMember, and object catch-all variants).
- Misleading doc comment — Corrected on the third overload.
- Fragile error-message assertion — The
Contains("Both conditions failed")assertion is gone;Async_Member_Or_Both_Failnow just asserts the exception is non-null.
Still open: ValueTask overloads don't use BuildMemberResult
The BuildMemberResult helper was extracted and applied to all 9 sync overloads and all 3 Task overloads — but the 3 new ValueTask overloads still inline the combine-and-return pattern:
// ValueTask overloads still do this:
if (pendingAssertion != null && combinerType != null)
{
Assertion<object?> combinedAssertion = combinerType == CombinerType.And
? new CombinedAndAssertion<TObject>(parentContext, pendingAssertion, erasedAssertion)
: new CombinedOrAssertion<TObject>(parentContext, pendingAssertion, erasedAssertion);
return new MemberAssertionResult<TObject>(parentContext, combinedAssertion);
}
return new MemberAssertionResult<TObject>(parentContext, erasedAssertion);While the Task overloads do:
return BuildMemberResult(parentContext, pendingAssertion, combinerType, erasedAssertion);This is the only remaining inconsistency. All three ValueTask overloads should end with return BuildMemberResult(parentContext, pendingAssertion, combinerType, erasedAssertion); to match the sync and Task overloads. The helper is already there — it's just a three-line mechanical change per overload.
The core feature is solid, the expression parser fix is correct, and the full overload matrix (sync/Task/ValueTask × TMember/TTransformed/object) is now complete. Once the ValueTask overloads are wired to BuildMemberResult, this is ready to merge.
There was a problem hiding this comment.
Fifth Review Pass
Good iteration — four of the five previously raised issues have now been resolved.
Resolved since last review
All four items raised in the fourth review have been addressed:
GetMemberPathchained method call bug — Fixed correctly. Thewhileloop handles bothMemberExpressionandMethodCallExpression.ValueTask<TMember>overloads — Added with full parity (TTransformed, TMember, and object catch-all variants).- Misleading doc comment — Corrected on the third overload.
- Fragile error-message assertion — The
Contains("Both conditions failed")assertion is gone;Async_Member_Or_Both_Failnow just asserts the exception is non-null.
Still open: ValueTask overloads don't use BuildMemberResult
The BuildMemberResult helper was extracted and applied to all 9 sync overloads and all 3 Task overloads — but the 3 new ValueTask overloads still inline the combine-and-return pattern:
// ValueTask overloads still do this:
if (pendingAssertion != null && combinerType != null)
{
Assertion<object?> combinedAssertion = combinerType == CombinerType.And
? new CombinedAndAssertion<TObject>(parentContext, pendingAssertion, erasedAssertion)
: new CombinedOrAssertion<TObject>(parentContext, pendingAssertion, erasedAssertion);
return new MemberAssertionResult<TObject>(parentContext, combinedAssertion);
}
return new MemberAssertionResult<TObject>(parentContext, erasedAssertion);While the Task overloads end with:
return BuildMemberResult(parentContext, pendingAssertion, combinerType, erasedAssertion);This is the only remaining inconsistency. All three ValueTask overloads should end with return BuildMemberResult(...) to match the sync and Task overloads. The helper is already there — it's just a one-line change per overload.
The core feature is solid, the expression parser fix is correct, and the full overload matrix (sync / Task / ValueTask x TMember / TTransformed / object) is now complete. Once the ValueTask overloads are wired to BuildMemberResult, this is ready to merge.
There was a problem hiding this comment.
Sixth Review Pass
All five previously raised issues have now been resolved — the helper is correctly applied to all 15 overloads (9 sync + 3 Task + 3 ValueTask). The core feature and the Mocks T.Mock() extension work are solid. Two new concerns in the latest state of the diff:
Missing ConfigureAwait(false) in all 6 async lambdas
All async mapping delegates use:
var memberContext = parentContext.Map<TMember>(async obj =>
{
// ...
return await compiled(obj); // ← missing ConfigureAwait
});Library code that awaits without .ConfigureAwait(false) captures the current SynchronizationContext, which can cause deadlocks in hosts that have one (WPF, WinForms, some test runners). The fix is mechanical:
return await compiled(obj).ConfigureAwait(false);
// or for ValueTask:
return await compiled(obj).ConfigureAwait(false);Both Task<T> and ValueTask<T> support .ConfigureAwait(false) on .NET 6+. Since TUnit targets net8.0;net9.0;net10.0, there's no compatibility concern.
No tests for the ValueTask<TMember> overloads
AsyncMemberTests.cs adds 20 tests, all using Task-returning members. The 3 new ValueTask<TMember> overloads have zero coverage. Adding a ValueTask-returning method to MyObject and two or three parallel tests (success, failure, chaining) would give basic confidence that the ValueTask code path works correctly and that BuildMemberResult chains properly for it.
Overall the PR is in very good shape. These are the only two remaining gaps.
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.30.0 to 1.30.8. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.30.8 <!-- Release notes generated using configuration in .github/release.yml at v1.30.8 --> ## What's Changed ### Other Changes * feat(mocks): migrate to T.Mock() extension syntax by @thomhurst in thomhurst/TUnit#5472 * feat: split TUnit.AspNetCore into Core + meta package by @thomhurst in thomhurst/TUnit#5474 * feat: add async Member() overloads for Task-returning member selectors by @thomhurst in thomhurst/TUnit#5475 ### Dependencies * chore(deps): update aspire to 13.2.2 by @thomhurst in thomhurst/TUnit#5464 * chore(deps): update dependency polyfill to 10.1.1 by @thomhurst in thomhurst/TUnit#5468 * chore(deps): update dependency polyfill to 10.1.1 by @thomhurst in thomhurst/TUnit#5467 * chore(deps): update tunit to 1.30.0 by @thomhurst in thomhurst/TUnit#5469 * chore(deps): update dependency microsoft.playwright to 1.59.0 by @thomhurst in thomhurst/TUnit#5473 **Full Changelog**: thomhurst/TUnit@v1.30.0...v1.30.8 Commits viewable in [compare view](thomhurst/TUnit@v1.30.0...v1.30.8). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
Member()overloads that acceptExpression<Func<TObject, Task<TMember>>>, automatically await the Task, and pass the unwrapped value to the assertion lambda. Users can now writeawait Assert.That(obj).Member(x => x.ReadStringAsync(), str => str.IsEqualTo("expected"))instead of breaking into separate statements.Expression.Compile()outside mapper lambdas in all 9 existing syncMember()overloads to avoid recompilation on every evaluation (particularly impactful underWaitsForpolling).GetMemberPathto handle method call expressions (e.g.ReadStringAsync()) and chained member access (e.g.Foo.BarAsync()).Closes #5470
Test plan
AsyncMemberTests.cscovering:MemberTestspass (no regressions from Compile hoist)MemberNullabilityTestspassMemberCollectionAssertionTestspass