Add withKnownIssue comments to known Issues #1014

aroben · 2025-03-11T17:41:24Z

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.

Checklist:

Code and documentation should follow the style of the Style Guide.
If public symbols are renamed or modified, DocC references should be updated.

aroben · 2025-03-11T18:03:42Z

@swift-ci test

### 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 multiple comments in its `comments` array: 1. The comment passed to `withKnownIssue()`, if any 2. The Issue's own comment(s) If the issue is recorded within multiple nested `withKnownIssue()` calls, only the comment of the innermost matching call is added to the Issue. ### 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.

aroben · 2025-03-11T18:17:34Z

@swift-ci test

aroben · 2025-03-11T19:43:23Z

@swift-ci test

aroben · 2025-03-11T20:02:07Z

@swift-ci test

Sources/Testing/Issues/Issue+Recording.swift

aroben · 2025-03-12T14:06:39Z

@stmontgomery I took a crack at putting the known-issue data into a separate stored property and converting isKnown to a computed property. Some questions:

I left a setter in place for isKnown to try to preserve ABI compatibility. Does this seem important to you?
What do you think about the names Issue.knownIssueContext and Issue.KnownIssueContext?
I replaced EncodedIssue.isKnown with a new EncodedIssue.knownIssueContext and made KnownIssueContext conform to Codable. Does that seem like the right approach? Do I need to bump the ABI version or do anything else to handle compatibility?

aroben · 2025-03-12T14:06:55Z

@swift-ci test

aroben · 2025-03-12T14:10:59Z

@swift-ci test

aroben · 2025-03-12T14:18:33Z

@swift-ci test

Sources/Testing/ABI/Encoded/ABI.EncodedKnownIssueContext.swift

Sources/Testing/Issues/KnownIssue.swift

Sources/Testing/ABI/Encoded/ABI.EncodedIssue.swift

Sources/Testing/Issues/Issue.swift

grynspan

This is not the direction I'd go with this change.

Sources/Testing/ABI/Encoded/ABI.EncodedIssue.swift

Sources/Testing/Issues/Issue.swift

aroben

This is not the direction I'd go with this change.

@grynspan For reference, I've pushed my initial version of this PR here: main...aroben:swift-testing:known-issue-comments-v1

In that take, I just prepended the known-issue comment to Issue.comments and left it at that. As you saw, @stmontgomery encouraged me to pull it out into its own struct to make the difference between the Issue's comment and the known-issue comment explicit.

Maybe you two can sync up and agree on an approach here? I'm happy to do it either way.

Sources/Testing/ABI/Encoded/ABI.EncodedIssue.swift

Sources/Testing/ABI/Encoded/ABI.EncodedKnownIssueContext.swift

Sources/Testing/ABI/Encoded/ABI.EncodedIssue.swift

Sources/Testing/Issues/Issue.swift

Sources/Testing/ABI/Encoded/ABI.EncodedIssue.swift

Sources/Testing/Issues/Issue.swift

We don't have a use for it yet.

aroben · 2025-04-07T14:19:38Z

After discussion with @grynspan and @stmontgomery I've reverted the ABI changes. Instead, we append the known issue comment (if any) to ABI.EncodedEvent.messages for the .issueRecorded event.

aroben · 2025-04-07T14:27:07Z

@swift-ci please test

aroben · 2025-04-07T14:54:29Z

I wanted to see if the known issue comment now appears in swift test's output, so I added a new suite to this repo:

struct TryIt {
  @Test func knownIssueTest() async throws {
    withKnownIssue("known issue comment") {
      Issue.record("issue comment")
    }
  }

  @Test func knownIssueTest_throws() async throws {
    withKnownIssue("known issue comment") {
      struct TheError: Error {}
      throw TheError()
    }
  }
}

Here's what I got:

$ SWT_SF_SYMBOLS_ENABLED=no xcrun swift test --filter TryIt      
Building for debugging...
[1/1] Write swift-version--1EBDF3DDE0C0C582.txt
Build complete! (0.16s)
Test Suite 'Selected tests' started at 2025-04-07 10:48:10.677.
Test Suite 'swift-testingPackageTests.xctest' started at 2025-04-07 10:48:10.678.
Test Suite 'swift-testingPackageTests.xctest' passed at 2025-04-07 10:48:10.678.
	 Executed 0 tests, with 0 failures (0 unexpected) in 0.000 (0.000) seconds
