-
Notifications
You must be signed in to change notification settings - Fork 465
Add support for Swift Testing in SwiftSyntaxMacrosTestsSupport #3192
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
Open
0xTim
wants to merge
6
commits into
swiftlang:main
Choose a base branch
from
0xTim:support-swift-testing
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
2ddc19b
Record issues if we're in a Swift testing environment
0xTim ba15ee0
Add support in other method
0xTim dff2179
Add tests
0xTim d7fd8ff
Private imports
0xTim 8f55686
Fix some of the CI issues
0xTim be537f1
Fix formatting
0xTim File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
61 changes: 61 additions & 0 deletions
61
Tests/SwiftSyntaxMacroExpansionTest/SwiftTestingTests.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors | ||
| // Licensed under Apache License v2.0 with Runtime Library Exception | ||
| // | ||
| // See https://swift.org/LICENSE.txt for license information | ||
| // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #if canImport(Testing) | ||
| import Testing | ||
| import SwiftSyntaxMacrosTestSupport | ||
|
|
||
| @Suite("Swift Testing Macro Expansion Tests") | ||
| struct SwiftTestingMacroExpansionTests { | ||
| @Test("Test Happy Path") | ||
| func testHappyPathWorks() { | ||
| assertMacroExpansion( | ||
| """ | ||
| @constantOne | ||
| var x: Int /*1*/ // hello | ||
| """, | ||
| expandedSource: """ | ||
| var x: Int { /*1*/ // hello | ||
| get { | ||
| return 1 | ||
| } | ||
| } | ||
| """, | ||
| macros: ["constantOne": ConstantOneGetter.self], | ||
| indentationWidth: .spaces(2) | ||
| ) | ||
| } | ||
|
|
||
| @Test("Test Failure") | ||
| func failureReportedCorrectly() { | ||
| withKnownIssue { | ||
| assertMacroExpansion( | ||
| """ | ||
| @constantOne | ||
| var x: Int /*1*/ // hello | ||
| """, | ||
| expandedSource: """ | ||
| var x: Int { /*1*/ // hello | ||
| get { | ||
| return 1 | ||
| } | ||
| } | ||
| """, | ||
| macros: ["constantOne": ConstantOneGetter.self], | ||
| indentationWidth: .spaces(4) | ||
| ) | ||
| } matching: { issue in | ||
| issue.description.contains("Macro expansion did not produce the expected expanded source") | ||
| } | ||
| } | ||
| } | ||
| #endif |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the correct way? Should we split it out into a
expectfunction instead?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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.currentissue - 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 theassertMacroExpansionfrom 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.Uh oh!
There was an error while loading. Please reload this page.
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.
@stmontgomery Your take? You okay with this presumably being nonfunctional with the package build?
Let's at least document it as unsupported in the symbol's Markup?
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.
First of all, thank you @0xTim for taking a stab at this. You have essentially beaten us to the punch; I was anticipating that one of us who contribute to Swift Testing would eventually propose a similar change to what you have here once @jerryjrchen's interoperability work had all landed. But yes, I do see some advantage to proceeding with this interim step forward now, even while I acknowledge @grynspan's point about it not working for detached tasks.
My understanding is that right now, since interoperability is neither available yet nor an approved feature, if we land this PR and a user has a dependency on both the swift-syntax and swift-testing packages, then what will happen is that the
SwiftSyntaxMacrosTestsSupportmodule from their swift-syntax build will import and link the copy of theTestingmodule from the toolchain they are building with. However, since they have a copy of swift-testing in the package graph as well, the user's own tests will import and link the copy of theTestingmodule from that package dependency, ignoring the toolchain copy. As a result, the new call to#expectinSwiftSyntaxMacrosTestsSupportwill interact with the "wrong" copy of theTestingmodule (the toolchain copy) and will effectively be a no-op because the toolchain copy won't have any active tests running, only the package copy will. Therefore, in this scenario, the fact that a user has a package dependency on swift-testing causes this fix to not have the desired effect, but it also is no worse than the status quo.Assuming that understanding is correct (and please correct me if not), I think it's not an ideal situation but also not worrisome enough in practice to avoid proceeding with this change. And the good news is that if/when interoperability does land, it should solve for this because it will centralize things to ensure that there is always only at most one "fallback handler" in each process.
I think this is definitely a good suggestion:
And I also think we should find a place to document the special considerations and pitfalls when you have both swift-syntax and swift-testing package dependencies. In general, we greatly encourage using Swift Testing from the toolchain and this is one reason why.
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.
Akshully, this could go even worse on non-Apple platforms (no two-level linker namespace) and result in duplicate symbol errors at link time.
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.
That's a good callout. And I suppose any testing library1 vended by any package would have that problem if it uses Swift Testing, assuming the package doesn't declare its own dependency on swift-testing and relies on the toolchain copy instead. I don't think problem is unique to swift-syntax, in other words, and IMO we shouldn't recommend that packages avoid using Swift Testing in test-helper libraries due to this problem.
So if we proceed with this change, it would probably be best to communicate to clients of swift-syntax that if they use
SwiftSyntaxMacrosTestsSupportwith Swift Testing tests, they should be sure to use the toolchain copy and not the swift-testing package. (Perhaps that could go in the release notes?) And if they insist on using the swift-testing package copy, they can instead use the "generic" test library (SwiftSyntaxMacrosGenericTestSupport). That would allow them to provide their own handler function, which can use their package dependency-provided copy of#expect.Footnotes
Note that the concept of a 'testing library' is not yet formally defined in Swift package manager, but I mean any library product in a package which imports
Testing. See also: swiftlang/swift-package-manager#9343 ↩