Skip to content

Commit fc35539

Browse files
stmontgomerystmontgomery
authored
Require explicit backtrace and source location when creating and recording issues (#688)
This refactors some of the logic for constructing `SourceContext` and `Issue` instances to require explicit arguments in more places. ### Motivation: We can provide clearer and more relevant details about recorded issues by ensuring that their backtrace and/or source location are accurate. In general, whenever collecting a backtrace, it's better to elide unhelpful frames from the testing library itself so that frames from user code are as close to the top of the call stack as possible. Similarly, whenever an explicit source location is known, it's best to propagate that in any recorded issues which originate at that location. Currently, the `SourceContext` type has default parameter values for both its `backtrace` and `sourceLocation` properties. Having default values for these parameters means that callers elsewhere in the codebase may accidentally not specify a value, thereby not propagating useful information. Or, in the case of backtraces, they may rely on the default value which results in additional, irrelevant frames in the call stack (representing thunks inserted by the compiler for the default value). The initializer for `Issue` also includes a default value for one of its parameters. ### Modifications: - Change `SourceContext.init(...)` to remove the default values for both of its parameters: `backtrace:` and `sourceLocation:`. - Change `Issue.init(...)` to no longer have default values for any of its parameters. - Remove the non-public, static `Issue.record(...)` functions, and change callers to rely on the instance `record(configuration:)` method. This consolidates the usage pattern across the `Testing` module for creating an `Issue` and then recording it. - Change `SkipInfo.init(...)` to remove the default value for its `sourceContext:` parameter. - Add a convenience `init(for:sourceLocation:)` which accepts an `Error` and correctly creates a `SourceContext` for it. - Add deprecated overloads of all SPIs which were modified to remove default parameter values. - Add convenience overloads to `TestingAdditions.swift` which _preserve_ default parameter values, to avoid unnecessary boilerplate in test-only code where it's safe to rely on such default values. ### 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 fdbfff0 commit fc35539

17 files changed

+125
-107
lines changed

‎Sources/Testing/ExitTests/ExitTest.swift

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,9 @@ func callExitTest(
178178
// common issues, however they would constitute a failure of the test
179179
// infrastructure rather than the test itself and perhaps should not cause
180180
// the test to terminate early.
181-
Issue.record(.errorCaught(error), comments: comments(), backtrace: .current(), sourceLocation: sourceLocation, configuration: configuration)
181+
let issue = Issue(kind: .errorCaught(error), comments: comments(), sourceContext: .init(backtrace: .current(), sourceLocation: sourceLocation))
182+
issue.record(configuration: configuration)
183+
182184
return __checkValue(
183185
false,
184186
expression: expression,

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,9 @@ public func __checkValue(
114114
// Ensure the backtrace is captured here so it has fewer extraneous frames
115115
// from the testing framework which aren't relevant to the user.
116116
let backtrace = Backtrace.current()
117-
Issue.record(.expectationFailed(expectation), comments: comments(), backtrace: backtrace, sourceLocation: sourceLocation)
117+
let issue = Issue(kind: .expectationFailed(expectation), comments: comments(), sourceContext: .init(backtrace: backtrace, sourceLocation: sourceLocation))
118+
issue.record()
119+
118120
return .failure(ExpectationFailedError(expectation: expectation))
119121
}
120122

‎Sources/Testing/Issues/Confirmation.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -173,12 +173,12 @@ public func confirmation<R>(
173173
defer {
174174
let actualCount = confirmation.count.rawValue
175175
if !expectedCount.contains(actualCount) {
176-
Issue.record(
177-
expectedCount.issueKind(forActualCount: actualCount),
176+
let issue = Issue(
177+
kind: expectedCount.issueKind(forActualCount: actualCount),
178178
comments: Array(comment),
179-
backtrace: .current(),
180-
sourceLocation: sourceLocation
179+
sourceContext: .init(backtrace: .current(), sourceLocation: sourceLocation)
181180
)
181+
issue.record()
182182
}
183183
}
184184
return try await body(confirmation)

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

Lines changed: 4 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -17,43 +17,6 @@ extension Issue {
1717
@TaskLocal
1818
static var currentKnownIssueMatcher: KnownIssueMatcher?
1919

20-
/// Record a new issue with the specified properties.
21-
///
22-
/// - Parameters:
23-
/// - kind: The kind of issue.
24-
/// - comments: An array of comments describing the issue. This array may be
25-
/// empty.
26-
/// - backtrace: The backtrace of the issue, if available. This value is
27-
/// used to construct an instance of ``SourceContext``.
28-
/// - sourceLocation: The source location of the issue. This value is used
29-
/// to construct an instance of ``SourceContext``.
30-
/// - configuration: The test configuration to use when recording the issue.
31-
/// The default value is ``Configuration/current``.
32-
///
33-
/// - Returns: The issue that was recorded.
34-
@discardableResult
35-
static func record(_ kind: Kind, comments: [Comment], backtrace: Backtrace?, sourceLocation: SourceLocation, configuration: Configuration? = nil) -> Self {
36-
let sourceContext = SourceContext(backtrace: backtrace, sourceLocation: sourceLocation)
37-
return record(kind, comments: comments, sourceContext: sourceContext, configuration: configuration)
38-
}
39-
40-
/// Record a new issue with the specified properties.
41-
///
42-
/// - Parameters:
43-
/// - kind: The kind of issue.
44-
/// - comments: An array of comments describing the issue. This array may be
45-
/// empty.
46-
/// - sourceContext: The source context of the issue.
47-
/// - configuration: The test configuration to use when recording the issue.
48-
/// The default value is ``Configuration/current``.
49-
///
50-
/// - Returns: The issue that was recorded.
51-
@discardableResult
52-
static func record(_ kind: Kind, comments: [Comment], sourceContext: SourceContext, configuration: Configuration? = nil) -> Self {
53-
let issue = Issue(kind: kind, comments: comments, sourceContext: sourceContext)
54-
return issue.record(configuration: configuration)
55-
}
56-
5720
/// Record this issue by wrapping it in an ``Event`` and passing it to the
5821
/// current event handler.
5922
///
@@ -176,13 +139,8 @@ extension Issue {
176139
// condition evaluated to `false`. Those functions record their own issue,
177140
// so we don't need to record another one redundantly.
178141
} catch {
179-
Issue.record(
180-
.errorCaught(error),
181-
comments: [],
182-
backtrace: Backtrace(forFirstThrowOf: error),
183-
sourceLocation: sourceLocation,
184-
configuration: configuration
185-
)
142+
let issue = Issue(for: error, sourceLocation: sourceLocation)
143+
issue.record(configuration: configuration)
186144
return error
187145
}
188146

@@ -222,13 +180,8 @@ extension Issue {
222180
// condition evaluated to `false`. Those functions record their own issue,
223181
// so we don't need to record another one redundantly.
224182
} catch {
225-
Issue.record(
226-
.errorCaught(error),
227-
comments: [],
228-
backtrace: Backtrace(forFirstThrowOf: error),
229-
sourceLocation: sourceLocation,
230-
configuration: configuration
231-
)
183+
let issue = Issue(for: error, sourceLocation: sourceLocation)
184+
issue.record(configuration: configuration)
232185
return error
233186
}
234187

‎Sources/Testing/Issues/Issue.swift

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -108,19 +108,38 @@ public struct Issue: Sendable {
108108
/// - comments: An array of comments describing the issue. This array may be
109109
/// empty.
110110
/// - sourceContext: A ``SourceContext`` indicating where and how this issue
111-
/// occurred. This defaults to a default source context returned by
112-
/// calling ``SourceContext/init(backtrace:sourceLocation:)`` with zero
113-
/// arguments.
111+
/// occurred.
114112
init(
115113
kind: Kind,
116114
comments: [Comment],
117-
sourceContext: SourceContext = .init()
115+
sourceContext: SourceContext
118116
) {
119117
self.kind = kind
120118
self.comments = comments
121119
self.sourceContext = sourceContext
122120
}
123121

122+
/// Initialize an issue instance representing a caught error.
123+
///
124+
/// - Parameters:
125+
/// - error: The error which was caught which this issue is describing.
126+
/// - sourceLocation: The source location of the issue. This value is used
127+
/// to construct an instance of ``SourceContext``.
128+
///
129+
/// The ``sourceContext`` property will have a ``SourceContext/backtrace``
130+
/// property whose value is the backtrace for the first throw of `error`.
131+
init(
132+
for error: any Error,
133+
sourceLocation: SourceLocation? = nil
134+
) {
135+
let sourceContext = SourceContext(backtrace: Backtrace(forFirstThrowOf: error), sourceLocation: sourceLocation)
136+
self.init(
137+
kind: .errorCaught(error),
138+
comments: [],
139+
sourceContext: sourceContext
140+
)
141+
}
142+
124143
/// The error which was associated with this issue, if any.
125144
///
126145
/// The value of this property is non-`nil` when ``kind-swift.property`` is

‎Sources/Testing/Issues/KnownIssue.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,12 +65,12 @@ private func _matchError(_ error: any Error, using issueMatcher: KnownIssueMatch
6565
/// attributed.
6666
private func _handleMiscount(by matchCounter: Locked<Int>, comment: Comment?, sourceLocation: SourceLocation) {
6767
if matchCounter.rawValue == 0 {
68-
Issue.record(
69-
.knownIssueNotRecorded,
68+
let issue = Issue(
69+
kind: .knownIssueNotRecorded,
7070
comments: Array(comment),
71-
backtrace: nil,
72-
sourceLocation: sourceLocation
71+
sourceContext: .init(backtrace: nil, sourceLocation: sourceLocation)
7372
)
73+
issue.record()
7474
}
7575
}
7676

‎Sources/Testing/Running/Runner.Plan.swift

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ extension Runner {
4141
///
4242
/// - Parameters:
4343
/// - skipInfo: A ``SkipInfo`` representing the details of this skip.
44-
indirect case skip(_ skipInfo: SkipInfo = .init())
44+
indirect case skip(_ skipInfo: SkipInfo)
4545

4646
/// The test should record an issue due to a failure during
4747
/// planning.
@@ -241,9 +241,7 @@ extension Runner.Plan {
241241
// If no trait specified that the test should be skipped, but one did
242242
// throw an error, then the action is to record an issue for that error.
243243
if case .run = action, let error = firstCaughtError {
244-
let sourceContext = SourceContext(backtrace: Backtrace(forFirstThrowOf: error))
245-
let issue = Issue(kind: .errorCaught(error), comments: [], sourceContext: sourceContext)
246-
action = .recordIssue(issue)
244+
action = .recordIssue(Issue(for: error))
247245
}
248246

249247
// If the test is still planned to run (i.e. nothing thus far has caused
@@ -257,15 +255,13 @@ extension Runner.Plan {
257255
do {
258256
try await test.evaluateTestCases()
259257
} catch {
260-
let sourceContext = SourceContext(backtrace: Backtrace(forFirstThrowOf: error))
261-
let issue = Issue(kind: .errorCaught(error), comments: [], sourceContext: sourceContext)
262-
action = .recordIssue(issue)
258+
action = .recordIssue(Issue(for: error))
263259
}
264260
}
265261

266262
// If the test is parameterized but has no cases, mark it as skipped.
267263
if case .run = action, let testCases = test.testCases, testCases.first(where: { _ in true }) == nil {
268-
action = .skip(SkipInfo(comment: "No test cases found."))
264+
action = .skip(SkipInfo(comment: "No test cases found.", sourceContext: .init(backtrace: nil, sourceLocation: test.sourceLocation)))
269265
}
270266

271267
actionGraph.updateValue(action, at: keyPath)
@@ -437,3 +433,12 @@ extension Runner.Plan.Action {
437433
}
438434
}
439435
#endif
436+
437+
// MARK: - Deprecated
438+
439+
extension Runner.Plan.Action {
440+
@available(*, deprecated, message: "Use .skip(_:) and pass a SkipInfo explicitly.")
441+
public static func skip() -> Self {
442+
.skip(SkipInfo())
443+
}
444+
}

‎Sources/Testing/Running/Runner.swift

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -340,13 +340,12 @@ extension Runner {
340340
try await testCase.body()
341341
}
342342
} timeoutHandler: { timeLimit in
343-
Issue.record(
344-
.timeLimitExceeded(timeLimitComponents: timeLimit),
343+
let issue = Issue(
344+
kind: .timeLimitExceeded(timeLimitComponents: timeLimit),
345345
comments: [],
346-
backtrace: .current(),
347-
sourceLocation: sourceLocation,
348-
configuration: configuration
346+
sourceContext: .init(backtrace: .current(), sourceLocation: sourceLocation)
349347
)
348+
issue.record(configuration: configuration)
350349
}
351350
}
352351
}

‎Sources/Testing/Running/SkipInfo.swift

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,9 @@ public struct SkipInfo: Sendable {
3333
/// - comment: A user-specified comment describing this skip, if any.
3434
/// Defaults to `nil`.
3535
/// - sourceContext: A source context indicating where this skip occurred.
36-
/// Defaults to a source context returned by calling
37-
/// ``SourceContext/init(backtrace:sourceLocation:)`` with zero arguments.
3836
public init(
3937
comment: Comment? = nil,
40-
sourceContext: SourceContext = .init()
38+
sourceContext: SourceContext
4139
) {
4240
self.comment = comment
4341
self.sourceContext = sourceContext
@@ -55,3 +53,12 @@ extension SkipInfo: Equatable, Hashable {}
5553
// MARK: - Codable
5654

5755
extension SkipInfo: Codable {}
56+
57+
// MARK: - Deprecated
58+
59+
extension SkipInfo {
60+
@available(*, deprecated, message: "Use init(comment:sourceContext:) and pass an explicit SourceContext.")
61+
public init(comment: Comment? = nil) {
62+
self.init(comment: comment, sourceContext: .init(backtrace: .current(), sourceLocation: nil))
63+
}
64+
}

‎Sources/Testing/SourceAttribution/SourceContext.swift

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,10 @@ public struct SourceContext: Sendable {
2626
/// source location.
2727
///
2828
/// - Parameters:
29-
/// - backtrace: The backtrace associated with the new instance. Defaults to
30-
/// the current backtrace (obtained via
31-
/// ``Backtrace/current(maximumAddressCount:)``).
32-
/// - sourceLocation: The source location associated with the new instance.
33-
public init(backtrace: Backtrace? = .current(), sourceLocation: SourceLocation? = nil) {
29+
/// - backtrace: The backtrace associated with the new instance.
30+
/// - sourceLocation: The source location associated with the new instance,
31+
/// if available.
32+
public init(backtrace: Backtrace?, sourceLocation: SourceLocation?) {
3433
self.backtrace = backtrace
3534
self.sourceLocation = sourceLocation
3635
}
@@ -41,3 +40,17 @@ extension SourceContext: Equatable, Hashable {}
4140
// MARK: - Codable
4241

4342
extension SourceContext: Codable {}
43+
44+
// MARK: - Deprecated
45+
46+
extension SourceContext {
47+
@available(*, deprecated, message: "Use init(backtrace:sourceLocation:) and pass both arguments explicitly instead.")
48+
public init(backtrace: Backtrace?) {
49+
self.init(backtrace: backtrace, sourceLocation: nil)
50+
}
51+
52+
@available(*, deprecated, message: "Use init(backtrace:sourceLocation:) and pass both arguments explicitly instead.")
53+
public init(sourceLocation: SourceLocation? = nil) {
54+
self.init(backtrace: nil, sourceLocation: sourceLocation)
55+
}
56+
}

‎Sources/Testing/Test+Macro.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -602,11 +602,11 @@ public func __invokeXCTestCaseMethod<T>(
602602
guard let xcTestCaseClass, isClass(xcTestCaseSubclass, subclassOf: xcTestCaseClass) else {
603603
return false
604604
}
605-
Issue.record(
606-
.apiMisused,
605+
let issue = Issue(
606+
kind: .apiMisused,
607607
comments: ["The @Test attribute cannot be applied to methods on a subclass of XCTestCase."],
608-
backtrace: nil,
609-
sourceLocation: sourceLocation
608+
sourceContext: .init(backtrace: nil, sourceLocation: sourceLocation)
610609
)
610+
issue.record()
611611
return true
612612
}

‎Sources/Testing/Traits/ConditionTrait.swift

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,12 @@ public struct ConditionTrait: TestTrait, SuiteTrait {
9292
}
9393

9494
if !result {
95-
let sourceContext = SourceContext(sourceLocation: sourceLocation)
95+
// We don't need to consider including a backtrace here because it will
96+
// primarily contain frames in the testing library, not user code. If an
97+
// error was thrown by a condition evaluated above, the caller _should_
98+
// attempt to get the backtrace of the caught error when creating an issue
99+
// for it, however.
100+
let sourceContext = SourceContext(backtrace: nil, sourceLocation: sourceLocation)
96101
throw SkipInfo(comment: commentOverride ?? comments.first, sourceContext: sourceContext)
97102
}
98103
}

‎Tests/TestingTests/EventRecorderTests.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ struct EventRecorderTests {
358358

359359
@Test("HumanReadableOutputRecorder counts issues without associated tests")
360360
func humanReadableRecorderCountsIssuesWithoutTests() {
361-
let issue = Issue(kind: .unconditional, comments: [], sourceContext: .init())
361+
let issue = Issue(kind: .unconditional)
362362
let event = Event(.issueRecorded(issue), testID: nil, testCaseID: nil)
363363
let context = Event.Context(test: nil, testCase: nil, configuration: nil)
364364

@@ -373,7 +373,7 @@ struct EventRecorderTests {
373373

374374
@Test("JUnitXMLRecorder counts issues without associated tests")
375375
func junitRecorderCountsIssuesWithoutTests() async throws {
376-
let issue = Issue(kind: .unconditional, comments: [], sourceContext: .init())
376+
let issue = Issue(kind: .unconditional)
377377
let event = Event(.issueRecorded(issue), testID: nil, testCaseID: nil)
378378
let context = Event.Context(test: nil, testCase: nil, configuration: nil)
379379

‎Tests/TestingTests/IssueTests.swift

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1038,7 +1038,7 @@ final class IssueTests: XCTestCase {
10381038

10391039
func testSetSourceLocationProperty() async throws {
10401040
let sourceLocation = SourceLocation(fileID: "A/B", filePath: "", line: 12345, column: 1)
1041-
var issue = Issue(kind: .unconditional, comments: [], sourceContext: .init(sourceLocation: sourceLocation))
1041+
var issue = Issue(kind: .unconditional, sourceContext: .init(sourceLocation: sourceLocation))
10421042

10431043
var issueSourceLocation = try XCTUnwrap(issue.sourceLocation)
10441044
XCTAssertEqual(issueSourceLocation.line, 12345)
@@ -1480,7 +1480,7 @@ struct IssueCodingTests {
14801480
}
14811481

14821482
@Test func errorSnapshot() throws {
1483-
let issue = Issue(kind: .errorCaught(NSError(domain: "Domain", code: 13)), comments: [])
1483+
let issue = Issue(kind: .errorCaught(NSError(domain: "Domain", code: 13)))
14841484
let underlyingError = try #require(issue.error)
14851485

14861486
let issueSnapshot = Issue.Snapshot(snapshotting: issue)
@@ -1501,11 +1501,7 @@ struct IssueCodingTests {
15011501
sourceLocation: sourceLocation
15021502
)
15031503

1504-
let issue = Issue(
1505-
kind: .apiMisused,
1506-
comments: [],
1507-
sourceContext: sourceContext
1508-
)
1504+
let issue = Issue(kind: .apiMisused, sourceContext: sourceContext)
15091505

15101506
let issueSnapshot = Issue.Snapshot(snapshotting: issue)
15111507
#expect(issueSnapshot.sourceContext == sourceContext)
@@ -1525,11 +1521,7 @@ struct IssueCodingTests {
15251521
sourceLocation: initialSourceLocation
15261522
)
15271523

1528-
let issue = Issue(
1529-
kind: .apiMisused,
1530-
comments: [],
1531-
sourceContext: sourceContext
1532-
)
1524+
let issue = Issue(kind: .apiMisused, sourceContext: sourceContext)
15331525

15341526
let updatedSourceLocation = SourceLocation(
15351527
fileID: "fileID2",

0 commit comments

Comments
 (0)