Add withKnownIssue comments to known Issues (#1014)

Add withKnownIssue comments to known Issues

### Motivation:

The Comment passed to withKnownIssue() is currently only used when a
known issue does not occur. When a known issue does occur, it is marked
isKnown but does not contain any record of the withKnownIssue() comment,
making it harder to understand why the issue was considered to be known,
especially if there are multiple nested withKnownIssue() calls.

### Modifications:

When an issue is marked "known" by a `withKnownIssue()` call, the
recorded issue will now have a new `knownIssueContext` property
containing the comment passed to `withKnownIssue()` (if any). This
comment will be included in the `messages` array of the
`ABI.EncodedEvent` that represents the issue.

If the issue is recorded within multiple nested `withKnownIssue()`
calls, `knownIssueContext` corresponds to the innermost matching call.

The `Issue.isKnown` setter is now deprecated and a no-op.

When an error is thrown within a `withKnownIssue()` call, the `Issue`
passed to the issue matcher used to have the known issue comment in
`Issue.comments`. That has now been removed; Issues for thrown errors
now have no comments at all when passed to the issue matcher. This
matches Issues for thrown errors that occur outside of
`withKnownIssue()` calls.

### 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.

---------

Co-authored-by: Jonathan Grynspan <[email protected]>
Co-authored-by: Stuart Montgomery <[email protected]>

Author: 3 people

Sources/Testing/Events/Recorder/Event.HumanReadableOutputRecorder.swift

@@ -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
@@ -449,6 +459,9 @@ extension Event.HumanReadableOutputRecorder {
         additionalMessages.append(Message(symbol: .difference, stringValue: differenceDescription))
       }
       additionalMessages += _formattedComments(issue.comments)
+      if let knownIssueComment = issue.knownIssueContext?.comment {
+        additionalMessages.append(_formattedComment(knownIssueComment))
+      }
 
       if verbosity > 0, case let .expectationFailed(expectation) = issue.kind {
         let expression = expectation.evaluatedExpression

Sources/Testing/ExitTests/ExitTest.swift

@@ -930,7 +930,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()
     }
   }

Sources/Testing/Issues/Issue+Recording.swift

@@ -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.
   ///
@@ -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)
     }
 

Sources/Testing/Issues/Issue.swift

@@ -129,9 +129,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.
   ///

Sources/Testing/Issues/KnownIssue.swift

@@ -8,44 +8,80 @@
 // 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) -> Issue.KnownIssueContext?
+
+  /// The matcher function for this known issue scope.
+  var matcher: Matcher
+
+  /// 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. Defaults to ``KnownIssueScope.current``.
+  ///   - 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`.
+  init(parent: KnownIssueScope? = .current, 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 any.
+  ///
+  /// 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, in 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) {
+  var issue = Issue(kind: .errorCaught(error), comments: [], sourceContext: sourceContext)
+  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
@@ -184,18 +220,17 @@ public func withKnownIssue(
   guard precondition() else {
     return try body()
   }
-  let matchCounter = Locked(rawValue: 0)
-  let issueMatcher = _combineIssueMatcher(issueMatcher, matchesCountedBy: matchCounter)
+  let scope = KnownIssueScope(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, in: scope, comment: comment, sourceLocation: sourceLocation)
     }
   }
 }
@@ -304,18 +339,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(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, in: scope, comment: comment, sourceLocation: sourceLocation)
     }
   }
 }

Tests/TestingTests/EventRecorderTests.swift

@@ -521,6 +521,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.rawValue.dropFirst().map(\.stringValue)
+    #expect(actualComments == expectedComments)
+  }
 }
 
 // MARK: - Fixtures
@@ -663,3 +692,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()
+    }
+  }
+}