-
Notifications
You must be signed in to change notification settings - Fork 221
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
Add checks for test functions marked with @Test attribute in relevant Rules #767
Add checks for test functions marked with @Test attribute in relevant Rules #767
Conversation
In the test code validation added checks to consider new Swift Testing syntax as well.
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.
P.S. I am planning to add check for new testing framework for NeverForceUnwrap, NeverUseForceTry and NeverUseImplicitlyUnwrappedOptionals rules as well.
Would you be willing to do that in this PR? I don't think it would make it too big to review and it would be nice to have a single PR that adds both new swift-testing-related features.
@allevato I have added the Swift Testing checks for |
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.
Looks really good! Just one comment about the attribute checking.
let attributeName = attribute.as(AttributeSyntax.self)?.attributeName | ||
return attributeName?.as(IdentifierTypeSyntax.self)?.name.text == name | ||
// support @Module.Attribute syntax as well | ||
|| attributeName?.as(MemberTypeSyntax.self)?.name.text == name |
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.
Since you're not checking the module name here, this would match anything, right? Like @Foo.Test
, for example.
It might be better to make the function signature hasAttribute(_ name: String, inModule: String)
and have the caller always provide it, but it would only be checked if it was a MemberTypeSyntax
. So hasAttribute("Test", inModule: "Testing")
would still match either @Test
or @Testing.Test
, but not @Foo.Test
.
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 you think if we will have 2 functions, one will be the current one and another function like you suggested?
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.
I think it's better for simplicity to have one function that does both checks. We can't definitively know if @Test
refers to @Testing.Test
without the file being type-checked, which we can't do in swift-format. So we're making an assumption that @Test
by itself refers to the one in Testing
and nothing else, because of how common we expect that pattern to be. If we're handling that as the common case, then there's no reason to not also handle @Testing.Test
with the same query—each of the individual rules shouldn't have to make two calls to find the attribute it's looking for.
// support @Module.Attribute syntax as well | ||
|| attributeName?.as(MemberTypeSyntax.self)?.name.text == name | ||
} | ||
/// Indicates whether the node has attribute with the given `name`. |
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 mention in the doc comment that the module name is only considered if present if the attribute is written as @Module.Name
, and that that's intentional? Otherwise it might look like a bug to someone reading it in the future.
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.
good point, done. 👍
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.
Thank you for adding this!
This pull request updates the
AlwaysUseLowerCamelCase
rule in the swift-format project to support the new Swift Testing framework.Previously, the rule allowed underscores in test function names based on the old
XCTest
framework criteria. I’ve added a check to also allow underscores for functions marked with the@Test
attribute.P.S. I am planning to add check for new testing framework for
NeverForceUnwrap
,NeverUseForceTry
andNeverUseImplicitlyUnwrappedOptionals
rules as well.