-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 TestCaseAccessibilityRule #3376
Conversation
This rule verifies that all members of XCTestCase subclasses are private (or fileprivate) except for those defined by XCTestCase. This is useful for a few things: 1. You can make sure code that isn't needed outside the test case isn't overly accessible 2. You can catch typos like `func tsetSomething` that won't actually be run This rule has configuration for `method_prefixes` so that custom methods can still be allowed. This is useful if you disable tests by renaming them (so they continue to be compiled) like `func disabled_testFoo` This change also extracts some useful shared logic around identifying what members are provided by XCTest since they are now used in multiple places.
Codecov Report
@@ Coverage Diff @@
## master #3376 +/- ##
==========================================
+ Coverage 90.48% 90.51% +0.02%
==========================================
Files 417 420 +3
Lines 20508 20552 +44
==========================================
+ Hits 18557 18602 +45
+ Misses 1951 1950 -1
Continue to review full report at Codecov.
|
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 good, and useful! I just have a few questions/suggestions.
Source/SwiftLintFramework/Rules/Lint/TestCaseAccessibilityRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintFramework/Rules/Lint/TestCaseAccessibilityRule.swift
Outdated
Show resolved
Hide resolved
….swift Co-authored-by: JP Simard <jp@jpsim.com>
….swift Co-authored-by: JP Simard <jp@jpsim.com>
Updated to more accurately identify XCTestCase members |
|
||
public var consoleDescription: String { | ||
return severityConfiguration.consoleDescription + | ||
", method_prefixes: [\(methodPrefixes)]" | ||
", allowed_prefixes: [\(allowedPrefixes)]" |
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.
Examples can now specify configurations, so it'd be nice to test something like disabled_test
as an allowed prefix.
SwiftLint/Source/SwiftLintFramework/Models/Example.swift
Lines 6 to 16 in da408b5
/// The untyped configuration to apply to the rule, if deviating from the default configuration. | |
/// The structure should match what is expected as a configuration value for the rule being tested. | |
/// | |
/// For example, if the following YAML would be used to configure the rule: | |
/// | |
/// ``` | |
/// severity: warning | |
/// ``` | |
/// | |
/// Then the equivalent configuration value would be `["severity": "warning"]`. | |
public private(set) var configuration: Any? |
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.
Is there an example of that working? I tried that and I don't believe it's passed all the way through the tests
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.
AFAICT it only is for analyze tests
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.
Oh I think you're right. Let's hold off for now, but it should be easy to make normal lint tests apply this configuration value.
return false | ||
static func isXCTestMember(kind: SwiftDeclarationKind, name: String, | ||
attributes: [SwiftDeclarationAttributeKind]) -> Bool { | ||
return attributes.contains(.override) |
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.
Nice.
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.
Really useful rule, thanks!
Nice rule! So maybe a possible improvement to consider: would it be possible to ie. only execute this rule's check within the main class declaration, and skip any extension declared on the test class instead? This would give some room in how to organise the class definition while still having this rule enabled. |
This rule verifies that all members of XCTestCase subclasses are private
(or fileprivate) except for those defined by XCTestCase. This is useful
for a few things:
overly accessible
func tsetSomething
that won't actually berun
This rule has configuration for
method_prefixes
so that custom methodscan still be allowed. This is useful if you disable tests by renaming
them (so they continue to be compiled) like
func disabled_testFoo
This change also extracts some useful shared logic around identifying
what members are provided by XCTest since they are now used in multiple
places.