Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SWT-NNNN]: Improving the API for third parties to record issues #532

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+126 −0
Open

[SWT-NNNN]: Improving the API for third parties to record issues #532

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03a224f
Proposal: Improving the API for third parties to record issues
younata Jul 12, 2024
b3ec755
Incorporate early rounds of feedback
younata Jul 12, 2024
f4855c9
Bikeshedding and other changes
younata Jul 20, 2024
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
123 changes: 123 additions & 0 deletions Documentation/Proposals/NNNN-third-party-issues.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
# 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
}

extension Issue.Kind {
public protocol ToolContext: Sendable, Encodable {
var toolName: String { get }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a default implementation of this property that supplies the module name. 🤔

}
}
```

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
Copy link
Contributor

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 ToolContext or recordedByTool or 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.)

Copy link
Author

@younata younata Jul 12, 2024 •
edited
Loading

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:

ToolContext isn't bad, but after more thought, I think that Metadata is a better suffix than Context. Regardless, I think that ToolMetadata is 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.Metadata does 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 ToolContext should have a different name. Perhaps also, to better express the intent that this is metadata, we should rename the toolName property to something like toolDescription? That gives the indication that this is meant more for human-readable display (under the assumption that developers have internalized -description properties like description and debugDescription are dynamically generated for human consumption?). Which also helps make it obvious that they can also have their ToolContext type contain other properties if they want. Additionally, the documentation for ToolContext should also explicitly state that it the ToolContext will 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. recordedByTool isn'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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metadata feels like it's vague enough that we'd come regret it later, even though it's nested inside Issue.Kind. What about a top-level type, ToolMetadata, that isn't specifically bound to Issue.Kind?

If we do grab the tool name from its module, then we don't need toolName at all, FWIW. I don't feel strongly about renaming it.

As for the Issue.Kind case, I still don't have a decent suggestion for something better. recordedByTool isn'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.

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.Kind are certainly descriptive, but in terms of the Swift Testing API that was ultimately used to produce them. For example, #expect produces .expectationFailed, and confirmation {} produces .confirmationMiscounted. So toolAPI() should produce toolSomethinged(). From a test author's point of view, it's mostly moot because they don't typically see an instance of Issue directly, they just see the output in CLI, Xcode, or VSCode.

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also (probably?) possible for us to get the module name of the module that contains the concrete type conforming to ToolContext—that's a little more reliable than a developer-supplied string here, but it's a symbol name rather than a plain English name, so it comes with a tradeoff.

Worth considering, even if it's just for the "alternatives considered" section.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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