[SWT-NNNN] Make the Issues API public & provide a public `Issue.record` instance method. #481

younata · 2024-06-13T17:24:02Z

See #474 for the related bug.

Read the full proposal here!

Motivation

To better facilitate integrating Swift Testing with third party assertion tools, Swift Testing should allow public access to creating and recording Issues directly.

Modifications:

At this stage, the PR is just the pitch doc, and I'm more than happy to contribute the small change as we discuss it. I figured it would be better to agree on the pitch first?

Result:

Once merged, this will open up third-party assertion tools to better integrate with Swift Testing.

Checklist:

Code and documentation should follow the style of the Style Guide.

…scoped to just making the issues api public

grynspan · 2024-06-13T17:54:58Z

One minor thing that worries me is that this allows creating issues of type .system which is meant to be reserved by the testing library. We can relax that definition, but I'm not sure we want to?

As well, should we consider deprecating the static record() functions? They exist to allow recording arbitrary issues without exposing a full initializer, so they're redundant with this change.

stmontgomery · 2024-06-13T18:00:34Z

Documentation/Proposals/NNNN-public-issues-api.md

+  public func record() -> Self {
+    record(configuration: nil)
+  }


For the purposes of a proposal, you can omit the body of the method

stmontgomery · 2024-06-13T18:01:27Z

Documentation/Proposals/NNNN-public-issues-api.md

+## Integration with supporting tools
+
+Third-party assertion tools would be able to directly create an `Issue`
+and then report it using the `Issue.report()` instance method. `Issue.report()`


These say report() but the actual proposed method is named record(), can you update those to be consistent throughout the document?

Documentation/Proposals/NNNN-public-issues-api.md

stmontgomery · 2024-06-13T18:13:13Z

One thing which is likely to pose a challenge with using this API as proposed is that other related types don't yet expose a public initializer, either. Perhaps most important is the Expectation type. It's used as an associated value of the Issue.Kind.expectationFailed(_ expectation: Expectation) enum case.

In order to correctly represent an expectation failure, a client would need to first programmatically construct an Expectation, which implies that more APIs may need to be exposed than just the ones mentioned here. One approach could be to start conservatively and identify only a small subset of the Expectation API which is likely needed to express basic expectation failures, and not attempt to include as much detail as the built-in #expect does.

I'd recommend looking through the Issue.Kind enum and evaluating how each one might be used, or whether it should be used at all (as @grynspan mentioned, .system doesn't seem relevant).

Co-authored-by: Stuart Montgomery <smontgomery@apple.com>

younata · 2024-06-13T21:20:10Z

One thing which is likely to pose a challenge with using this API as proposed is that other related types don't yet expose a public initializer, either. Perhaps most important is the Expectation type. It's used as an associated value of the Issue.Kind.expectationFailed(_ expectation: Expectation) enum case.

Thanks for the review!

I think a decent next-steps for me would be to go and implement this in my branch, and check out what needs to be modified to actually do this.

- Makes the Issue initializer public, with a slight cleanup. - Creates the Issue.record() instance method, which just calls out to Issue.record(configuration:) - Marks the Issue.Kind.system case as only available for tools integrations. - Issue.record(configuration:) no longer has a default value of nil, to avoid confusion with the new Issue.record() methods. - Creates a public initializer for Expectation, which creates an empty Expression - Adds an explicit internal initializer for Expectation, where we can pass in Expressions - Allow third parties to read & write to the Expectation.mismatchedErrorDescription and Expectation.differenceDescription properties

younata · 2024-06-14T17:09:51Z

Update: I added an implementation, which I used with Nimble to actually create an integration.

You can check out the basic-swift-testing-support branch on Nimble if you'd like to try it out. Both the xcodeproj and the Package.swift use this branch of swift-testing. Run the SwiftTestingSupportSuite to verify that the error is properly recorded in Swift Testing.

grynspan · 2024-06-14T18:32:50Z

Sources/Testing/Expectations/Expectation.swift

