Fix Assert.That: single-pass evaluation (#6690) and consistent method-call display (#6691) by Evangelink · Pull Request #8306 · microsoft/testfx

Evangelink · 2026-05-17T19:27:02Z

Summary

Two related bugs in Assert.That(Expression<Func<bool>>) produced misleading failure messages. Both originate in src/TestFramework/TestFramework/Assertions/Assert.That.cs. Since the fixes overlap heavily on the same MethodCallExpression handling logic, they are addressed in a single PR.

Issue #6690 — Expression evaluated twice

The previous implementation called condition.Compile()() to check the boolean result, then walked the expression tree and called Expression.Lambda(subExpr).Compile().DynamicInvoke() for each interesting sub-expression to build the failure details. For stateful operands the reported value didn't match the value actually compared:

var box = new Box();      // box.GetNumber() returns 1, 2, 3, ... on each call
Assert.That(() => box.GetNumber() == 2);
// Old failure message:  box.GetNumber() = 2  (the *second* call)
// Comparison actually performed: 1 == 2  -> false, message looked like 2 == 2.

Fix

Replaced the two-pass eval-then-walk approach with a single ExpressionVisitor-based rewriter:

AnalyzeExpression walks the tree once to identify capture points and their display names.
CaptureRewriter rewrites each captured sub-expression e at slot i to Block { var t = e; captures[i] = (object)t; t } so each captured value is recorded as a side effect of the single root evaluation.
A sentinel value UnsetCapture distinguishes "not evaluated" (short-circuited / unreached) from "captured null".
For unset captures, only pure operand reads (variables/properties, array indexers and lengths, get_Item/Get indexers — see IsSafeToReevaluate) are re-evaluated lazily so the failure message stays informative for short-circuited cases like name == "x" && obj.Property == y. Arbitrary method calls are not re-evaluated, preserving short-circuit semantics for the user's side-effecting code (the motivating case for Assert.That evaluates expression twice #6690).

Issue #6691 — Inconsistent method-call display names

callExpr.ToString() + regex cleaning produced inconsistent names:

Call site	Before	Expected
Static method, same type	`GetAnimal2()`	`Test1.GetAnimal2()`
Instance method on `this`, same type	`Namespace.Test1.GetAnimal()`	`this.GetAnimal()`
Instance method on local	`zoo.GetAnimal()`	`zoo.GetAnimal()` ✓
Static method on other type	`GetAnimal2()`	`Zoo.GetAnimal2()`

Fix

New GetMethodCallDisplayName constructs display names directly from the MethodCallExpression structure:

Extension methods → receiver.Method(rest)
Static methods → TypeName.Method(args)
Captured-this instance methods → this.Method(args) (detected by IsCapturedThis: a MemberExpression whose field is named <>...__this, or a ConstantExpression whose runtime type exactly matches the declaring type — exact-type rather than IsInstanceOfType avoids mislabeling a base-typed local as this when the asserted method is inherited from a base)
Other instance methods → objectDisplay.Method(args)

Tests

11 new tests added in test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs:

That_ExpressionWithSideEffect_EvaluatesOnlyOnce — explicit call-count check
That_ExpressionWithSideEffect_FailureMessageReflectsFirstEvaluation
That_ExpressionWithSideEffect_AppearingTwice_EvaluatesEachOccurrenceOnce
That_ShortCircuitedExpression_StillReportsBothOperands
That_ShortCircuitedExpression_DoesNotEvaluateUnreachedSideEffect — locks down that short-circuited Counter.GetNumber() is NOT re-run by the fallback path
That_ShortCircuitedPropertyAccess_IsReevaluatedForReporting — locks down the documented IsSafeToReevaluate trade-off for properties
That_InstanceMethodOnSameType_RendersAsThis
That_StaticMethodOnSameType_RendersWithTypeName
That_InstanceMethodOnOtherType_RendersWithObjectName
That_StaticMethodOnOtherType_RendersWithTypeName
That_InstanceMethodOnLocalOfBaseType_DoesNotRenderAsThis — regression guard for the IsCapturedThis exact-type check

Verification

All 750 AssertTests pass (61 existing + 11 new + other Assert categories).
build.cmd succeeds with 0 warnings, 0 errors across netstandard2.0, net462, net8.0, net9.0.
Single-pass design and edge cases (<>4__this detection, ConstantExpression-as-this, inherited-method receivers, property/indexer fallback, short-circuit semantics) were reviewed by an expert-reviewer pass and all findings were addressed.

Notes

No new public API surface; all additions are private helpers in AssertExtensions.
A netstandard2.0-only ReferenceEqualityComparer polyfill is added (#if !NET gated) because System.Collections.Generic.ReferenceEqualityComparer is .NET 5+ only.
The implementation is not AOT-friendly because it uses Expression.Compile() / DynamicInvoke(), but Assert.That already required these on the previous code path so this PR neither adds nor removes that concern.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

…splay Fixes two related issues in MSTest's Assert.That(Expression<Func<bool>>): * Issue #6690: the expression was evaluated twice -- once to determine the boolean result and once per sub-expression for the failure details -- producing misleading values for stateful/side-effecting calls. Replaced the two-pass eval-then-walk approach with a single ExpressionVisitor-based rewriter that records each captured sub-expression's value as a side effect of the single root evaluation. Pure operand reads (variables/properties, array indexers, get_Item/Get) are re-evaluated lazily when short-circuited so the failure message stays informative; arbitrary method calls are intentionally NOT re-evaluated to preserve short-circuit semantics for side-effecting code. * Issue #6691: method-call display names were inconsistent (static methods missed the type prefix; instance methods on \ his\ showed the full namespace). Replaced the ToString-based path with GetMethodCallDisplayName, which builds the name directly from the MethodCallExpression so static methods show \TypeName.Method(...)\, instance methods on captured \ his\`n show \ his.Method(...)\, extension methods show \ eceiver.Method(rest)\, and other instance methods show \object.Method(args)\. Adds 11 new unit tests covering both issues plus regression guards for short-circuit semantics, inherited-method receiver disambiguation, and all four method-call display permutations from #6691. All 750 AssertTests pass and build.cmd succeeds with 0 warnings, 0 errors across netstandard2.0, net462, net8.0, and net9.0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR updates Assert.That(Expression<Func<bool>>) to capture failure details during the assertion’s single evaluation and improves method-call display names in assertion messages.

Changes:

Reworks expression analysis/capture to avoid re-evaluating side-effecting sub-expressions.
Adds method-call display-name handling for static, instance, and extension calls.
Adds regression tests for side effects, short-circuiting, and method-call rendering.

Show a summary per file

File	Description
`src/TestFramework/TestFramework/Assertions/Assert.That.cs`	Implements single-pass capture and method-call display helpers.
`test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs`	Adds regression tests for issues #6690 and #6691.

Copilot's findings

Comments suppressed due to low confidence (1)

test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs:1156

This test also swallows the expected failure without asserting that it happened. If the assertion unexpectedly passes, the test can still pass with CallCount equal to 0, so it does not guard the failing short-circuit scenario it is intended to cover.

        try
        {
            act();
        }
        catch (AssertFailedException)
        {

Files reviewed: 2/2 changed files
Comments generated: 2

Evangelink

Review Summary

The fix for #6690 (double evaluation) and #6691 (inconsistent display names) is architecturally sound. The single-pass CaptureRewriter/AnalysisContext design is the right approach to guarantee expressions are evaluated exactly once while still capturing values for the failure message. The IsSafeToReevaluate heuristic is well-documented, the IsCapturedThis exact-type check is correct (and guarded by a regression test), and the #if !NET ReferenceEqualityComparer polyfill compiles correctly on all TFMs because IEqualityComparer<T> is contravariant (in T). The 11 new tests cover the key scenarios.

Findings

#	Severity	Location	Summary
1	MAJOR	line 124	`lambda.Compile()` (instrumented, heavier lambda) is now called on every `Assert.That()` invocation, including passing ones — the old fast path compiled a simpler `Func<bool>`. Worth a comment at minimum; consider caching per expression-tree site if it becomes a bottleneck.
2	MINOR	line 155	Short-circuit fallback still uses `Expression.Lambda(expr).Compile().DynamicInvoke()` — a typed `Func<object?>` compile avoids reflection boxing.
3	MINOR	line 380	`DeclaringType?.Name` returns only the innermost class name for nested types; `CleanTypeName(dt.FullName ?? dt.Name)` would give richer context with no test regressions.

No public API is added. Localization is not affected. Thread safety, null safety, and cross-TFM compatibility look correct.

Generated by Expert Code Review (on open) for issue #8306 · ● 32.3M

Evangelink · 2026-05-18T07:06:31Z

@copilot address review comments

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot · 2026-05-18T07:14:19Z

@copilot address review comments

Done in 901f64a. I addressed the new review comments on Assert.That (compile-path trade-off comment, typed fallback invocation, and nested static type display).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Aligns with the same fix in PR #8307 / PR #8262 so the assertion message shows the comparer's short type name instead of the full namespaced string. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 3/3 changed files
Comments generated: 3

+            MemberExpression => true,
+            BinaryExpression { NodeType: ExpressionType.ArrayIndex } => true,
+            UnaryExpression { NodeType: ExpressionType.ArrayLength } => true,
+            // Indexer-style method calls are conventionally pure reads; the previous implementation
+            // evaluated them eagerly too. Keep this limited to actual indexers and multidimensional
+            // array reads so arbitrary user-defined `Get(...)` methods are not re-invoked.
+            MethodCallExpression { Method.Name: "get_Item" } => true,
+            MethodCallExpression { Method.Name: "Get", Method.DeclaringType: { } declaringType } when declaringType == typeof(Array) => true,


+            return $"{typeName}.{methodName}({argsStr})";
+        }
+
+        if (IsCapturedThis(callExpr.Object, callExpr.Method.DeclaringType))


        else if (callExpr.Method.Name == "Get" && callExpr.Object is not null && callExpr.Arguments.Count > 0)
        {
            string objectName = GetCleanMemberName(callExpr.Object);
            string indexDisplay = string.Join(", ", callExpr.Arguments.Select(GetIndexArgumentDisplay));
            string indexerDisplay = $"{objectName}[{indexDisplay}]";
-            TryAddExpressionValue(callExpr, indexerDisplay, details);
+            context.AddCapture(callExpr, indexerDisplay);


Evangelink · 2026-05-18T13:17:24Z

@copilot address review comments

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot · 2026-05-18T13:28:07Z

@copilot address review comments

Done in caea834. I addressed the review comments by tightening safe fallback reevaluation to recursively validate child expressions, limiting Get indexer formatting to actual Array.Get/indexers, fixing inherited-instance this rendering, and adding regression tests for all three cases.

Two test failures surfaced on CI for the latest Assert.That changes: 1. `IsArrayGetMethod` only recognized `System.Array.Get` — multidimensional array indexers (`int[,]`) expose runtime-synthesized `Get` methods declared on the array type itself, so the indexer-style display path was bypassed and the failure message rendered `matrix.Get(row, col)` instead of `matrix[row, col]`. Switched the check to look at `Object.Type.IsArray`. 2. `CompilerGeneratedDisplayClassRegex` did not cross nested-type `+` boundaries, so captured locals inside a nested helper class (e.g. `InheritedMethodProbe` inside `AssertTests`) were rendered as `AssertTests+expected` instead of just `expected`. Allowed the regex to walk the full `A+B+<>c__DisplayClass...` prefix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 2/2 changed files
Comments generated: 3

            case BinaryExpression binaryExpr:
-                ExtractVariablesFromExpression(binaryExpr.Left, details, suppressIntermediateValues);
-                ExtractVariablesFromExpression(binaryExpr.Right, details, suppressIntermediateValues);
+                AnalyzeExpression(binaryExpr.Left, context, suppressIntermediateValues);
+                AnalyzeExpression(binaryExpr.Right, context, suppressIntermediateValues);


                }

                break;



+            && callExpr.Method.IsDefined(typeof(ExtensionAttribute), inherit: false)
+            && callExpr.Arguments.Count > 0)
+        {
+            string receiver = GetCleanMemberName(callExpr.Arguments[0]);


Evangelink · 2026-05-18T17:53:09Z

Pushed 9a09645 to fix the two test failures from the latest CI run:

That_IndexExpression_WithMultipleIndices_ExtractsVariablesCorrectly — IsArrayGetMethod was checking Method.DeclaringType == typeof(Array), but multidimensional arrays (int[,]) expose runtime-synthesized Get methods declared on the array type itself, not on System.Array. Switched the check to inspect Object.Type.IsArray so matrix[row, col] renders correctly again.
That_InheritedInstanceMethodOnThis_RendersAsThis — CompilerGeneratedDisplayClassRegex did not cross nested-type + boundaries, so captured locals inside InheritedMethodProbe (nested in AssertTests) were rendered as AssertTests+expected instead of just expected. Updated the regex to walk the full A+B+...+<>c__DisplayClass… prefix.

All 77 Assert.That unit tests pass locally on net8.0 and net9.0.

The MacOS Debug failure on the previous run was a HangDump_DefaultSetting_CreateDump flake (createdump time exceeded the 8s timeout), unrelated to this PR.

Evangelink · 2026-05-19T04:05:25Z

@copilot address review comments

… extension-method this detection Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot · 2026-05-19T04:19:22Z

@copilot address review comments

Done in the latest commit. The three review findings are addressed:

Assignment-style BinaryExpression nodes (=, +=, -=, …) — added IsAssignmentNodeType helper and a guard case in AnalyzeExpression that breaks early, so CaptureRewriter never sees the writable LHS. Regression test: That_ManuallyConstructedAssignExpression_DoesNotThrow.
Unary update nodes (++x, x++, --x, x--) — added IsUnaryUpdateNodeType helper and a symmetric guard case. Regression test: That_ManuallyConstructedPreIncrementAssignExpression_DoesNotThrow.
Extension method receiver IsCapturedThis detection — GetMethodCallDisplayName now calls IsCapturedThis(firstArg, callExpr.Method.GetParameters()[0].ParameterType) for the first argument of extension methods, so Assert.That(() => this.SomeExtension() == x) renders this.SomeExtension() correctly regardless of whether this is a compiler-generated <>4__this field or a ConstantExpression. Regression test: That_ExtensionMethodOnThis_RendersAsThis.

The test type intentionally exposes a public field so Expression.Assign can target it (Expression.Field requires FieldInfo). Suppress SA1401 around the field rather than reshape the type. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 2/2 changed files
Comments generated: 4

Evangelink · 2026-05-19T09:13:34Z

+            // Assignment-style binary nodes (=, +=, -=, …) require writable left-hand operands.
+            // CaptureRewriter would replace the LHS with a Block, making it non-writable and
+            // causing the rewritten lambda to fail to compile. Skip these entirely.
+            case BinaryExpression binaryExpr when IsAssignmentNodeType(binaryExpr.NodeType):
                break;


Addressed in #8352 (follow-up to the merged PR). AnalyzeWritableOperand now recurses into the readable sub-expressions of an assignment LHS (member receiver, index target and arguments) and the RHS is analyzed normally, while the writable storage location itself is still left uncaptured to keep the LHS assignable after CaptureRewriter runs.

Evangelink · 2026-05-19T09:13:37Z

+            // Unary update nodes (++x, x++, --x, x--) require writable operands. CaptureRewriter
+            // would replace the operand with a Block, making it non-writable and causing compilation
+            // of the rewritten lambda to fail. Skip these entirely.
+            case UnaryExpression unaryExpr when IsUnaryUpdateNodeType(unaryExpr.NodeType):
+                break;


Addressed in #8352 (follow-up). The same AnalyzeWritableOperand helper now recurses into the readable receiver chain of a unary-update operand (e.g., the container.Inner part of --container.Inner.Value) so failure details still surface those values, while the writable storage location is left uncaptured.

Evangelink · 2026-05-19T09:13:39Z

+        // Skip Func and Action delegates as they don't provide useful information in assertion failures.
+        // Use the static type so we don't have to evaluate the expression at analysis time.
+        if (IsFuncOrActionType(memberExpr.Type))
        {
            return;
        }



Addressed in #8352 (follow-up). The static-type Func/Action skip in AnalyzeMemberExpression is removed; filtering is now applied uniformly at detail-build time using the runtime value's type, restoring the historical behavior where a null delegate-typed member still surfaces as null in details. A regression test (That_NullDelegateTypedMember_StillAppearsInDetails) locks this in.

Evangelink · 2026-05-19T09:13:41Z

+        // Compile and invoke ONCE.
+        // This intentionally pays the rewrite+compile cost on every Assert.That call (including passing ones)
+        // to guarantee single-pass evaluation for correctness (#6690). If this ever shows up as a hotspot,
+        // we can consider caching the compiled delegate by expression-tree instance.
+        var lambda = Expression.Lambda<Func<object?[], bool>>(rewrittenBody, arrayParam);


Followed up in #8352 with a more precise trade-off comment explaining why reference-keyed plan caching would not help the common inline Assert.That(() => ...) pattern (each call constructs a fresh Expression<Func<bool>> instance), while leaving the door open to add caching later if a workload that re-uses expression trees shows measurable overhead. No caching is implemented.

…ted reference Resets PR #8308 onto current main and re-adds the unique contribution from @jakubjares as a single test plus its helper types. After PR #8307 was merged via #8306 and PR #8358 added the CA1305 fix, the old branch was hopelessly diverged from main. Instead of trying to thread the needle through 16+ source conflicts, this commit takes main's source/test files verbatim and only adds: * That_TwoCallsReturningSameMutatedReference_DeduplicatesInDetails * Shape (helper) * BoxOfShapes (helper) The test pins down the current UX limitation: when two calls return the SAME mutable reference and the comparison is reference-inequality, the cache stores the same reference for both call expressions; by the time details are extracted the object has been mutated, both slots resolve to the same value, and TryAddExpressionValue's same-value dedupe means only one entry surfaces (`Shape: Circle`). Documenting this behavior gives us a regression guard if we ever decide to improve the diagnostic. Replaces the previous `WithMessage("non")` placeholder with the real expected message. All 76 That_* tests pass. Co-authored-by: Jakub Jareš <me@jakubjares.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 17, 2026 19:27

Copilot started reviewing on behalf of Evangelink May 17, 2026 19:27 View session

Copilot AI reviewed May 17, 2026

View reviewed changes

Comment thread test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs Outdated

Comment thread src/TestFramework/TestFramework/Assertions/Assert.That.cs Outdated

Evangelink commented May 17, 2026

View reviewed changes

Comment thread src/TestFramework/TestFramework/Assertions/Assert.That.cs

Comment thread src/TestFramework/TestFramework/Assertions/Assert.That.cs Outdated

Comment thread src/TestFramework/TestFramework/Assertions/Assert.That.cs Outdated

Copilot started work on behalf of Evangelink May 18, 2026 07:06 View session

Address Assert.That review feedback

901f64a

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot finished work on behalf of Evangelink May 18, 2026 07:15

Address Assert.That review feedback

25ced7d

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 18, 2026 10:07

Copilot started reviewing on behalf of Evangelink May 18, 2026 10:09 View session

Fix AreAllDistinct to report comparer short name

7d953d6

Aligns with the same fix in PR #8307 / PR #8262 so the assertion message shows the comparer's short type name instead of the full namespaced string. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI reviewed May 18, 2026

View reviewed changes

Merge branch 'main' into evangelink/fix-6690-6691-assert-that

67027fd

Copilot started work on behalf of Evangelink May 18, 2026 13:17 View session

Fix Assert.That fallback safety and inherited this rendering

caea834

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 18, 2026 13:27

Evangelink review requested due to automatic review settings May 18, 2026 13:27

Copilot finished work on behalf of Evangelink May 18, 2026 13:29

Copilot AI review requested due to automatic review settings May 18, 2026 17:40

Copilot started reviewing on behalf of Evangelink May 18, 2026 17:40 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

Copilot started work on behalf of Evangelink May 19, 2026 04:05 View session

Address review: skip assignment/update nodes in capture analysis, fix…

45f4400

… extension-method this detection Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot finished work on behalf of Evangelink May 19, 2026 04:19

Copilot AI review requested due to automatic review settings May 19, 2026 07:31

Copilot started reviewing on behalf of Evangelink May 19, 2026 07:38 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

Evangelink merged commit 29545ad into main May 19, 2026
21 checks passed

Evangelink deleted the evangelink/fix-6690-6691-assert-that branch May 19, 2026 08:53

Copilot AI mentioned this pull request May 19, 2026

Fix Assert.That to not evaluate expression twice (re-opens #6699) #8307

Merged

This was referenced May 19, 2026

Assert.That: capture readable LHS sub-expressions and restore runtime Func/Action filter #8352

Merged

Fix CA1305 in Assert.That test (int.ToString() needs IFormatProvider) #8358

Merged

Assert.That follow-ups from #8306 review #8359

Closed

Evangelink mentioned this pull request May 19, 2026

Assert.That: additional test cases for object-typed sub-expressions (stacks on #8307) #8308

Merged

Evangelink mentioned this pull request May 19, 2026

Assert.That: restore friendly method-call display from #6691 #8364

Open

Conversation

Evangelink commented May 17, 2026

Summary

Issue #6690 — Expression evaluated twice

Fix

Issue #6691 — Inconsistent method-call display names

Fix

Tests

Verification

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Evangelink left a comment

Choose a reason for hiding this comment

Review Summary

Findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Evangelink commented May 18, 2026

Uh oh!

Copilot AI commented May 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Evangelink commented May 18, 2026

Uh oh!

Copilot AI commented May 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Evangelink commented May 18, 2026

Uh oh!

Evangelink commented May 19, 2026

Uh oh!

Copilot AI commented May 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Evangelink May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Evangelink May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Evangelink May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Evangelink May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants