Skip to content

Require explicit backtrace and source location when creating and recording issues #688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Sources/Testing/ExitTests/ExitTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,9 @@ func callExitTest(
// common issues, however they would constitute a failure of the test
// infrastructure rather than the test itself and perhaps should not cause
// the test to terminate early.
Issue.record(.errorCaught(error), comments: comments(), backtrace: .current(), sourceLocation: sourceLocation, configuration: configuration)
let issue = Issue(kind: .errorCaught(error), comments: comments(), sourceContext: .init(backtrace: .current(), sourceLocation: sourceLocation))
issue.record(configuration: configuration)

return __checkValue(
false,
expression: expression,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,9 @@ public func __checkValue(
// Ensure the backtrace is captured here so it has fewer extraneous frames
// from the testing framework which aren't relevant to the user.
let backtrace = Backtrace.current()
Issue.record(.expectationFailed(expectation), comments: comments(), backtrace: backtrace, sourceLocation: sourceLocation)
let issue = Issue(kind: .expectationFailed(expectation), comments: comments(), sourceContext: .init(backtrace: backtrace, sourceLocation: sourceLocation))
issue.record()

return .failure(ExpectationFailedError(expectation: expectation))
}

Expand Down
8 changes: 4 additions & 4 deletions Sources/Testing/Issues/Confirmation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,12 @@ public func confirmation<R>(
defer {
let actualCount = confirmation.count.rawValue
if !expectedCount.contains(actualCount) {
Issue.record(
expectedCount.issueKind(forActualCount: actualCount),
let issue = Issue(
kind: expectedCount.issueKind(forActualCount: actualCount),
comments: Array(comment),
backtrace: .current(),
sourceLocation: sourceLocation
sourceContext: .init(backtrace: .current(), sourceLocation: sourceLocation)
)
issue.record()
}
}
return try await body(confirmation)
Expand Down
55 changes: 4 additions & 51 deletions Sources/Testing/Issues/Issue+Recording.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,43 +17,6 @@ extension Issue {
@TaskLocal
static var currentKnownIssueMatcher: KnownIssueMatcher?

/// Record a new issue with the specified properties.
///
/// - Parameters:
/// - kind: The kind of issue.
/// - comments: An array of comments describing the issue. This array may be
/// empty.
/// - backtrace: The backtrace of the issue, if available. This value is
/// used to construct an instance of ``SourceContext``.
/// - sourceLocation: The source location of the issue. This value is used
/// to construct an instance of ``SourceContext``.
/// - configuration: The test configuration to use when recording the issue.
/// The default value is ``Configuration/current``.
///
/// - Returns: The issue that was recorded.
@discardableResult
static func record(_ kind: Kind, comments: [Comment], backtrace: Backtrace?, sourceLocation: SourceLocation, configuration: Configuration? = nil) -> Self {
let sourceContext = SourceContext(backtrace: backtrace, sourceLocation: sourceLocation)
return record(kind, comments: comments, sourceContext: sourceContext, configuration: configuration)
}

/// Record a new issue with the specified properties.
///
/// - Parameters:
/// - kind: The kind of issue.
/// - comments: An array of comments describing the issue. This array may be
/// empty.
/// - sourceContext: The source context of the issue.
/// - configuration: The test configuration to use when recording the issue.
/// The default value is ``Configuration/current``.
///
/// - Returns: The issue that was recorded.
@discardableResult
static func record(_ kind: Kind, comments: [Comment], sourceContext: SourceContext, configuration: Configuration? = nil) -> Self {
let issue = Issue(kind: kind, comments: comments, sourceContext: sourceContext)
return issue.record(configuration: configuration)
}

/// Record this issue by wrapping it in an ``Event`` and passing it to the
/// current event handler.
///
Expand Down Expand Up @@ -176,13 +139,8 @@ extension Issue {
// condition evaluated to `false`. Those functions record their own issue,
// so we don't need to record another one redundantly.
} catch {
Issue.record(
.errorCaught(error),
comments: [],
backtrace: Backtrace(forFirstThrowOf: error),
sourceLocation: sourceLocation,
configuration: configuration
)
let issue = Issue(for: error, sourceLocation: sourceLocation)
issue.record(configuration: configuration)
return error
}

Expand Down Expand Up @@ -222,13 +180,8 @@ extension Issue {
// condition evaluated to `false`. Those functions record their own issue,
// so we don't need to record another one redundantly.
} catch {
Issue.record(
.errorCaught(error),
comments: [],
backtrace: Backtrace(forFirstThrowOf: error),
sourceLocation: sourceLocation,
configuration: configuration
)
let issue = Issue(for: error, sourceLocation: sourceLocation)
issue.record(configuration: configuration)
return error
}

Expand Down
27 changes: 23 additions & 4 deletions Sources/Testing/Issues/Issue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -108,19 +108,38 @@ public struct Issue: Sendable {
/// - comments: An array of comments describing the issue. This array may be
/// empty.
/// - sourceContext: A ``SourceContext`` indicating where and how this issue
/// occurred. This defaults to a default source context returned by
/// calling ``SourceContext/init(backtrace:sourceLocation:)`` with zero
/// arguments.
/// occurred.
init(
kind: Kind,
comments: [Comment],
sourceContext: SourceContext = .init()
sourceContext: SourceContext
) {
self.kind = kind
self.comments = comments
self.sourceContext = sourceContext
}

/// Initialize an issue instance representing a caught error.
///
/// - Parameters:
/// - error: The error which was caught which this issue is describing.
/// - sourceLocation: The source location of the issue. This value is used
/// to construct an instance of ``SourceContext``.
///
/// The ``sourceContext`` property will have a ``SourceContext/backtrace``
/// property whose value is the backtrace for the first throw of `error`.
init(
for error: any Error,
sourceLocation: SourceLocation? = nil
) {
let sourceContext = SourceContext(backtrace: Backtrace(forFirstThrowOf: error), sourceLocation: sourceLocation)
self.init(
kind: .errorCaught(error),
comments: [],
sourceContext: sourceContext
)
}

/// The error which was associated with this issue, if any.
///
/// The value of this property is non-`nil` when ``kind-swift.property`` is
Expand Down
8 changes: 4 additions & 4 deletions Sources/Testing/Issues/KnownIssue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,12 @@ private func _matchError(_ error: any Error, using issueMatcher: KnownIssueMatch
/// attributed.
private func _handleMiscount(by matchCounter: Locked<Int>, comment: Comment?, sourceLocation: SourceLocation) {
if matchCounter.rawValue == 0 {
Issue.record(
.knownIssueNotRecorded,
let issue = Issue(
kind: .knownIssueNotRecorded,
comments: Array(comment),
backtrace: nil,
sourceLocation: sourceLocation
sourceContext: .init(backtrace: nil, sourceLocation: sourceLocation)
)
issue.record()
}
}

Expand Down
21 changes: 13 additions & 8 deletions Sources/Testing/Running/Runner.Plan.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ extension Runner {
///
/// - Parameters:
/// - skipInfo: A ``SkipInfo`` representing the details of this skip.
indirect case skip(_ skipInfo: SkipInfo = .init())
indirect case skip(_ skipInfo: SkipInfo)

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

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

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

actionGraph.updateValue(action, at: keyPath)
Expand Down Expand Up @@ -437,3 +433,12 @@ extension Runner.Plan.Action {
}
}
#endif

// MARK: - Deprecated

extension Runner.Plan.Action {
@available(*, deprecated, message: "Use .skip(_:) and pass a SkipInfo explicitly.")
public static func skip() -> Self {
.skip(SkipInfo())
}
}
9 changes: 4 additions & 5 deletions Sources/Testing/Running/Runner.swift
Original file line number Diff line number Diff line change
Expand Up @@ -340,13 +340,12 @@ extension Runner {
try await testCase.body()
}
} timeoutHandler: { timeLimit in
Issue.record(
.timeLimitExceeded(timeLimitComponents: timeLimit),
let issue = Issue(
kind: .timeLimitExceeded(timeLimitComponents: timeLimit),
comments: [],
backtrace: .current(),
sourceLocation: sourceLocation,
configuration: configuration
sourceContext: .init(backtrace: .current(), sourceLocation: sourceLocation)
)
issue.record(configuration: configuration)
}
}
}
Expand Down
13 changes: 10 additions & 3 deletions Sources/Testing/Running/SkipInfo.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,9 @@ public struct SkipInfo: Sendable {
/// - comment: A user-specified comment describing this skip, if any.
/// Defaults to `nil`.
/// - sourceContext: A source context indicating where this skip occurred.
/// Defaults to a source context returned by calling
/// ``SourceContext/init(backtrace:sourceLocation:)`` with zero arguments.
public init(
comment: Comment? = nil,
sourceContext: SourceContext = .init()
sourceContext: SourceContext
) {
self.comment = comment
self.sourceContext = sourceContext
Expand All @@ -55,3 +53,12 @@ extension SkipInfo: Equatable, Hashable {}
// MARK: - Codable

extension SkipInfo: Codable {}

// MARK: - Deprecated

extension SkipInfo {
@available(*, deprecated, message: "Use init(comment:sourceContext:) and pass an explicit SourceContext.")
public init(comment: Comment? = nil) {
self.init(comment: comment, sourceContext: .init(backtrace: .current(), sourceLocation: nil))
}
}
23 changes: 18 additions & 5 deletions Sources/Testing/SourceAttribution/SourceContext.swift
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,10 @@ public struct SourceContext: Sendable {
/// source location.
///
/// - Parameters:
/// - backtrace: The backtrace associated with the new instance. Defaults to
/// the current backtrace (obtained via
/// ``Backtrace/current(maximumAddressCount:)``).
/// - sourceLocation: The source location associated with the new instance.
public init(backtrace: Backtrace? = .current(), sourceLocation: SourceLocation? = nil) {
/// - backtrace: The backtrace associated with the new instance.
/// - sourceLocation: The source location associated with the new instance,
/// if available.
public init(backtrace: Backtrace?, sourceLocation: SourceLocation?) {
self.backtrace = backtrace
self.sourceLocation = sourceLocation
}
Expand All @@ -41,3 +40,17 @@ extension SourceContext: Equatable, Hashable {}
// MARK: - Codable

extension SourceContext: Codable {}

// MARK: - Deprecated

extension SourceContext {
@available(*, deprecated, message: "Use init(backtrace:sourceLocation:) and pass both arguments explicitly instead.")
public init(backtrace: Backtrace?) {
self.init(backtrace: backtrace, sourceLocation: nil)
}

@available(*, deprecated, message: "Use init(backtrace:sourceLocation:) and pass both arguments explicitly instead.")
public init(sourceLocation: SourceLocation? = nil) {
self.init(backtrace: nil, sourceLocation: sourceLocation)
}
}
8 changes: 4 additions & 4 deletions Sources/Testing/Test+Macro.swift
Original file line number Diff line number Diff line change
Expand Up @@ -602,11 +602,11 @@ public func __invokeXCTestCaseMethod<T>(
guard let xcTestCaseClass, isClass(xcTestCaseSubclass, subclassOf: xcTestCaseClass) else {
return false
}
Issue.record(
.apiMisused,
let issue = Issue(
kind: .apiMisused,
comments: ["The @Test attribute cannot be applied to methods on a subclass of XCTestCase."],
backtrace: nil,
sourceLocation: sourceLocation
sourceContext: .init(backtrace: nil, sourceLocation: sourceLocation)
)
issue.record()
return true
}
7 changes: 6 additions & 1 deletion Sources/Testing/Traits/ConditionTrait.swift
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,12 @@ public struct ConditionTrait: TestTrait, SuiteTrait {
}

if !result {
let sourceContext = SourceContext(sourceLocation: sourceLocation)
// We don't need to consider including a backtrace here because it will
// primarily contain frames in the testing library, not user code. If an
// error was thrown by a condition evaluated above, the caller _should_
// attempt to get the backtrace of the caught error when creating an issue
// for it, however.
let sourceContext = SourceContext(backtrace: nil, sourceLocation: sourceLocation)
throw SkipInfo(comment: commentOverride ?? comments.first, sourceContext: sourceContext)
}
}
Expand Down
4 changes: 2 additions & 2 deletions Tests/TestingTests/EventRecorderTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ struct EventRecorderTests {

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

Expand All @@ -373,7 +373,7 @@ struct EventRecorderTests {

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

Expand Down
16 changes: 4 additions & 12 deletions Tests/TestingTests/IssueTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1038,7 +1038,7 @@ final class IssueTests: XCTestCase {

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

var issueSourceLocation = try XCTUnwrap(issue.sourceLocation)
XCTAssertEqual(issueSourceLocation.line, 12345)
Expand Down Expand Up @@ -1480,7 +1480,7 @@ struct IssueCodingTests {
}

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

let issueSnapshot = Issue.Snapshot(snapshotting: issue)
Expand All @@ -1501,11 +1501,7 @@ struct IssueCodingTests {
sourceLocation: sourceLocation
)

let issue = Issue(
kind: .apiMisused,
comments: [],
sourceContext: sourceContext
)
let issue = Issue(kind: .apiMisused, sourceContext: sourceContext)

let issueSnapshot = Issue.Snapshot(snapshotting: issue)
#expect(issueSnapshot.sourceContext == sourceContext)
Expand All @@ -1525,11 +1521,7 @@ struct IssueCodingTests {
sourceLocation: initialSourceLocation
)

let issue = Issue(
kind: .apiMisused,
comments: [],
sourceContext: sourceContext
)
let issue = Issue(kind: .apiMisused, sourceContext: sourceContext)

let updatedSourceLocation = SourceLocation(
fileID: "fileID2",
Expand Down
Loading