Skip to content

Commit 51f97be

Browse files
stmontgomerystmontgomery
authored
Improve handling of required expectation failures in error matcher closure of #expect { } throws: { } (#305)
This improves the handling of `#require` failures in the `throws: { error in … }` error-matching closure of an `#expect { } throws: { }` expectation, by ensuring the resulting issues are relevant. Take this example: ```swift struct AnotherError: Error {} struct MyError: Error { ... } @test func example() { #expect { throw AnotherError() } throws: { error in let myError = try #require(error as? MyError) return myError.someProperty } } ``` Before this PR, the following issues were recorded: 1. ❌ Expectation failed: (error → AnotherError()) as? (MyError → AnotherError) 2. ❌ Caught error: ExpectationFailedError(expectation: Testing.Expectation(...)) 3. ❌ Expectation failed: a second error "ExpectationFailedError(expectation: Testing.Expectation(...))" was thrown when checking error "AnotherError()" Issue 1 is from the `#require`, and it's correct and appropriate. But issue 2 is unnecessary noise; it's revealing an implementation detail about the underlying error which all `#require` expectations throw when they fail (`ExpectationFailedError`), so this issue ought to be suppressed. Issue 3 is _meant_ to indicate the failure of the overall `#expect { } throws: { }`, but it's phrased in a misleading way which focuses on the internal error mentioned earlier, instead of describing the real problem: that the caught error did not match. After this PR, the following issues are recorded instead: - ❌ Expectation failed: (error → AnotherError()) as? (MyError → AnotherError) - ❌ Expectation failed: unexpected error "AnotherError()" was thrown Along the way, I consolidated some logic into a more widely-shared helper which invokes closures which may throw and translates caught errors into issues. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
1 parent 622429f commit 51f97be

File tree

4 files changed

+120
-52
lines changed

4 files changed

+120
-52
lines changed

