Skip to content

Add withKnownIssue comments to known Issues #1014

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 27 commits into from
Apr 24, 2025
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c6cbd3d
Add withKnownIssue comments to known Issues
aroben Mar 10, 2025
acdb283
KnownIssueContext.current
aroben Mar 12, 2025
08b1c3f
KnownIssueContext -> KnownIssueScope
aroben Mar 12, 2025
45f8d9f
Issue.knownIssueContext
aroben Mar 12, 2025
e2da439
Update comment
aroben Mar 12, 2025
e0251d8
Fix typo
aroben Mar 12, 2025
89b0a67
Remove KnownIssueScope.Match
aroben Mar 12, 2025
deffa29
Renames for clarity
aroben Mar 12, 2025
e6e4fd4
Update tests
aroben Mar 12, 2025
a9b3163
Fix up doc comments
aroben Mar 18, 2025
1282cab
Remove Issue.isKnown.set since this is only SPI
aroben Mar 18, 2025
0dfcff8
Preserve EncodedIssue ABI
aroben Mar 18, 2025
7cd86f6
Add ABI.EncodedKnownIssueContext
aroben Mar 24, 2025
c0e8ce7
Fix doc comments
aroben Mar 24, 2025
af08da2
Replace EncodedIssue._knownIssueContext with an extra Message
aroben Apr 7, 2025
b66a484
Remove KnownIssueContext.sourceLocation
aroben Apr 7, 2025
b9275e8
Resurrect Issue.isKnown.setter
aroben Apr 7, 2025
81fe877
Remove `withKnownIssue()` shorthand
aroben Apr 7, 2025
9192e4a
Add a test for the HumanReadableOutputRecorder change
aroben Apr 7, 2025
feb96cf
Rename KnownIssueScope.match -> matcher
aroben Apr 7, 2025
874f830
Deduplicate known issue comments for thrown errors
aroben Apr 7, 2025
6caad07
Use Locked.rawValue
aroben Apr 7, 2025
316d10c
Explain why we treat .errorCaught specially
aroben Apr 7, 2025
8567a61
Apply suggestions from code review
aroben Apr 24, 2025
1ae8633
Don't put the known issue comment in Issue.comments for thrown errors
aroben Apr 24, 2025
00ad66d
Apply suggestions from code review
aroben Apr 24, 2025
0a3a995
Incorporate review comments
aroben Apr 24, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,20 @@ extension Event.HumanReadableOutputRecorder {
/// - Parameters:
/// - comments: The comments that should be formatted.
///
/// - Returns: A formatted string representing `comments`, or `nil` if there
/// are none.
/// - Returns: An array of formatted messages representing `comments`, or an
/// empty array if there are none.
private func _formattedComments(_ comments: [Comment]) -> [Message] {
comments.map { Message(symbol: .details, stringValue: $0.rawValue) }
comments.map(_formattedComment)
}

/// Get a string representing a single comment, formatted for output.
///
/// - Parameters:
/// - comment: The comment that should be formatted.
///
/// - Returns: A formatted message representing `comment`.
private func _formattedComment(_ comment: Comment) -> Message {
Message(symbol: .details, stringValue: comment.rawValue)
}

/// Get a string representing the comments attached to a test, formatted for
Expand Down Expand Up @@ -443,6 +453,13 @@ extension Event.HumanReadableOutputRecorder {
additionalMessages.append(Message(symbol: .difference, stringValue: differenceDescription))
}
additionalMessages += _formattedComments(issue.comments)
if let knownIssueComment = issue.knownIssueContext?.comment {
if case .errorCaught = issue.kind {
// The known issue comment is already included in `issue.comments`.
} else {
additionalMessages.append(_formattedComment(knownIssueComment))
}
}

if verbosity > 0, case let .expectationFailed(expectation) = issue.kind {
let expression = expectation.evaluatedExpression
Expand Down
6 changes: 5 additions & 1 deletion Sources/Testing/ExitTests/ExitTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -746,7 +746,11 @@ extension ExitTest {
sourceLocation: issue.sourceLocation
)
var issueCopy = Issue(kind: issueKind, comments: comments, sourceContext: sourceContext)
issueCopy.isKnown = issue.isKnown
if issue.isKnown {
// The known issue comment, if there was one, is already included in
// the `comments` array above.
issueCopy.knownIssueContext = Issue.KnownIssueContext()
}
issueCopy.record()
}
}
Expand Down
12 changes: 2 additions & 10 deletions Sources/Testing/Issues/Issue+Recording.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,6 @@
//