Test Suite 'Selected tests' passed at 2025-04-07 10:48:10.678.
	 Executed 0 tests, with 0 failures (0 unexpected) in 0.000 (0.001) seconds
◇ Test run started.
↳ Testing Library Version: feb96cf762e5732f8702e1dc9755fa2908bfb5ba (modified)
◇ Suite TryIt started.
◇ Test knownIssueTest_throws() started.
◇ Test knownIssueTest() started.
✘ Test knownIssueTest() recorded a known issue at EventRecorderTests.swift:660:19: Issue recorded
↳ issue comment
↳ known issue comment
✘ Test knownIssueTest() passed after 0.001 seconds with 1 known issue.
✘ Test knownIssueTest_throws() recorded a known issue at EventRecorderTests.swift:665:19: Caught error: TheError()
↳ known issue comment
↳ known issue comment
✘ Test knownIssueTest_throws() passed after 0.001 seconds with 1 known issue.
✘ Suite TryIt passed after 0.001 seconds with 2 known issues.
✘ Test run with 2 tests passed after 0.001 seconds with 2 known issues.

This looks decent, but you can see that known issue comment is duplicated for knownIssueTest_throws(). This is because _matchError puts the known issue comment directly in the Issue.comments array, but then we also include it in Issue.knownIssueContext, so we end up with two copies of the comment.

Here are some ways to handle this:

Leave it as-is
Remove the known issue comment from Issue.comments, leaving it only in Issue.knownIssueContext.
a. This seems right from a modeling perspective, but will in practice remove the known issue comment from Xcode's inline error annotation UI
Omit the known issue comment from Issue.knownIssueContext for thrown errors
a. This feels bad, though I'm not sure it has any practical downsides.
Teach HumanReadableOutputRecorder to somehow detect this case and avoid the duplication
a. We should be able to detect this reliably: if case .errorCaught = issue.kind, issue.isKnown

I'm leaning toward 4 (and would add a test for it of course).

aroben · 2025-04-07T15:28:56Z

I'm leaning toward 4 (and would add a test for it of course).

I implemented this.

aroben · 2025-04-07T15:29:00Z

@swift-ci please test

Tests/TestingTests/EventRecorderTests.swift

grynspan · 2025-04-07T15:47:52Z

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