‎Sources/Testing/Expectations/ExpectationChecking+Macro.swift

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1002,14 +1002,13 @@ public func __checkClosureCall<R>(
10021002
mismatchExplanationValue = explanation
10031003
} catch {
10041004
expression = expression.capturingRuntimeValues(error)
1005-
do {
1005+
let secondError = Issue.withErrorRecording(at: sourceLocation) {
10061006
errorMatches = try errorMatcher(error)
1007-
if !errorMatches {
1008-
mismatchExplanationValue = mismatchExplanation?(error) ?? "unexpected error \(_description(of: error)) was thrown"
1009-
}
1010-
} catch let secondError {
1011-
Issue.record(.errorCaught(secondError), comments: comments(), backtrace: .current(), sourceLocation: sourceLocation)
1007+
}
1008+
if let secondError {
10121009
mismatchExplanationValue = "a second error \(_description(of: secondError)) was thrown when checking error \(_description(of: error))"
1010+
} else if !errorMatches {
1011+
mismatchExplanationValue = mismatchExplanation?(error) ?? "unexpected error \(_description(of: error)) was thrown"
10131012
}
10141013
}
10151014

@@ -1051,14 +1050,13 @@ public func __checkClosureCall<R>(
10511050
mismatchExplanationValue = explanation
10521051
} catch {
10531052
expression = expression.capturingRuntimeValues(error)
1054-
do {
1053+
let secondError = await Issue.withErrorRecording(at: sourceLocation) {
10551054
errorMatches = try await errorMatcher(error)
1056-
if !errorMatches {
1057-
mismatchExplanationValue = mismatchExplanation?(error) ?? "unexpected error \(_description(of: error)) was thrown"
1058-
}
1059-
} catch let secondError {
1060-
Issue.record(.errorCaught(secondError), comments: comments(), backtrace: .current(), sourceLocation: sourceLocation)
1055+
}
1056+
if let secondError {
10611057
mismatchExplanationValue = "a second error \(_description(of: secondError)) was thrown when checking error \(_description(of: error))"
1058+
} else if !errorMatches {
1059+
mismatchExplanationValue = mismatchExplanation?(error) ?? "unexpected error \(_description(of: error)) was thrown"
10621060
}
10631061
}
10641062

‎Sources/Testing/Issues/Issue+Recording.swift

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,11 @@ extension Issue {
105105
issue.record()
106106
return issue
107107
}
108+
}
109+
110+
// MARK: - Recording issues for errors
108111

112+
extension Issue {
109113
/// Record a new issue when a running test unexpectedly catches an error.
110114
///
111115
/// - Parameters:
@@ -133,8 +137,98 @@ extension Issue {
133137
issue.record()
134138
return issue
135139
}
140+
141+
/// Catch any error thrown from a closure and record it as an issue instead of
142+
/// allowing it to propagate to the caller.
143+
///
144+
/// - Parameters:
145+
/// - sourceLocation: The source location to attribute any caught error to.
146+
/// - configuration: The test configuration to use when recording an issue.
147+
/// The default value is ``Configuration/current``.
148+
/// - body: A closure that might throw an error.
149+
///
150+
/// - Returns: The issue representing the caught error, if any error was
151+
/// caught, otherwise `nil`.
152+
@discardableResult
153+
static func withErrorRecording(
154+
at sourceLocation: SourceLocation,
155+
configuration: Configuration? = nil,
156+
_ body: () throws -> Void
157+
) -> (any Error)? {
158+
// Ensure that we are capturing backtraces for errors before we start
159+
// expecting to see them.
160+
Backtrace.startCachingForThrownErrors()
161+
defer {
162+
Backtrace.flushThrownErrorCache()
163+
}
164+
165+
do {
166+
try body()
167+
} catch is ExpectationFailedError {
168+
// This error is thrown by expectation checking functions to indicate a
169+
// condition evaluated to `false`. Those functions record their own issue,
170+
// so we don't need to record another one redundantly.
171+
} catch {
172+
Issue.record(
173+
.errorCaught(error),
174+
comments: [],
175+
backtrace: Backtrace(forFirstThrowOf: error),
176+
sourceLocation: sourceLocation,
177+
configuration: configuration
178+
)
179+
return error
180+
}
181+
182+
return nil
183+
}
184+
185+
/// Catch any error thrown from an asynchronous closure and record it as an
186+
/// issue instead of allowing it to propagate to the caller.
187+
///
188+
/// - Parameters:
189+
/// - sourceLocation: The source location to attribute any caught error to.
190+
/// - configuration: The test configuration to use when recording an issue.
191+
/// The default value is ``Configuration/current``.
192+
/// - body: An asynchronous closure that might throw an error.
193+
///
194+
/// - Returns: The issue representing the caught error, if any error was
195+
/// caught, otherwise `nil`.
196+
@discardableResult
197+
static func withErrorRecording(
198+
at sourceLocation: SourceLocation,
199+
configuration: Configuration? = nil,
200+
_ body: () async throws -> Void
201+
) async -> (any Error)? {
202+
// Ensure that we are capturing backtraces for errors before we start
203+
// expecting to see them.
204+
Backtrace.startCachingForThrownErrors()
205+
defer {
206+
Backtrace.flushThrownErrorCache()
207+
}
208+
209+
do {
210+
try await body()
211+
} catch is ExpectationFailedError {
212+
// This error is thrown by expectation checking functions to indicate a
213+
// condition evaluated to `false`. Those functions record their own issue,
214+
// so we don't need to record another one redundantly.
215+
} catch {
216+
Issue.record(
217+
.errorCaught(error),
218+
comments: [],
219+
backtrace: Backtrace(forFirstThrowOf: error),
220+
sourceLocation: sourceLocation,
221+
configuration: configuration
222+
)
223+
return error
224+
}
225+
226+
return nil
227+
}
136228
}
137229

230+
// MARK: - Debugging failures
231+
138232
/// A function called by the testing library when a failure occurs.
139233
///
140234
/// Whenever a test failure (specifically, a non-known ``Issue``) is recorded,

‎Sources/Testing/Running/Runner.swift

Lines changed: 2 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -56,42 +56,6 @@ public struct Runner: Sendable {
5656
// MARK: - Running tests
5757

5858
extension Runner {
59-
/// Catch errors thrown from a closure and process them as issues instead of
60-
/// allowing them to propagate to the caller.
61-
///
62-
/// - Parameters:
63-
/// - sourceLocation: The source location to attribute caught errors to.
64-
/// - body: A closure that might throw an error.
65-
///
66-
/// This function encapsulates the standard error handling performed by
67-
/// ``Runner`` when running a test or test case.
68-
private func _withErrorHandling(sourceLocation: SourceLocation, _ body: () async throws -> Void) async {
69-
// Ensure that we are capturing backtraces for errors before we start
70-
// expecting to see them.
71-
Backtrace.startCachingForThrownErrors()
72-
defer {
73-
Backtrace.flushThrownErrorCache()
74-
}
75-
76-
do {
77-
try await body()
78-
79-
} catch is ExpectationFailedError {
80-
// This error is thrown by `__check()` to indicate that its condition
81-
// evaluated to `false`. That function emits its own issue, so we don't
82-
// need to emit one here.
83-
84-
} catch {
85-
Issue.record(
86-
.errorCaught(error),
87-
comments: [],
88-
backtrace: Backtrace(forFirstThrowOf: error),
89-
sourceLocation: sourceLocation,
90-
configuration: configuration
91-
)
92-
}
93-
}
94-
9559
/// Execute the ``CustomExecutionTrait/execute(_:for:testCase:)`` functions
9660
/// associated with the test in a plan step.
9761
///
@@ -239,7 +203,7 @@ extension Runner {
239203

240204
if let step = stepGraph.value, case .run = step.action {
241205
await Test.withCurrent(step.test) {
242-
await _withErrorHandling(sourceLocation: step.test.sourceLocation) {
206+
_ = await Issue.withErrorRecording(at: step.test.sourceLocation, configuration: configuration) {
243207
try await _executeTraits(for: step, testCase: nil) {
244208
// Run the test function at this step (if one is present.)
245209
if let testCases = step.test.testCases {
@@ -308,7 +272,7 @@ extension Runner {
308272

309273
await Test.Case.withCurrent(testCase) {
310274
let sourceLocation = step.test.sourceLocation
311-
await _withErrorHandling(sourceLocation: sourceLocation) {
275+
await Issue.withErrorRecording(at: sourceLocation, configuration: configuration) {
312276
try await withTimeLimit(for: step.test, configuration: configuration) {
313277
try await _executeTraits(for: step, testCase: testCase) {
314278
try await testCase.body()

‎Tests/TestingTests/IssueTests.swift

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -541,7 +541,7 @@ final class IssueTests: XCTestCase {
541541

542542
func testErrorCheckingWithExpect_Mismatching() async throws {
543543
let expectationFailed = expectation(description: "Expectation failed")
544-
expectationFailed.expectedFulfillmentCount = 11
544+
expectationFailed.expectedFulfillmentCount = 13
545545

546546
var configuration = Configuration()
547547
configuration.eventHandler = { event, _ in
@@ -600,6 +600,12 @@ final class IssueTests: XCTestCase {
600600
#expect(throws: MyError.self) {
601601
try nonVoidReturning()
602602
}
603+
#expect {
604+
throw MyError()
605+
} throws: { error in
606+
let parameterizedError = try #require(error as? MyParameterizedError)
607+
return parameterizedError.index == 123
608+
}
603609
}.run(configuration: configuration)
604610

605611
await fulfillment(of: [expectationFailed], timeout: 0.0)
@@ -702,7 +708,7 @@ final class IssueTests: XCTestCase {
702708

703709
func testErrorCheckingWithExpectAsync_Mismatching() async throws {
704710
let expectationFailed = expectation(description: "Expectation failed")
705-
expectationFailed.expectedFulfillmentCount = 11
711+
expectationFailed.expectedFulfillmentCount = 13
706712

707713
var configuration = Configuration()
708714
configuration.eventHandler = { event, _ in
@@ -753,6 +759,12 @@ final class IssueTests: XCTestCase {
753759
await #expect(throws: MyError.self) {
754760
try await nonVoidReturning()
755761
}
762+
await #expect { () async throws in
763+
throw MyError()
764+
} throws: { error in
765+
let parameterizedError = try #require(error as? MyParameterizedError)
766+
return parameterizedError.index == 123
767+
}
756768
}.run(configuration: configuration)
757769

758770
await fulfillment(of: [expectationFailed], timeout: 0.0)

0 commit comments

Comments
 (0)