extension Issue {
/// The known issue matcher, as set by `withKnownIssue()`, associated with the
/// current task.
///
/// If there is no call to `withKnownIssue()` executing on the current task,
/// the value of this property is `nil`.
@TaskLocal
static var currentKnownIssueMatcher: KnownIssueMatcher?

/// Record this issue by wrapping it in an ``Event`` and passing it to the
/// current event handler.
///
Expand All @@ -38,9 +30,9 @@ extension Issue {

// If this issue matches via the known issue matcher, set a copy of it to be
// known and record the copy instead.
if !isKnown, let issueMatcher = Self.currentKnownIssueMatcher, issueMatcher(self) {
if !isKnown, let context = KnownIssueScope.current?.matcher(self) {
var selfCopy = self
selfCopy.isKnown = true
selfCopy.knownIssueContext = context
return selfCopy.record(configuration: configuration)
}

Expand Down
22 changes: 21 additions & 1 deletion Sources/Testing/Issues/Issue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,29 @@ public struct Issue: Sendable {
@_spi(ForToolsIntegrationOnly)
public var sourceContext: SourceContext

/// A type representing a
/// ``withKnownIssue(_:isIntermittent:sourceLocation:_:when:matching:)`` call
/// that matched an issue.
@_spi(ForToolsIntegrationOnly)
public struct KnownIssueContext: Sendable {
/// The comment that was passed to
/// ``withKnownIssue(_:isIntermittent:sourceLocation:_:when:matching:)``.
public var comment: Comment?
}

/// A ``KnownIssueContext-swift.struct`` representing the
/// ``withKnownIssue(_:isIntermittent:sourceLocation:_:when:matching:)`` call
/// that matched this issue, if any.
@_spi(ForToolsIntegrationOnly)
public var knownIssueContext: KnownIssueContext? = nil

/// Whether or not this issue is known to occur.
@_spi(ForToolsIntegrationOnly)
public var isKnown: Bool = false
public var isKnown: Bool {
get { knownIssueContext != nil }
@available(*, deprecated, message: "Setting this property has no effect.")
set {}
}

/// Initialize an issue instance with the specified details.
///
Expand Down
101 changes: 69 additions & 32 deletions Sources/Testing/Issues/KnownIssue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,44 +8,83 @@
// See https://swift.org/CONTRIBUTORS.txt for Swift project authors
//

/// Combine an instance of ``KnownIssueMatcher`` with any previously-set one.
///
/// - Parameters:
/// - issueMatcher: A function to invoke when an issue occurs that is used to
/// determine if the issue is known to occur.
/// - matchCounter: The counter responsible for tracking the number of matches
/// found with `issueMatcher`.
///
/// - Returns: A new instance of ``Configuration`` or `nil` if there was no
/// current configuration set.
private func _combineIssueMatcher(_ issueMatcher: @escaping KnownIssueMatcher, matchesCountedBy matchCounter: Locked<Int>) -> KnownIssueMatcher {
let oldIssueMatcher = Issue.currentKnownIssueMatcher
return { issue in
if issueMatcher(issue) || true == oldIssueMatcher?(issue) {
matchCounter.increment()
return true
/// A type that represents an active
/// ``withKnownIssue(_:isIntermittent:sourceLocation:_:when:matching:)``
/// call and any parent calls.
///
/// A stack of these is stored in `KnownIssueScope.current`.
struct KnownIssueScope: Sendable {
/// A function which determines if an issue matches a known issue scope or
/// any of its ancestor scopes.
///
/// - Parameters:
/// - issue: The issue being matched.
/// - Returns: A known issue context containing information about the known
/// issue, if the issue is considered "known" by this known issue scope or any
/// ancestor scope, or `nil` otherwise.
typealias Matcher = @Sendable (Issue) -> Issue.KnownIssueContext?

/// Determine if an issue is known to this scope or any of its ancestor
/// scopes.
///
/// Returns `nil` if the issue is not known.
var matcher: @Sendable (Issue) -> Issue.KnownIssueContext?

/// The number of issues this scope and its ancestors have matched.
let matchCounter: Locked<Int>

/// Create a new ``KnownIssueScope`` by combining a new issue matcher with
/// any already-active scope.
///
/// - Parameters:
/// - parent: The context that should be checked next if `issueMatcher`
/// fails to match an issue.
/// - issueMatcher: A function to invoke when an issue occurs that is used
/// to determine if the issue is known to occur.
/// - context: The context to be associated with issues matched by
/// `issueMatcher`.
/// - Returns: A new instance of ``KnownIssueScope``.
init(parent: KnownIssueScope?, issueMatcher: @escaping KnownIssueMatcher, context: Issue.KnownIssueContext) {
let matchCounter = Locked(rawValue: 0)
self.matchCounter = matchCounter
matcher = { issue in
let matchedContext = if issueMatcher(issue) {
context
} else {
parent?.matcher(issue)
}
if matchedContext != nil {
matchCounter.increment()
}
return matchedContext
}
return false
}

/// The active known issue scope for the current task.
///
/// If there is no call to
/// ``withKnownIssue(_:isIntermittent:sourceLocation:_:when:matching:)``
/// executing on the current task, the value of this property is `nil`.
@TaskLocal
static var current: KnownIssueScope?
}

/// Check if an error matches using an issue-matching function, and throw it if
/// it does not.
///
/// - Parameters:
/// - error: The error to test.
/// - issueMatcher: A function to which `error` is passed (after boxing it in
/// an instance of ``Issue``) to determine if it is known to occur.
/// - scope: The known issue scope that is processing the error.
/// - comment: An optional comment to apply to any issues generated by this
/// function.
/// - sourceLocation: The source location to which the issue should be
/// attributed.
private func _matchError(_ error: any Error, using issueMatcher: KnownIssueMatcher, comment: Comment?, sourceLocation: SourceLocation) throws {
private func _matchError(_ error: any Error, using scope: KnownIssueScope, comment: Comment?, sourceLocation: SourceLocation) throws {
let sourceContext = SourceContext(backtrace: Backtrace(forFirstThrowOf: error), sourceLocation: sourceLocation)
var issue = Issue(kind: .errorCaught(error), comments: Array(comment), sourceContext: sourceContext)
if issueMatcher(issue) {
if let context = scope.matcher(issue) {
// It's a known issue, so mark it as such before recording it.
issue.isKnown = true
issue.knownIssueContext = context
issue.record()
} else {
// Rethrow the error, allowing the caller to catch it or for it to propagate
Expand Down Expand Up @@ -184,18 +223,17 @@ public func withKnownIssue(
guard precondition() else {
return try body()
}
let matchCounter = Locked(rawValue: 0)
let issueMatcher = _combineIssueMatcher(issueMatcher, matchesCountedBy: matchCounter)
let scope = KnownIssueScope(parent: .current, issueMatcher: issueMatcher, context: Issue.KnownIssueContext(comment: comment))
defer {
if !isIntermittent {
_handleMiscount(by: matchCounter, comment: comment, sourceLocation: sourceLocation)
_handleMiscount(by: scope.matchCounter, comment: comment, sourceLocation: sourceLocation)
}
}
try Issue.$currentKnownIssueMatcher.withValue(issueMatcher) {
try KnownIssueScope.$current.withValue(scope) {
do {
try body()
} catch {
try _matchError(error, using: issueMatcher, comment: comment, sourceLocation: sourceLocation)
try _matchError(error, using: scope, comment: comment, sourceLocation: sourceLocation)
}
}
}
Expand Down Expand Up @@ -304,18 +342,17 @@ public func withKnownIssue(
guard await precondition() else {
return try await body()
}
let matchCounter = Locked(rawValue: 0)
let issueMatcher = _combineIssueMatcher(issueMatcher, matchesCountedBy: matchCounter)
let scope = KnownIssueScope(parent: .current, issueMatcher: issueMatcher, context: Issue.KnownIssueContext(comment: comment))
defer {
if !isIntermittent {
_handleMiscount(by: matchCounter, comment: comment, sourceLocation: sourceLocation)
_handleMiscount(by: scope.matchCounter, comment: comment, sourceLocation: sourceLocation)
}
}
try await Issue.$currentKnownIssueMatcher.withValue(issueMatcher) {
try await KnownIssueScope.$current.withValue(scope) {
do {
try await body()
} catch {
try _matchError(error, using: issueMatcher, comment: comment, sourceLocation: sourceLocation)
try _matchError(error, using: scope, comment: comment, sourceLocation: sourceLocation)
}
}
}
61 changes: 61 additions & 0 deletions Tests/TestingTests/EventRecorderTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,35 @@ struct EventRecorderTests {
recorder.record(Event(.runEnded, testID: nil, testCaseID: nil), in: context)
}
}

@Test(
"HumanReadableOutputRecorder includes known issue comment in messages array",
arguments: [
("recordWithoutKnownIssueComment()", ["#expect comment"]),
("recordWithKnownIssueComment()", ["#expect comment", "withKnownIssue comment"]),
("throwWithoutKnownIssueComment()", []),
("throwWithKnownIssueComment()", ["withKnownIssue comment"]),
]
)
func knownIssueComments(testName: String, expectedComments: [String]) async throws {
var configuration = Configuration()
let recorder = Event.HumanReadableOutputRecorder()
let messages = Locked<[Event.HumanReadableOutputRecorder.Message]>(rawValue: [])
configuration.eventHandler = { event, context in
guard case .issueRecorded = event.kind else { return }
messages.withLock {
$0.append(contentsOf: recorder.record(event, in: context))
}
}

await runTestFunction(named: testName, in: PredictablyFailingKnownIssueTests.self, configuration: configuration)

// The first message is something along the lines of "Test foo recorded a
// known issue" and includes a source location, so is inconvenient to
// include in our expectation here.
let actualComments = messages.withLock(\.self).dropFirst().map(\.stringValue)
#expect(actualComments == expectedComments)
}
}

// MARK: - Fixtures
Expand Down Expand Up @@ -639,3 +668,35 @@ struct EventRecorderTests {
#expect(arg > 0)
}
}

@Suite(.hidden) struct PredictablyFailingKnownIssueTests {
@Test(.hidden)
func recordWithoutKnownIssueComment() {
withKnownIssue {
#expect(Bool(false), "#expect comment")
}
}

@Test(.hidden)
func recordWithKnownIssueComment() {
withKnownIssue("withKnownIssue comment") {
#expect(Bool(false), "#expect comment")
}
}

@Test(.hidden)
func throwWithoutKnownIssueComment() {
withKnownIssue {
struct TheError: Error {}
throw TheError()
}
}

@Test(.hidden)
func throwWithKnownIssueComment() {
withKnownIssue("withKnownIssue comment") {
struct TheError: Error {}
throw TheError()
}
}
}
Loading