@@ -18,7 +18,6 @@ public struct Expectation: Sendable {
  ///
  /// If this expectation passed, the value of this property is `nil` because no
  /// error mismatch occurred.
-  @_spi(ForToolsIntegrationOnly)


This property should probably stay SPI for now as it's a bit hacky and probably not the final design for this data.

grynspan · 2024-06-14T18:32:53Z

Sources/Testing/Expectations/Expectation.swift

@@ -27,7 +26,6 @@ public struct Expectation: Sendable {
  /// If this expectation passed, the value of this property is `nil` because
  /// the difference is only computed when necessary to assist with diagnosing
  /// test failures.
-  @_spi(ForToolsIntegrationOnly)


This property should probably stay SPI for now as it's a bit hacky and probably not the final design for this data.

grynspan · 2024-06-14T18:34:31Z

Sources/Testing/Issues/Issue+Recording.swift

@@ -54,6 +54,15 @@ extension Issue {
    return issue.record(configuration: configuration)
  }

+  /// Record this issue by wrapping it in an ``Event`` and passing it to the


I don't think we really discuss events or event handlers in our API documentation because they're not otherwise part of our API surface. How about simply:

/// Record this issue.

grynspan · 2024-06-14T18:34:49Z

Sources/Testing/Issues/Issue+Recording.swift

+  /// Record this issue by wrapping it in an ``Event`` and passing it to the
+  /// current event handler.
+  ///
+  /// - Returns: The issue that was recorded (`self` or a modified copy of it.)


We should make sure the API documentation explains what might be modified (the SPI documentation is, and this is a technical term here, "lazy".)

grynspan · 2024-06-14T18:35:56Z

Sources/Testing/Issues/Issue.swift

@@ -65,6 +65,7 @@ public struct Issue: Sendable {

    /// An issue due to a failure in the underlying system, not due to a failure
    /// within the tests being run.
+    @_spi(ForToolsIntegrationOnly)


We should leave .system as SPI for now since we don't generally want external components to generate system issues. These represent problems with Swift Testing itself. (We could maybe add an additional case e.g. case integratedToolFailed(description: String), what do you think @stmontgomery ?)

Just so I understand, you're saying this line is correct? Or is there a more-correct @_spi annotation than ForToolsIntegrationOnly? The SPI doc only lists Experimental and ForToolsIntegrationOnly.

Yes, adding the SPI marker here is probably the right thing to do, but I'd like @stmontgomery to chime in too.

I think it's OK to leave .system without an @_spi attribute. Although it's true that users shouldn't generally record system issues themselves, I do think it can be useful for them to match against issues of that kind using other APIs like withKnownIssue. So personally, I don't think we need to add it as part of this proposal/PR.

grynspan · 2024-06-14T18:38:01Z

Sources/Testing/Issues/Issue.swift

@@ -93,9 +94,9 @@ public struct Issue: Sendable {
  ///     occurred. This defaults to a default source context returned by
  ///     calling ``SourceContext/init(backtrace:sourceLocation:)`` with zero
  ///     arguments.
-  init(
+  public init(


This is fine but will produce a source context for the issue that points into Swift Testing rather than the call site, which is an oversight of the existing code to be fair. As well, we're contemplating lowering Backtrace and SourceContext to SPI since the Swift standard library has its own backtrace type now that we'll eventually want to adopt and we don't want a naming conflict.

How about we expose a different initializer with the signature:

init( _ kind: Kind, comments: [Comment] = [], sourceLocation: SourceLocation = #_sourceLocation )

(Whether or not kind deserves a label is something we can bikeshed.)

grynspan · 2024-06-14T18:39:21Z

Documentation/Proposals/NNNN-public-issues-api.md

+    kind: Kind,
+    comments: [Comment] = [],
+    sourceContext: SourceContext = .init()
+  ) {}


Proposals don't need to include the bodies of methods/initializers/etc. You can leave the squiggly brackets out here.

grynspan · 2024-06-14T18:40:22Z

Documentation/Proposals/NNNN-public-issues-api.md

+}
+```
+
+Furthermore, to support passing in any `Issue.Kind` to `Issue`, `Expectation`


It is intentional that Expectation does not have a public initializer: it is always supposed to be the product of #expect or #require. If you record an issue that does not come from one of those macros, it should not be producing an instance of this type.

See my alternative suggestion

grynspan · 2024-06-14T18:41:43Z

Documentation/Proposals/NNNN-public-issues-api.md

+
+## Acknowledgments
+
+I'd like to thank [Jonathon Grynspan](https://github.com/grynspan) and


Jonathan. :)

stmontgomery · 2024-06-28T03:52:20Z

Sources/Testing/Expectations/Expectation.swift

+    self.isPassing = isPassing
+    self.isRequired = isRequired
+    self.sourceLocation = sourceLocation
+  }


I'd like to explore ideas for ways to structure this that avoid needing to publicly expose an init on Expectation. I know I brought that idea up in an earlier comment on this PR, but after reviewing this type again, and taking @grynspan 's comments about the two properties above into consideration, I'm wary to open this type up for general API use right now until it's a bit better designed.

As one alternative, I wonder if we could expose a static func on Issue.Kind which takes a generic message, as well as one or more values of the evaluated expression, and internally construct an Expectation with the relevant Expression and Expression.Value values representing all of the passed-in values? Something like

extension Issue.Kind { public static func expectationFailed<each T>( _ description: String, values: repeat each T, isRequired: Bool, sourceLocation: SourceLocation ) -> Self }

This would require some internal change to __Expression but that seems doable, and it would allow capturing rich details of the left- and right-hand side values of binary expressions.

grynspan · 2024-06-29T13:48:16Z

If we were to proceed with #490, would we still need this API change?

younata · 2024-07-01T15:27:00Z

If we were to proceed with #490, would we still need this API change?

No, this PR can be closed if we proceed with #490, which solves this with less code required of third-party tools.

grynspan · 2024-07-08T16:35:01Z

No, this PR can be closed if we proceed with #490, which solves this with less code required of third-party tools.

With that in mind, I'd like to suggest we close out this PR as well as #474, and focus on that PR. (If you agree that it's an approach that works for you.) We'll want to do an API PR and forum post for that one too, if you're interested in writing it?

younata · 2024-07-09T15:21:37Z

With that in mind, I'd like to suggest we close out this PR as well as #474, and focus on that PR. (If you agree that it's an approach that works for you.) We'll want to do an API PR and forum post for that one too, if you're interested in writing it?

Ok, sure. I'll take that on.

younata added 2 commits June 12, 2024 22:57

Rough draft of creating a public issues api

8c66cfa

expand a bit more on this proposal, and provide reasons to keep this …

3658356

…scoped to just making the issues api public

grynspan assigned younata Jun 13, 2024

grynspan added enhancement New feature or request public-api Affects public API api-proposal API proposal PRs (documentation only) labels Jun 13, 2024

stmontgomery reviewed Jun 13, 2024

View reviewed changes

Documentation/Proposals/NNNN-public-issues-api.md Outdated Show resolved Hide resolved