Conversation
# Conflicts: # src/TestFramework/TestFramework/Assertions/Assert.cs
test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.ScopeTests.cs
Show resolved
Hide resolved
Co-authored-by: Youssef Victor <[email protected]>
test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.ScopeTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This pull request implements soft assertions through a new Assert.Scope() API that allows multiple assertion failures to be collected and reported together, addressing issue #571. When using Assert.Scope(), assertions failures are collected instead of thrown immediately, and all collected failures are reported when the scope is disposed.
Changes:
- Adds
Assert.Scope()experimental API for soft assertion collection - Refactors assertion failure reporting: introduces
ReportAssertFailedfor soft assertions while keepingThrowAssertFailedfor hard assertions (Assert.Fail,Assert.Inconclusive) - Updates all assertion methods to use
ReportAssertFailedexcept for hard assertions and parameter validation - Includes comprehensive RFC documentation explaining design decisions around nullability annotations and postconditions
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/TestFramework/TestFramework/Assertions/AssertScope.cs | Core implementation of assertion scope using AsyncLocal and ConcurrentQueue for thread-safe failure collection |
| src/TestFramework/TestFramework/Assertions/Assert.Scope.cs | Public API for Assert.Scope() marked as experimental |
| src/TestFramework/TestFramework/Assertions/Assert.cs | Adds ReportAssertFailed method with soft assertion support and refactors message formatting |
| src/TestFramework/TestFramework/Assertions/Assert.*.cs | Updates all assertion classes (AreEqual, IsTrue, IsNull, etc.) to use ReportAssertFailed |
| src/TestFramework/TestFramework/Assertions/StringAssert.cs | Updates string assertions to use ReportAssertFailed |
| src/TestFramework/TestFramework/Assertions/CollectionAssert.cs | Updates collection assertions to use ReportAssertFailed |
| src/TestFramework/TestFramework/Resources/FrameworkMessages.resx | Adds new messages for scope failures, removes AssertThatFailedFormat, updates AssertionFailed format |
| src/TestFramework/TestFramework/Resources/xlf/*.xlf | Updates all localization files with new strings marked as needing translation |
| src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt | Adds Assert.Scope() to public API |
| test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.ScopeTests.cs | Comprehensive test coverage for Assert.Scope() functionality |
| test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.That.cs | Updates test expectations for Assert.That message format changes |
| test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.ThrowsExceptionTests.cs | Renames test to match ReportAssertFailed |
| docs/RFCs/011-Soft-Assertions-Nullability-Design.md | Detailed RFC documenting design decisions, nullability annotation handling, and trade-offs |
test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.ScopeTests.cs
Show resolved
Hide resolved
test/UnitTests/TestFramework.UnitTests/Assertions/AssertTests.ScopeTests.cs
Show resolved
Hide resolved
Youssef1313
left a comment
There was a problem hiding this comment.
Approved. But I would love to see a full E2E test
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/TestFramework/TestFramework/Assertions/Assert.cs:235
CheckParameterNotNullnow throws viaCreateAssertFailedExceptiondirectly. This bypassesLaunchDebuggerIfNeeded()and also loses the[StackTraceHidden]behavior provided byThrowAssertFailed, so null-parameter failures will behave differently from other assertion failures. Consider routing this throughThrowAssertFailed(assertionName, finalMessage)(or otherwise ensuring debugger-launch + stacktrace-hiding parity).
internal static void CheckParameterNotNull([NotNull] object? param, string assertionName, string parameterName)
{
if (param is null)
{
string finalMessage = string.Format(CultureInfo.CurrentCulture, FrameworkMessages.NullParameterToAssert, parameterName);
throw CreateAssertFailedException(assertionName, finalMessage);
}
| LaunchDebuggerIfNeeded(); | ||
| AssertFailedException assertionFailedException = CreateAssertFailedException(assertionName, message); | ||
| if (AssertScope.Current is { } scope) | ||
| { |
There was a problem hiding this comment.
When ReportAssertFailed collects an AssertFailedException into the active scope, the exception has not been thrown yet, so it won’t have a meaningful stack trace for the original assertion site. On dispose, the single failure’s stack trace will point at AssertScope.Dispose(), and for multiple failures the inner AssertFailedExceptions in the AggregateException will typically have empty stack traces. Consider capturing/preserving a stack trace at the point of failure (e.g., via ExceptionDispatchInfo.SetCurrentStackTrace when available, or an internal throw/catch to populate the stack) so results/debugging can point to the actual failing assertions.
This issue also appears on line 229 of the same file.
| { | |
| { | |
| try | |
| { | |
| throw assertionFailedException; | |
| } | |
| catch (AssertFailedException ex) | |
| { | |
| assertionFailedException = ex; | |
| } |
| throw singleError; | ||
| // Use ExceptionDispatchInfo to preserve the original stack trace captured at the | ||
| // assertion call site, rather than resetting it to point at Dispose. | ||
| ExceptionDispatchInfo.Capture(errorsSnapshot[0]).Throw(); |
There was a problem hiding this comment.
Is this the right place to capture? Or should it be in AddError instead?
There was a problem hiding this comment.
And how do we handle that for the case of multiple exceptions?
Fixes #571