-
Notifications
You must be signed in to change notification settings - Fork 47
[chore]: improve Swift Testing support in test utilities #341
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
Conversation
d950991
to
8ed5356
Compare
196fa87
to
d27e738
Compare
@@ -15,6 +15,7 @@ | |||
*/ | |||
|
|||
#if DEBUG | |||
import IssueReporting |
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.
for my education: where is this lib coming from? Apple side?
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.
this is a pointfree library – it's a dependency of the CustomDump lib that we already use: https://github.com/pointfreeco/swift-issue-reporting
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.
though now that you bring it up... i wonder if it should be listed as an explicit dependency in the package?
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.
yup and i see where the magic is coming from: https://github.com/pointfreeco/swift-issue-reporting/blob/main/Sources/IssueReporting/ReportIssue.swift#L71
@@ -65,6 +65,7 @@ let package = Package( | |||
.package(url: "https://github.com/pointfreeco/swift-macro-testing", from: "0.4.0"), | |||
.package(url: "https://github.com/pointfreeco/swift-perception", from: "1.5.0"), | |||
.package(url: "https://github.com/pointfreeco/swift-custom-dump", from: "1.2.1"), | |||
.package(url: "https://github.com/pointfreeco/xctest-dynamic-overlay", from: "1.0.0"), |
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.
we currently already depend on this transitively through 'swift-custom-dump'. added as an explicit dependency because we're directly importing it in some spots now, and i assume that's the 'right thing to do'.
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.
LGTM, better to have another one more familiar with this repo to review though
@@ -62,9 +63,9 @@ extension RenderTester { | |||
guard !workflow.worker.isEquivalent(to: expectedWorker) else { | |||
return | |||
} | |||
XCTFail( | |||
reportIssue( |
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.
There's no Apple-provided way to do this?
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.
by 'this', what do you mean? record an issue in either testing framework? if so, i don't think so, at least not that i could easily find.
migrates test utilities from
XCTest
assertions to analogs provided by theCustomDump
andIssueReporting
libraries. this should fix the issue where assertion failures would be invisible to client code written using Swift Testing rather than XCTest.ideally we should probably add a lint check in the relevant places to enforce use of the appropriate API, but that will be a separate task.
resolves: #330