[SWT-NNNN]: Improving the API for third parties to record issues #532
+126
−0
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
younata marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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" | ||
|
There was a problem hiding this comment. It is also (probably?) possible for us to get the module name of the module that contains the concrete type conforming to Worth considering, even if it's just for the "alternatives considered" section. There was a problem hiding this comment. That's a very good point that I didn't even think about. I added it to the alternatives considered section, because toolName (and other custom metadata, yes) might also include other metadata that would be either very difficult or impossible to determine (I used version info as an example, because I don't think there's a way for packages to infer what their version string is). |
||
| } | ||
|
|
||
| // ... | ||
| 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 | ||
younata marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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. | ||
younata marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| 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. | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, this is an opportunity for you to refine the names of the new symbols we're introducing here. If you hate
ToolContextorrecordedByToolor what-have-you, let's discuss.(That's not to say you must change names, just that the names we've used in my draft PR don't need to be the final ones if you think we can improve on them.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, and I didn't do that because I was more focused on just writing the pitch.
I'll do some of my thinking on that here, and later refine I can that in to the pitch doc:
ToolContextisn't bad, but after more thought, I think thatMetadatais a better suffix thanContext. Regardless, I think thatToolMetadatais a better description of what's expected.As for the Tool Prefix, I'm pretty meh about it. I wonder if maybe dropping the prefix entirely is better?
Issue.Kind.Metadatadoes a decent-enough job of namespacing. It is a bit weird that a type named "Metadata" is only intended to be used by third party tools though.Similarly, I wonder if the actual contents of
ToolContextshould have a different name. Perhaps also, to better express the intent that this is metadata, we should rename thetoolNameproperty to something liketoolDescription? That gives the indication that this is meant more for human-readable display (under the assumption that developers have internalized -description properties likedescriptionanddebugDescriptionare dynamically generated for human consumption?). Which also helps make it obvious that they can also have theirToolContexttype contain other properties if they want. Additionally, the documentation forToolContextshould also explicitly state that it theToolContextwill be included in the json stream from Swift Testing.As for the Issue.Kind case, I still don't have a decent suggestion for something better.
recordedByToolisn't bad, per-se, but it describes where the issue came from, not really what kind of issue it is (this might be a distinction without a difference). More thought on this is required.I don't hate your suggestions, and I think they're really useful for helping to move the conversation along!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metadatafeels like it's vague enough that we'd come regret it later, even though it's nested insideIssue.Kind. What about a top-level type,ToolMetadata, that isn't specifically bound toIssue.Kind?If we do grab the tool name from its module, then we don't need
toolNameat all, FWIW. I don't feel strongly about renaming it.The hard part here is that Swift Testing itself hasn't got a clue what kind of issue something is. The existing cases of
Issue.Kindare certainly descriptive, but in terms of the Swift Testing API that was ultimately used to produce them. For example,#expectproduces.expectationFailed, andconfirmation {}produces.confirmationMiscounted. SotoolAPI()should producetoolSomethinged(). From a test author's point of view, it's mostly moot because they don't typically see an instance ofIssuedirectly, they just see the output in CLI, Xcode, or VSCode.