@@ -454,7 +454,11 @@ extension Event.HumanReadableOutputRecorder {
      }
      additionalMessages += _formattedComments(issue.comments)
      if let knownIssueComment = issue.knownIssueContext?.comment {
-        additionalMessages.append(_formattedComment(knownIssueComment))
+        if case .errorCaught = issue.kind {


Can you explain why this specific case is being singled out?

Pushed an updated comment, how does it look to you?

Seems like the correct fix might be to remove the comment from _matchError() so that nothing needs to be special-cased?

Note for compatibility, we would want to clear the comments property of Issue in _matchError() after calling the matcher function as there could be code out there that inspects it:

withKnownIssue { ... } matching: { issue in issue.comments.first?.contains("abc") ?? false }

(Something like that.)

Seems like the correct fix might be to remove the comment from _matchError() so that nothing needs to be special-cased?

We certainly could, but that has a downside that I mentioned in #1014 (comment):

Remove the known issue comment from Issue.comments, leaving it only in Issue.knownIssueContext.
a. This seems right from a modeling perspective, but will in practice remove the known issue comment from Xcode's inline error annotation UI

Yes, but I'm trying not to introduce any regressions along the way.

I appreciate that :) And that's a good goal—but we also need to be mindful of creating technical debt for ourselves.

OK, so do you think I should take approach 2 despite the regression in Xcode and the weirdness of Issue.comments changing after the matcher runs?

We discussed this and decided to remove the comment entirely from _matchError. This has a slight risk of causing incompatibilities because the matcher function will no longer see the comment there, but it's actually kind of weird that the matcher sees the comment anyway since it is always the same as the comment passed to withKnownIssue:

@Test func knownIssueCommentPassedToIssueMatcher() throws { struct E: Error {} try withKnownIssue("Known Issue Comment") { throw E() } matching: { issue in print("Comments passed to matcher:", issue.comments) // prints: Comments passed to matcher: ["Known Issue Comment"] // which seems unhelpful since we can see the comment a few lines up in the withKnownIssue call return true } }```

I agree we should stop including the comment passed to withKnownIssue (if any) in the comments of the Issue constructed in _matchError().

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

aroben · 2025-04-07T16:24:59Z

@swift-ci please test

stmontgomery

A few nitpicky formatting comments I saw as I caught up with the latest changes

stmontgomery · 2025-04-09T20:14:29Z

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

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


Suggested change

/// empty array if there are none.

/// empty array if there are none.

stmontgomery · 2025-04-09T20:20:18Z

Sources/Testing/Issues/KnownIssue.swift

+  /// - 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?


Explicitly declare issue so that the - Parameters: entry refers to it:

Suggested change

typealias Matcher = @Sendable (Issue) -> Issue.KnownIssueContext?

typealias Matcher = @Sendable (_ issue: Issue) -> Issue.KnownIssueContext?

stmontgomery · 2025-04-09T20:20:37Z

Sources/Testing/Issues/KnownIssue.swift

+  /// - 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.


Suggested change

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

///

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

stmontgomery · 2025-04-09T20:29:59Z

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

@@ -454,7 +454,11 @@ extension Event.HumanReadableOutputRecorder {
      }
      additionalMessages += _formattedComments(issue.comments)
      if let knownIssueComment = issue.knownIssueContext?.comment {
-        additionalMessages.append(_formattedComment(knownIssueComment))
+        if case .errorCaught = issue.kind {


I agree we should stop including the comment passed to withKnownIssue (if any) in the comments of the Issue constructed in _matchError().

aroben requested review from stmontgomery, grynspan, briancroom, SeanROlszewski and suzannaratcliff as code owners March 11, 2025 17:41

grynspan assigned aroben Mar 11, 2025

grynspan added bug 🪲 Something isn't working issue-handling Related to Issue handling within the testing library labels Mar 11, 2025

grynspan added this to the Swift 6.2 milestone Mar 11, 2025

aroben force-pushed the known-issue-comments branch from cf99d11 to c6cbd3d Compare March 11, 2025 18:17

stmontgomery reviewed Mar 11, 2025

View reviewed changes

Sources/Testing/Issues/Issue+Recording.swift Outdated Show resolved Hide resolved

stmontgomery reviewed Mar 11, 2025

View reviewed changes

Sources/Testing/Issues/Issue+Recording.swift Outdated Show resolved Hide resolved

aroben added 3 commits March 12, 2025 09:42

KnownIssueContext.current

acdb283

KnownIssueContext -> KnownIssueScope

08b1c3f

Issue.knownIssueContext

45f8d9f

aroben added 4 commits March 12, 2025 10:08

Update comment

e2da439

Fix typo

e0251d8

Remove KnownIssueScope.Match

89b0a67

Renames for clarity

deffa29

Update tests

e6e4fd4

aroben requested a review from stmontgomery March 12, 2025 18:01

stmontgomery reviewed Apr 3, 2025

View reviewed changes

Sources/Testing/ABI/Encoded/ABI.EncodedKnownIssueContext.swift Outdated Show resolved Hide resolved

stmontgomery reviewed Apr 3, 2025

View reviewed changes

Sources/Testing/Issues/KnownIssue.swift Outdated Show resolved Hide resolved

grynspan reviewed Apr 3, 2025

View reviewed changes

Sources/Testing/ABI/Encoded/ABI.EncodedIssue.swift Outdated Show resolved Hide resolved

grynspan reviewed Apr 3, 2025

View reviewed changes

Sources/Testing/Issues/Issue.swift Outdated Show resolved Hide resolved

grynspan requested changes Apr 3, 2025

View reviewed changes

grynspan reviewed Apr 3, 2025

View reviewed changes

Sources/Testing/ABI/Encoded/ABI.EncodedIssue.swift Outdated Show resolved Hide resolved

grynspan reviewed Apr 3, 2025

View reviewed changes

Sources/Testing/ABI/Encoded/ABI.EncodedIssue.swift Outdated Show resolved Hide resolved

grynspan reviewed Apr 3, 2025

View reviewed changes

Sources/Testing/Issues/Issue.swift Outdated Show resolved Hide resolved

aroben commented Apr 4, 2025

View reviewed changes

grynspan reviewed Apr 4, 2025

View reviewed changes

Sources/Testing/Issues/Issue.swift Outdated Show resolved Hide resolved

grynspan modified the milestones: Swift 6.2, Swift 6.x Apr 6, 2025

aroben added 5 commits April 7, 2025 10:00

Replace EncodedIssue._knownIssueContext with an extra Message

af08da2

Remove KnownIssueContext.sourceLocation

b66a484

We don't have a use for it yet.

Resurrect Issue.isKnown.setter

b9275e8

Remove withKnownIssue() shorthand

81fe877

Add a test for the HumanReadableOutputRecorder change

9192e4a

Rename KnownIssueScope.match -> matcher

feb96cf

Deduplicate known issue comments for thrown errors

874f830

grynspan reviewed Apr 7, 2025

View reviewed changes

Tests/TestingTests/EventRecorderTests.swift Outdated Show resolved Hide resolved

grynspan reviewed Apr 7, 2025

View reviewed changes

aroben and others added 2 commits April 7, 2025 12:21

Use Locked.rawValue

6caad07

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

Explain why we treat .errorCaught specially

316d10c

stmontgomery reviewed Apr 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add withKnownIssue comments to known Issues #1014

Add withKnownIssue comments to known Issues #1014

aroben commented Mar 11, 2025 •

edited

Loading

aroben commented Mar 11, 2025

aroben commented Mar 11, 2025

aroben commented Mar 11, 2025

aroben commented Mar 11, 2025

aroben commented Mar 12, 2025

aroben commented Mar 12, 2025

aroben commented Mar 12, 2025

aroben commented Mar 12, 2025

grynspan left a comment

aroben left a comment

aroben commented Apr 7, 2025

aroben commented Apr 7, 2025

aroben commented Apr 7, 2025 •

edited

Loading

aroben commented Apr 7, 2025

aroben commented Apr 7, 2025

grynspan Apr 7, 2025

aroben Apr 7, 2025

grynspan Apr 7, 2025

grynspan Apr 7, 2025

aroben Apr 7, 2025 •

edited

Loading

aroben Apr 7, 2025

grynspan Apr 7, 2025

aroben Apr 7, 2025

aroben Apr 9, 2025

stmontgomery Apr 9, 2025

aroben commented Apr 7, 2025

stmontgomery left a comment

stmontgomery Apr 9, 2025

stmontgomery Apr 9, 2025

stmontgomery Apr 9, 2025

stmontgomery Apr 9, 2025

	/// empty array if there are none.
	/// empty array if there are none.

	typealias Matcher = @Sendable (Issue) -> Issue.KnownIssueContext?
	typealias Matcher = @Sendable (_ issue: Issue) -> Issue.KnownIssueContext?

Add withKnownIssue comments to known Issues #1014

Are you sure you want to change the base?

Add withKnownIssue comments to known Issues #1014

Conversation

aroben commented Mar 11, 2025 • edited Loading

Motivation:

Modifications:

Checklist:

aroben commented Mar 11, 2025

aroben commented Mar 11, 2025

aroben commented Mar 11, 2025

aroben commented Mar 11, 2025

aroben commented Mar 12, 2025

aroben commented Mar 12, 2025

aroben commented Mar 12, 2025

aroben commented Mar 12, 2025

grynspan left a comment

Choose a reason for hiding this comment

aroben left a comment

Choose a reason for hiding this comment

aroben commented Apr 7, 2025

aroben commented Apr 7, 2025

aroben commented Apr 7, 2025 • edited Loading

aroben commented Apr 7, 2025

aroben commented Apr 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aroben Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aroben commented Apr 7, 2025

stmontgomery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aroben commented Mar 11, 2025 •

edited

Loading

aroben commented Apr 7, 2025 •

edited

Loading

aroben Apr 7, 2025 •

edited

Loading