-
Notifications
You must be signed in to change notification settings - Fork 113
Infer a display name for tests and suites which have a raw identifier name consisting of a single token #1174
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
base: main
Are you sure you want to change the base?
Conversation
@swift-ci please test |
This is not a bug but intentional behaviour, as we discussed previously. It's a change in behaviour for existing test functions/suites and so it needs to be reviewed by the community via the Swift Evolution process. |
Sources/TestingMacros/Support/Additions/FunctionDeclSyntaxAdditions.swift
Outdated
Show resolved
Hide resolved
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.
.
if let namedDecl = declaration.asProtocol((any NamedDeclSyntax).self), | ||
let rawIdentifier = namedDecl.name.rawIdentifier { | ||
if let displayName, let displayNameArgument { | ||
// Disallow having an explicit display name for tests and suites with raw |
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 only actual change you ought to need here outside of unit tests is to change the line:
let rawIdentifier = namedDecl.name.rawIdentifier {
To, I think:
case let nameWithoutBackticks = namedDecl.name.textWithoutBackticks,
nameWithoutBackticks != namedDecl.name.text {
i.e. "if the name had backticks in source, it's a display name regardless of whether the language considers it a raw ID."
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.
diff --git a/Sources/TestingMacros/Support/AttributeDiscovery.swift b/Sources/TestingMacros/Support/AttributeDiscovery.swift
index 3d95df29..83953ff7 100644
--- a/Sources/TestingMacros/Support/AttributeDiscovery.swift
+++ b/Sources/TestingMacros/Support/AttributeDiscovery.swift
@@ -141,11 +141,12 @@ struct AttributeInfo {
// Disallow an explicit display name for tests and suites with raw
// identifier names as it's redundant and potentially confusing.
if let namedDecl = declaration.asProtocol((any NamedDeclSyntax).self),
- let rawIdentifier = namedDecl.name.rawIdentifier {
+ case let nameWithoutBackticks = namedDecl.name.textWithoutBackticks,
+ nameWithoutBackticks != namedDecl.name.text {
if let displayName, let displayNameArgument {
context.diagnose(.declaration(namedDecl, hasExtraneousDisplayName: displayName, fromArgument: displayNameArgument, using: attribute))
} else {
- displayName = StringLiteralExprSyntax(content: rawIdentifier)
+ displayName = StringLiteralExprSyntax(content: nameWithoutBackticks)
}
}
diff --git a/Tests/TestingTests/Support/GraphTests.swift b/Tests/TestingTests/Support/GraphTests.swift
index 5f956dd6..9de7e28f 100644
--- a/Tests/TestingTests/Support/GraphTests.swift
+++ b/Tests/TestingTests/Support/GraphTests.swift
@@ -53,8 +53,7 @@ struct GraphTests {
#expect(graph.subgraph(at: "C2", "C3")?.value == 2468)
}
- @Test("subscript([K]) operator")
- func `subscript`() {
+ @Test func `subscript([K]) operator`() {
let graph = Graph<String, Int>(value: 123, children: [
"C1": Graph(value: 456),
"C2": Graph(value: 789, children: [
…ames. This PR modifies the behaviour of this compile-time macro diagnostic: > 🛑 "Attribute 'Test' specifies display name 'foo' for function with implicit > display name 'bar' If `bar` (in the above context) is _not_ considered a raw identifier by the language, we emit a warning instead of an error. This will allow us to adjust display-name-from-backticked-name inference (see #1174) without introducing a source-breaking change for a declaration such as: ```swift @test("subscript([K]) operator") func `subscript`() ``` (The above is a real-world test function in our own package that would be impacted.) Note that we don't actually have a code path that triggers this warning yet. #1174, if approved and merged, would introduce such a code path.
…ames. (#1175) This PR modifies the behaviour of this compile-time macro diagnostic: > 🛑 "Attribute 'Test' specifies display name 'foo' for function with implicit display name 'bar' If `bar` (in the above context) is _not_ considered a raw identifier by the language, we emit a warning instead of an error. This will allow us to adjust display-name-from-backticked-name inference (see #1174) without introducing a source-breaking change for a declaration such as: ```swift @test("subscript([K]) operator") func `subscript`() ``` (The above is a real-world test function in our own package that would be impacted.) Note that we don't actually have a code path that triggers this warning yet. #1174, if approved and merged, would introduce such a code path. Here's an example of what that would look like: <img width="774" alt="Screenshot showing the warning diagnostic presented for func subscript()" src="https://github.com/user-attachments/assets/ee8ff23c-8cbb-4335-af36-24a54deac6cc" /> ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
Sources/TestingMacros/Support/Additions/TokenSyntaxAdditions.swift
Outdated
Show resolved
Hide resolved
… name consisting of a single token
5b81d1c
to
2bc3ee5
Compare
@swift-ci please test |
@Test("Raw identifier is detected") | ||
func rawIdentifier() { | ||
#expect(TokenSyntax.identifier("`hello`").rawIdentifier == nil) |
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.
Can you re-add these cases please? They're still valid.
@@ -28,6 +28,10 @@ private import Foundation | |||
|
|||
@Sendable func freeSyncFunctionParameterized2(_ i: Int, _ j: String) {} | |||
|
|||
#if compiler(>=6.2) && hasFeature(RawIdentifiers) | |||
@Test(.hidden, arguments: [0]) func `ValidSingleCapitalizedToken`(someLengthyParameterName: Int) {} |
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.
Should we add a test that uses an escaped keyword name too?
This tweaks the logic for inferring a display name for a test or suite which has a raw identifier name so that it will take effect even if the content of the raw identifier is a single, valid token.
Motivation:
Typically, raw identifiers (introduced in SE-0451) are intended for use when you want to include characters in the name of a test or suite which aren't valid in identifiers — such as whitespace or (some) punctuation. However, it can sometimes be useful to specify a raw identifier for a test or suite even when it isn't strictly needed, because of how the testing library implicitly uses the content of a raw identifier as the display name.
For example, consider a parameterized test function which currently has an explicit display name consisting of a single word, and has a lengthy parameter label:
This test will have the plain display name
Authentication
and that's what will be displayed in console output and in the UI of integrated tools/IDEs. Even though the display name is only a single word, it allows the test author to precisely control how the test is presented in results and omit the lengthy parameter label, which allows the output to be cleaner and easier to understand.Now imagine the user wishes to modernize their tests by adopting the new raw identifiers feature. They do so by switching to the following:
In this scenario, because of an edge case in the current handling logic of the testing library, no display name will be inferred at all, and the console output and UI tools will show the full underlying name which is
Authentication(authenticationSessionCoordinator:)
. However this isn't ideal, since the user no longer has an idiomatic way to specify the succinct display name they want using a raw identifier because it happens to consist of a single token without any whitespace or special characters. Although this can be worked around by specifying two or more words inside the raw identifier, there isn't an obvious reason why doing so is necessary and users should be permitted to specify a single valid token as a display name if they want to.Modifications:
With the fix in this PR, the above test would have the expected display name of
Authentication
inferred. Specifically, this impacts thedisplayName
, not thename
, property ofTest
.Checklist: