Add support for Swift Testing in SwiftSyntaxMacrosTestsSupport #3192
Conversation
0xTim
requested review from
ahoppen,
bnbarham,
hamishknight and
rintaro
as code owners
| buildConfiguration: buildConfiguration, | ||
| failureHandler: { | ||
| #if canImport(Testing) | ||
| if Test.current != nil { |
There was a problem hiding this comment.
This is not the correct test to determine which library is in use because code can run in a detached task. See swiftlang/swift-testing#475
There was a problem hiding this comment.
What is the correct way? Should we split it out into a expect function instead?
There was a problem hiding this comment.
There is no correct way at this time, which is why that issue is still open. Jerry's work should allow us to just call #expect() here and have it work under all configurations.
There was a problem hiding this comment.
The pitch looks like it will solve the issue, but still require work in the library to migrate over to Swift Testing APIs. What I propose is that we land this now, as it solves a problem that exists for users today (and potentially provide a release in the next monthly Linux release/Swift patch release) and then fix forward when the proposal lands. Given it's still in the pitch phase it likely won't be landed until 6.4 and waiting 10 months for a solution seems like a bad idea.
Regarding the Test.current issue - from my understanding this works in all instances apart from those running in a detached task. For this specific case, I can't see a scenario when a user would be using the assertMacroExpansion from a detached task so we can fix this for the majority of the users and those attempting to use it from a detached task will see no change in behaviour.
There was a problem hiding this comment.
@stmontgomery Your take? You okay with this presumably being nonfunctional with the package build?
For this specific case, I can't see a scenario when a user would be using the assertMacroExpansion from a detached task so we can fix this for the majority of the users and those attempting to use it from a detached task will see no change in behaviour.
Let's at least document it as unsupported in the symbol's Markup?
There was a problem hiding this comment.
If we can agree here that
SwiftSyntaxcan provideTestingsupport by depending on the toolchain'sTestingmodule can we unblock the PR on this matter?
I'd be morally okay with saying "the features in the swift-syntax repository are dependent on the built-in copy of Swift Testing even if you include a package dependency" however this will break builds on non-Apple platforms with flat linker namespaces due to duplicate symbols at link time.
I think there is still the larger question on how this is currently implemented. The
if Test.current != nilcheck in this method is certainly less than ideal. Instead I would propose that we add an entirely new method calledexpectMacroExpansionthat is based onTestingunconditionally. So existing users ofassertMacroExpansioncan continue to use it withXCTestandTestingusers can start adopting the new one. How does that sound to everyone?
Once @jerryjrchen's work on the interop feature lands, it will be possible to implement this in a way that depends on neither XCTest nor Swift Testing. It may be a better idea to just wait until that work is done and revisit the problem at that point.
There was a problem hiding this comment.
I'd be morally okay with saying "the features in the swift-syntax repository are dependent on the built-in copy of Swift Testing even if you include a package dependency" however this will break builds on non-Apple platforms with flat linker namespaces due to duplicate symbols at link time.
Only if there is both Testing from the toolchain and from the package right? Which I thought we agreed is only really valid in development environments.
There was a problem hiding this comment.
Only if there is both Testing from the toolchain and from the package right? Which I thought we agreed is only really valid in development environments.
All testing environments are development environments.
If swift-syntax explicitly links the copy of Swift Testing in the toolchain, that will break developers who want to test their macros and have a package dependency on Swift Testing.
In addition, if they are using the package copy of Swift Testing and you are using the toolchain's copy, your calls to e.g. #expect() won't be routed to the infrastructure the developer's test target links against, so a failure will be invisible.
Jerry's work should give us an escape hatch for this problem, so we should wait until it lands and then make the necessary changes here.
There was a problem hiding this comment.
My concern with waiting is that it could be a year until that's shipped, meanwhile users have no indications their tests are passing incorrectly, whereas we could fix it today for the majority of use cases
There was a problem hiding this comment.
I appreciate the concern, but there are technical blockers here. We must not cause build failures for teams using Swift Testing as a package.
4 participants
As described in Adopt Swift-Testing in test utils such as
SwiftSyntaxMacrosTestSupport#2720SwiftSyntaxMacrosTestsSupportdoes not work with Swift Testing, which is an issue given Swift Testing is the way to test projects and XCTest is deprecated. If produces false positives (where tests pass even if they shouldn't) which is a major issue, especially as there are no warnings.This PR adds support for Swift Testing so that tests fail correctly. This does not introduce an issue with a circular dependency on Swift Testing. There is a circular dependency at the package level, but this is allowed due to swiftlang/swift-package-manager#7530. There is no circular dependency between targets.
SwiftSyntaxMacrosTestsSupportSwiftSyntaxMacrosTestSupport#2720