Skip to content

Implement a version of SwiftSyntaxMacrosTestsSupport that is framework agnostic #2647

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 16, 2024
Merged

Implement a version of SwiftSyntaxMacrosTestsSupport that is framework agnostic #2647

merged 7 commits into from
May 16, 2024

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented May 7, 2024

SwiftSyntaxMacrosTestsSupport does not depend on XCTest or Foundation can be used to write macro tests using swift-testing.

I am not quite happy with the name SwiftSyntaxMacrosTestsSupportFrameworkAgnostic yet but haven’t come up with one that’s significantly better. I’m open to suggestions.

rdar://119519713

@grynspan Could you check if SwiftSyntaxMacrosTestsSupportFrameworkAgnostic works to write macro tests using swift-testing?

@ahoppen
Move assertStringsEqualWithDiff into a module that doesn’t depend o…
0b890bb 
…n Foundation or XCTest

This allows us to use these functions from a framework-agnostic `SwiftSyntaxMacroTestSupport` module.

All changes are in underscored modules and thus don’t have any API impact.
@ahoppen ahoppen requested a review from bnbarham as a code owner May 7, 2024 18:38
@ahoppen
Copy link
Member Author

ahoppen commented May 7, 2024

@swift-ci Please test

@grynspan
Copy link
Contributor

grynspan commented May 7, 2024

Happy to help—can you clarify what you need from me?

@ahoppen ahoppen force-pushed the framework-agnostic-tests branch from fe0773e to 2b3d748 Compare May 7, 2024 19:03
@ahoppen
Copy link
Member Author

ahoppen commented May 7, 2024 •
edited
Loading

It would be good to get some confirmation that this change actually allows writing tests for macros using swift-testing.

@ahoppen
Copy link
Member Author

ahoppen commented May 7, 2024

@swift-ci Please test

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented May 7, 2024

@swift-ci Please test

@grynspan
Copy link
Contributor

grynspan commented May 7, 2024

It would be good to get some confirmation that this change actually allows writing tests for macros using swift-testing.

Looks like it ought to work, although I noticed it seems a bit picky about whitespace not matching.

I'm getting this warning:

/Volumes/Dev/Source/swift-testing/.build/checkouts/swift-syntax/Sources/SwiftSyntaxMacrosTestSupportFrameworkAgnostic/Assertions.swift:21:8: warning: public import of 'SwiftSyntaxMacros' was not used in public declarations or inlinable code
19 | public import SwiftSyntax
20 | public import SwiftSyntaxMacroExpansion
21 | public import SwiftSyntaxMacros
| `- warning: public import of 'SwiftSyntaxMacros' was not used in public declarations or inlinable code
22 | private import _SwiftSyntaxTestSupportFrameworkAgnostic
23 | #else

I don't think this is a great interface (this is partially copied from my previous PR, I assume?) but short of adding macro-aware overloads of #expect() to swift-testing, I guess it's the best we can do.

@ahoppen ahoppen force-pushed the framework-agnostic-tests branch 2 times, most recently from dc4c0ab to beb322f Compare May 7, 2024 23:32
@ahoppen
Copy link
Member Author

ahoppen commented May 7, 2024

@swift-ci Please test

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented May 7, 2024

@swift-ci Please test

rintaro
rintaro reviewed May 7, 2024
View reviewed changes
Sources/SwiftSyntaxMacrosTestSupportFrameworkAgnostic/Assertions.swift Outdated
}

/// Defines the details of a test failure, consisting of a message and the location at which the l
public struct TestFailureSpec {
Copy link
Member

Choose a reason for hiding this comment

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

Have you consider throwing this as an Error and catch let error as TestFailureSpec {?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that we could only throw a single failure and not multiple.

@ahoppen
Copy link
Member Author

ahoppen commented May 8, 2024

Note to self: A suggestion for the module name SwiftSyntaxMacrosGenericTestSupport instead of SwiftSyntaxMacrosTestSupportFrameworkAgnostic.

@grynspan
Copy link
Contributor

grynspan commented May 8, 2024

If we eventually wanted to make it a cross-module overlay, I think the idiomatic name would be _Testing_SwiftSyntaxMacros, I think. (That's not a good name, just a thought.)

ahoppen added 2 commits May 8, 2024 15:43
@ahoppen
Make unsigned and static properties in TestFailureLocation SPI
7c11953 
Instead, expose `String` and `Int` variants publicly that can be passed directly in to `Issue.record` from `swift-testing`.
@ahoppen
Rename module from FrameworkAgnostic to Generic
537e1a2
@ahoppen ahoppen force-pushed the framework-agnostic-tests branch from 1715ac7 to 537e1a2 Compare May 8, 2024 22:43
@ahoppen
Copy link
Member Author

ahoppen commented May 8, 2024

@swift-ci Please test

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented May 9, 2024

@swift-ci Please test

rintaro
rintaro approved these changes May 10, 2024
View reviewed changes
@ahoppen
Copy link
Member Author

ahoppen commented May 13, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented May 13, 2024

@swift-ci Please test Windows

bnbarham
bnbarham approved these changes May 16, 2024
View reviewed changes
Sources/SwiftSyntaxMacrosGenericTestSupport/Assertions.swift
/// Defines the location at which the a test failure should be anchored. This is typically the location where the
/// assertion function is called.
public struct TestFailureLocation {
@_spi(XCTestFailureLocation) public let staticFileID: StaticString
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you purposefully not updating the SPI name, or is it just a left over from the move?

Copy link
Contributor

@grynspan grynspan May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These symbols are only used when using the existing XCTest-based functions, and are used internally in those functions. The ones used by a caller using swift-testing use the normalized types (String and Int) instead of the types used by XCTFail() (StaticString and UInt.) This was an intentional decision. :)

@ahoppen ahoppen merged commit 8a4d12e into swiftlang:main May 16, 2024
3 checks passed
@ahoppen ahoppen deleted the framework-agnostic-tests branch May 16, 2024 22:43
@ahoppen ahoppen mentioned this pull request May 16, 2024
Merged
@ahoppen ahoppen mentioned this pull request Jun 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rintaro rintaro rintaro approved these changes

@bnbarham bnbarham bnbarham approved these changes

@hamishknight hamishknight hamishknight approved these changes

@grynspan grynspan Awaiting requested review from grynspan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ahoppen @grynspan @rintaro @bnbarham @hamishknight