From 03a224fc98b2e98dde93879f94b0233ea19a661f Mon Sep 17 00:00:00 2001 From: Rachel Brindle Date: Fri, 12 Jul 2024 08:49:26 -0700 Subject: [PATCH 1/3] Proposal: Improving the API for third parties to record issues --- .../Proposals/NNNN-third-party-issues.md | 108 ++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 Documentation/Proposals/NNNN-third-party-issues.md diff --git a/Documentation/Proposals/NNNN-third-party-issues.md b/Documentation/Proposals/NNNN-third-party-issues.md new file mode 100644 index 000000000..7b0f57630 --- /dev/null +++ b/Documentation/Proposals/NNNN-third-party-issues.md @@ -0,0 +1,108 @@ +# Improve robustness of recording Issues + +* Proposal: [SWT-NNNN](NNNN-third-party-issues.md) +* Authors: [Rachel Brindle](https://github.com/younata), [Jonathan Grynspan](https://github.com/grynspan) +* Status: **Awaiting review** +* Bug: [apple/swift-testing#490](https://github.com/apple/swift-testing/issues/490) +* Implementation: [apple/swift-testing#513](https://github.com/apple/swift-testing/pull/513) +* Review: [To be added] + +## Introduction + +Integration with third party tools is important for the success of Swift Testing +and the ecosystem as a whole. To support this, Swift Testing should offer tools +more control over how custom issues are recorded and what is shown. + +## Motivation + +There are several third-party assertion libraries that developers are used to +using, and Swift Testing should make it as easy as possible for the developers +and maintainers of these third-party assertion libraries to integrate with +Swift Testing. + +Swift Testing currently offers the `Issue.record` family of static methods, +which provides an acceptable interface for one-off/custom assertions without +using the `#expect` or `#require` macros. Unfortunately, the public +`Issue.record` static method does not offer a way to precisely control the +error information shown to the user - all Issues are recorded as "unconditional +failure", which is false if the Issue actually happened as the result of an +assertion failing. + +## Proposed solution + +We create a new public `Issue.record` static method, specifically to record and +display only the information given to it: + +```swift +extension Issue { + @discardableResult public static func record( + _ comment: Comment? = nil, + context toolContext: some Issue.Kind.ToolContext, + sourceLocation: SourceLocation = #_sourceLocation + ) -> Self +} +``` + +This method is then used by tools to provide a comment of what happened, as well +as information for the user to know what tool actually recorded the issue. + +## Detailed design + +`Issue.record` creates an issue of kind `Issue.Kind.recordedByTool`. This is a +new case specifically for this API, and serves to hold on to the +`Issue.Kind.ToolContext`. `ToolContext` is a protocol specifically to allow +test tool authors to provide information about the tool that produced the issue, +such as the name of the tool. + +When displaying the Issue to the console/IDE, only the comment and toolContext +are used to generate the failure information: + +```swift +struct MyToolContext: Issue.Kind.ToolContext { + let toolName = "Sample Tool" +} + +// ... +Issue.record("an example issue", context: MyToolContext()) +// "an example issue (from 'Sample Tool')" would be shown to the user. +``` + +## Source compatibility + +This is entirely additive. All existing code will continue to work. + +## Integration with supporting tools + +Tool integrating with the testing library need to create an object +conforming to `Issue.Kind.ToolContext` and use that with `Issue.record` to +record an issue with an arbitrary message. + +But if they wish, they can still use the existing `Issue.record` API to record +unconditional failures. + +## Future directions + +Third-party assertion tools that already integrate with XCTest also need a +more robust API for detecting whether to report an assertion failure to XCTest +or Swift Testing. See [#475](https://github.com/apple/swift-testing/issues/475), +[apple/swift-corelibs-xctest#491](https://github.com/apple/swift-corelibs-xctest/issues/491), +and FB14167426 for issues related to that. Detecting the test runner is a +separate enough concern that it should not be part of this proposal. + +## Alternatives considered + +We could do nothing and require third party tools to use the existing +`Issue.record` API. However, this results in a subpar experience for developers +wishing to use those third party tools. + +This proposal came out of discussion around a [previous, similar proposal](https://github.com/apple/swift-testing/pull/481) +to open up the `Issue` API and allowing arbitrary `Issue` instances to be +recorded. That proposal was dropped in favor of this one, which is significantly +simpler and opens up as little API surface as possible. + +## Acknowledgments + +I'd like to thank [Stuart Montgomery](https://github.com/stmontgomery) for his +help and insight leading to this proposal. Jonathan Grynspan is already +listed as an author for his role creating the implementation, but I also want to +explicitly thank him for his help and insight as well. From b3ec755936d1345c0ba5880b4a0e9e6e9cd2bbd1 Mon Sep 17 00:00:00 2001 From: Rachel Brindle Date: Fri, 12 Jul 2024 11:37:50 -0700 Subject: [PATCH 2/3] Incorporate early rounds of feedback --- .../Proposals/NNNN-third-party-issues.md | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/Documentation/Proposals/NNNN-third-party-issues.md b/Documentation/Proposals/NNNN-third-party-issues.md index 7b0f57630..d53bfe295 100644 --- a/Documentation/Proposals/NNNN-third-party-issues.md +++ b/Documentation/Proposals/NNNN-third-party-issues.md @@ -41,6 +41,12 @@ extension Issue { sourceLocation: SourceLocation = #_sourceLocation ) -> Self } + +extension Issue.Kind { + public protocol ToolContext: Sendable, Encodable { + var toolName: String { get } + } +} ``` This method is then used by tools to provide a comment of what happened, as well @@ -73,9 +79,9 @@ This is entirely additive. All existing code will continue to work. ## Integration with supporting tools -Tool integrating with the testing library need to create an object -conforming to `Issue.Kind.ToolContext` and use that with `Issue.record` to -record an issue with an arbitrary message. +Tool integrating with the testing library need to create a type conforming to +`Issue.Kind.ToolContext` and use that with `Issue.record` to record an issue +with an arbitrary message. But if they wish, they can still use the existing `Issue.record` API to record unconditional failures. @@ -93,13 +99,22 @@ separate enough concern that it should not be part of this proposal. We could do nothing and require third party tools to use the existing `Issue.record` API. However, this results in a subpar experience for developers -wishing to use those third party tools. +wishing to use those third party tools, and that tools can't include any custom +metadata in their issues. This proposal came out of discussion around a [previous, similar proposal](https://github.com/apple/swift-testing/pull/481) to open up the `Issue` API and allowing arbitrary `Issue` instances to be recorded. That proposal was dropped in favor of this one, which is significantly simpler and opens up as little API surface as possible. +It's likely possible for us to get the module name of the module containing the +concrete type conforming to `ToolContext`. Which is more reliable than a +developer-supplied string. However, this also would be a symbolicated string, +which may or may not be possible to demangle into a human-readable name. +Additionally, reading the `toolName` allows developers to provide extra +information that we might not otherwise be able to infer, such as version +data. + ## Acknowledgments I'd like to thank [Stuart Montgomery](https://github.com/stmontgomery) for his From f4855c98925e2e6c613b49d38fe928710ea4e453 Mon Sep 17 00:00:00 2001 From: Rachel Brindle Date: Sat, 20 Jul 2024 08:34:38 -0700 Subject: [PATCH 3/3] Bikeshedding and other changes - Rename Issue.Kind.ToolContext to ToolMetadata and mark it as a top-level protocol - Rename the toolName property of ToolMetadata to toolDescription - Move up the discussion on providing a default implementation of toolDescription to the detailed description section, because that's actually a good idea. - Mark that the 'Recorded by tool' message should be displayed on another line from the actual issue itself. --- .../Proposals/NNNN-third-party-issues.md | 57 ++++++++++--------- 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/Documentation/Proposals/NNNN-third-party-issues.md b/Documentation/Proposals/NNNN-third-party-issues.md index d53bfe295..c294e0f71 100644 --- a/Documentation/Proposals/NNNN-third-party-issues.md +++ b/Documentation/Proposals/NNNN-third-party-issues.md @@ -30,22 +30,21 @@ assertion failing. ## Proposed solution -We create a new public `Issue.record` static method, specifically to record and -display only the information given to it: +We create a new protocol, `ToolMetadata`, as well as a new public `Issue.record` +static method, specifically to record and display only the information given to +it: ```swift extension Issue { @discardableResult public static func record( _ comment: Comment? = nil, - context toolContext: some Issue.Kind.ToolContext, + context toolContext: some ToolMetadata, sourceLocation: SourceLocation = #_sourceLocation ) -> Self } -extension Issue.Kind { - public protocol ToolContext: Sendable, Encodable { - var toolName: String { get } - } +public protocol ToolMetadata: Sendable, Encodable { + var toolDescription: String { get } } ``` @@ -55,33 +54,42 @@ as information for the user to know what tool actually recorded the issue. ## Detailed design `Issue.record` creates an issue of kind `Issue.Kind.recordedByTool`. This is a -new case specifically for this API, and serves to hold on to the -`Issue.Kind.ToolContext`. `ToolContext` is a protocol specifically to allow -test tool authors to provide information about the tool that produced the issue, -such as the name of the tool. +new case specifically for this API, and serves to hold on to the `ToolMetadata`. +`ToolMetadata` is a protocol specifically to allow test tool authors to provide +information about the tool that produced the issue, which they can output to the +console via the `toolDescription` property, or the entire thing can be dumped as +JSON using the Swift Testing ABI. -When displaying the Issue to the console/IDE, only the comment and toolContext -are used to generate the failure information: +When displaying the Issue to the console/IDE, this information is shown to the +user on the next line of the console output. This avoids additional cluttering +of inline issues for IDEs and tools that display the first line of an issue. ```swift -struct MyToolContext: Issue.Kind.ToolContext { - let toolName = "Sample Tool" +struct MyToolMetadata: ToolMetadata { + let toolDescription = "Sample Tool" } // ... Issue.record("an example issue", context: MyToolContext()) -// "an example issue (from 'Sample Tool')" would be shown to the user. +// "an example issue\n(From 'Sample Tool')" would be output to the console. ``` +To simplify the creation of a `ToolMetadata` type, we will also create an +extension to `ToolMetadata` to provide a default value for `toolDescription`. +This default will output the `ToolMetadata`'s module symbol name. This enables +those who wish to provide custom information to do so by specifying +`toolDescription`, while simplifying adoption for those who don't want or +need to provide a custom `toolDescription`. + ## Source compatibility This is entirely additive. All existing code will continue to work. ## Integration with supporting tools -Tool integrating with the testing library need to create a type conforming to -`Issue.Kind.ToolContext` and use that with `Issue.record` to record an issue -with an arbitrary message. +Tools integrating with the testing library need to create a type conforming to +`ToolMetadata` and use that with `Issue.record` to record an issue with an +arbitrary message. But if they wish, they can still use the existing `Issue.record` API to record unconditional failures. @@ -95,6 +103,9 @@ or Swift Testing. See [#475](https://github.com/apple/swift-testing/issues/475), and FB14167426 for issues related to that. Detecting the test runner is a separate enough concern that it should not be part of this proposal. +The `ToolMetadata` protocol can be used in other places intended for integration +with third party tools to allow them to provide metadata on the tool itself. + ## Alternatives considered We could do nothing and require third party tools to use the existing @@ -107,14 +118,6 @@ to open up the `Issue` API and allowing arbitrary `Issue` instances to be recorded. That proposal was dropped in favor of this one, which is significantly simpler and opens up as little API surface as possible. -It's likely possible for us to get the module name of the module containing the -concrete type conforming to `ToolContext`. Which is more reliable than a -developer-supplied string. However, this also would be a symbolicated string, -which may or may not be possible to demangle into a human-readable name. -Additionally, reading the `toolName` allows developers to provide extra -information that we might not otherwise be able to infer, such as version -data. - ## Acknowledgments I'd like to thank [Stuart Montgomery](https://github.com/stmontgomery) for his