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.

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

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

Closed
wants to merge 8 commits into from
178 changes: 178 additions & 0 deletions Documentation/Proposals/NNNN-public-issues-api.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
# Public Issues API

* Proposal: [SWT-NNNN](NNNN-public-issues-api.md)
* Authors: [Rachel Brindle](https://github.com/younata)
* Status: **Awaiting review**
* Bug: [apple/swift-testing#474](https://github.com/apple/swift-testing/issues/474)
* Implementation: [apple/swift-testing#481](https://github.com/apple/swift-testing/pull/481)
* Review: n/a

## Introduction

Swift Testing should provide a more robust Issue-reporting API, to facilitate
better integration with third-party assertion tools.

## 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, `Issue.record` is not
robust enough for dedicated assertion libraries like Nimble. For example,
there is no current way to provide an `Issue.Kind` to `Issue.record`.


## Proposed solution

I propose making the initializer for `Issue` public, and provide a public
`Issue.record()` instance method to report that Issue. This provides a clean,
easy to maintain way to create and record custom Issues that assertion tools
are already used to because it is analogous to the XCTIssue API in XCTest.

To help support this, I also propose making a public initializer for
`Expectation`. This public initializer will not take in an `Expression`.
Instead, the public initializer will create an empty `Expression`, in order to
maintain compatibility with the current JSON ABI.

## Detailed design

Create a public initializer for `Issue`, which is not gated by
`@_spi(ForToolsIntegrationOnly)`:

```swift
public struct Issue: Sendable {
// ...

/// Initialize an issue instance with the specified details.
///
/// - Parameters:
/// - kind: The kind of issue this value represents.
/// - comments: An array of comments describing the issue. This array may
/// be empty.
/// - sourceContext: A ``SourceContext`` indicating where and how this
/// issue occurred. This defaults to a default source context returned by
/// calling ``SourceContext/init(backtrace:sourceLocation:)`` with zero
/// arguments.
public init(
kind: Kind,
comments: [Comment] = [],
sourceContext: SourceContext = .init()
) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

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


// ...
}
```

Additionally, create a public `record()` method on Issue, which takes no
arguments, and simply calls the `Issue.record(configuration:)` instance
method with nil:

```swift
extension Issue {
/// 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.)
@discardableResult
public func record() -> Self {}
}
```

This change requires us to eliminate the default value of `nil` for the
`Issue.record(configuration:)` instance method:

```swift
extension Issue {
func record(configuration: Configuration?) -> Self {}
}
```

Furthermore, to support passing in any `Issue.Kind` to `Issue`, `Expectation`
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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

needs to have a public initializer. This initializer will not take in an
`Expression`, but instead directly create an empty `Expression`. To continue
supporting the existing internal API, there will be a second internal
initializer that does take in an `Expression`:

```public struct Expectation: Sendable {
// ...
public init(
mismatchedErrorDescription: String? = nil,
differenceDescription: String? = nil,
isPassing: Bool,
isRequired: Bool,
sourceLocation: SourceLocation
) {}

init(
evaluatedExpression: Expression,
mismatchedErrorDescription: String? = nil,
differenceDescription: String? = nil,
isPassing: Bool,
isRequired: Bool,
sourceLocation: SourceLocation
)
}
```

This also accompanies a change to make the
`Expectation.mismatchedErrorDescription` and
`Expectation.differenceDescription` properties publicly accessible. Which is
done so that third party tools do not need to pass in an `Expression`.

## Source compatibility

This is strictly an additive change. No deletion of code, nothing is being made
private. Only new code and making existing code public.

## Integration with supporting tools

Third-party assertion tools would be able to directly create an `Issue`
and then report it using the `Issue.record()` instance method. `Issue.record()`
would then work similarly to how it does now. This flow is analogous to
reporting an issue in XCTest using the XCTIssue API.

## Future directions

Also necessary for supporting third-party assertion tools is providing a stable
and reliable API for accessing the current test. This somewhat exists with the
current `Test.current` static property, but that returns the
semantically-incorrect value of nil if you access it from a detached task. This
will be defined in a future proposal.

## Alternatives considered

One potential approach is to extend or provide overloads to `Issue.record` that
allows developers to specify every property on an `Issue`. However, this is
undesirable from a maintenance point of view: every time a property is added to
or removed from `Issue`, we would similarly have to update `Issue.record`.
While trivial, having that extra source of truth is mildly annoying.

Another approach discussed in
[apple/swift-testing#474](https://github.com/apple/swift-testing/issues/474)
is making `Issue` public, but tagged with `@_spi(ForToolsIntegrationOnly)`.
Which is also not desirable because that limits third party integrations to
only supporting Swift Testing when managed through Swift Package Manager.
Which means that third party integrations would not be able to support Swift
Testing when managed using Cocoapods, Carthage, or even just by depending on
the raw code. This increases the support burden on those third party tools
because they then have to explain why they only support Swift Testing when
managed through Swift Package Manager.

I also considered combining this with a proposal to provide a more reliable
API for accessing the current test, as mentioned in the Future Directions
section. I decided against this because while that problem is motivation by the
same motivation behind this proposal, it is also a significantly harder
problem. Which is deserving of a full document describing the problem and the
solution. Additionally, the concern behind this proposal is to provide the same
access to issue reporting that `#expect` and `#require` enjoy, while making
accessing the current test more reliable and robust is a separate concern.

## Acknowledgments

I'd like to thank [Jonathon Grynspan](https://github.com/grynspan) and
Copy link
Contributor

Choose a reason for hiding this comment

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

Jonathan. :)

[Stuart Montgomery](https://github.com/stmontgomery) for fielding my Issue
report and encouraging me to contribute more to this community.
34 changes: 32 additions & 2 deletions Sources/Testing/Expectations/Expectation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

public var mismatchedErrorDescription: String?

/// A description of the difference between the operands in the expression
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

public var differenceDescription: String?

/// Whether the expectation passed or failed.
Expand All @@ -41,6 +39,38 @@ public struct Expectation: Sendable {

/// The source location where this expectation was evaluated.
public var sourceLocation: SourceLocation

///
public init(
mismatchedErrorDescription: String? = nil,
differenceDescription: String? = nil,
isPassing: Bool,
isRequired: Bool,
sourceLocation: SourceLocation
) {
self.evaluatedExpression = Expression("")
self.mismatchedErrorDescription = mismatchedErrorDescription
self.differenceDescription = differenceDescription
self.isPassing = isPassing
self.isRequired = isRequired
self.sourceLocation = sourceLocation
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


init(
evaluatedExpression: Expression,
mismatchedErrorDescription: String? = nil,
differenceDescription: String? = nil,
isPassing: Bool,
isRequired: Bool,
sourceLocation: SourceLocation
) {
self.evaluatedExpression = evaluatedExpression
self.mismatchedErrorDescription = mismatchedErrorDescription
self.differenceDescription = differenceDescription
self.isPassing = isPassing
self.isRequired = isRequired
self.sourceLocation = sourceLocation
}
}

/// A type describing an error thrown when an expectation fails during
Expand Down
11 changes: 10 additions & 1 deletion Sources/Testing/Issues/Issue+Recording.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

/// current event handler.
///
/// - Returns: The issue that was recorded (`self` or a modified copy of it.)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

/// Record this issue by wrapping it in an ``Event`` and passing it to the
/// current event handler.
///
Expand All @@ -63,7 +72,7 @@ extension Issue {
///
/// - Returns: The issue that was recorded (`self` or a modified copy of it.)
@discardableResult
func record(configuration: Configuration? = nil) -> Self {
func record(configuration: Configuration?) -> Self {
// If this issue is a caught error of kind SystemError, reinterpret it as a
// testing system issue instead (per the documentation for SystemError.)
if case let .errorCaught(error) = kind, let error = error as? SystemError {
Expand Down
5 changes: 3 additions & 2 deletions Sources/Testing/Issues/Issue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?)

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

case system
}

Expand Down Expand Up @@ -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(
Copy link
Contributor

@grynspan grynspan Jun 14, 2024

Choose a reason for hiding this comment

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

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

kind: Kind,
comments: [Comment],
comments: [Comment] = [],
sourceContext: SourceContext = .init()
) {
self.kind = kind
Expand Down