-
Notifications
You must be signed in to change notification settings - Fork 92
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
Changes from all commits
8c66cfa
3658356
aa29f74
25ffd91
2f1e47c
4dbf75b
56a6883
e945bad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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() | ||
) {} | ||
|
||
// ... | ||
} | ||
``` | ||
|
||
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` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is intentional that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my alternative suggestion |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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 | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 As one alternative, I wonder if we could expose a 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 |
||
|
||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
/// | ||
|
@@ -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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should leave There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's OK to leave |
||
case system | ||
} | ||
|
||
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 How about we expose a different initializer with the signature: init(
_ kind: Kind,
comments: [Comment] = [],
sourceLocation: SourceLocation = #_sourceLocation
) (Whether or not |
||
kind: Kind, | ||
comments: [Comment], | ||
comments: [Comment] = [], | ||
sourceContext: SourceContext = .init() | ||
) { | ||
self.kind = kind | ||
|
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.
Proposals don't need to include the bodies of methods/initializers/etc. You can leave the squiggly brackets out here.