[SWT-NNNN]: Improving the API for third parties to record issues #532
Conversation
younata
requested review from
stmontgomery,
grynspan,
dennisweissmann,
briancroom,
SeanROlszewski and
suzannaratcliff
as code owners
|
|
||
| ## Detailed design | ||
|
|
||
| `Issue.record` creates an issue of kind `Issue.Kind.recordedByTool`. This is a |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
|
|
||
| ```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 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.
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).
grynspan
added
tools integration
|
|
||
| extension Issue.Kind { | ||
| public protocol ToolContext: Sendable, Encodable { | ||
| var toolName: String { get } |
There was a problem hiding this comment.
We could add a default implementation of this property that supplies the module name. 🤔
- 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.
|
We're not ignoring this proposal, I promise! |
Read the full proposal here
Motivation:
To improve the experience of users using third party assertion tools, we should improve the API for recording Issues so that assertion failures aren't misleadingly described as unconditional failures.
Modifications:
This PR is just the proposal doc. See #513 for a proposed change based on this.
Checklist: