Skip to content
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 test_parent_classes option to single_test_class, balanced_xctest_lifecycle, and empty_xctest rules #4233

Conversation

mildm8nnered
Copy link
Collaborator

Adds a test_parent_classes option to the single_test_class, balanced_xctest_lifecycle, and empty_xctest rules, resolving the remaining rules mentioned in #4200.

Very similar to #4214 but see notes below.

For example, given

class MyTestCase: XCTestCase { }

class SwiftLintDemoTests: XCTestCase {
    // Unbalanced
    override func setUpWithError() throws {
        XCTFail("Hello")
    }

    // Empty
    func testExample() throws {

    }
}

// Second test class
class SwiftLintDemoTests2: XCTestCase {
    func testExample() throws {
        XCTFail("Hello")
    }
}

The following configurations will pick up the violations:

opt_in_rules:
  - balanced_xctest_lifecycle
  - single_test_class
  - empty_xctest_method

balanced_xctest_lifecycle:
  test_parent_classes: ["MyTestCase"]

single_test_class:
  test_parent_classes: ["MyTestCase"]

empty_xctest_method:
  test_parent_classes: ["MyTestCase"]

Notes

There is a lot of redundancy in the configurations. BalancedXCTestLifecycleConfiguration.swift and EmptyXCTestMethodConfiguration.swift are identical, and SingleTestClassConfiguration.swift differs only in including QuickSpec as an additional default parent class.

I didn't roll anything up as yet, but obviously it would be trivial to either have an abstract parent class, or to have two of the configurations inherit from the other one, and to take the default parent classes as a constructor argument.

Also, I had to implement makeViolation(file: SwiftLintFile, position: AbsolutePosition) -> StyleViolation myself in EmptyXCTestMethodRule.swift because my rule's configuration was no longer a SeverityConfiguration - I wonder if there some way I could have avoided having to do that.

@SwiftLintBot
Copy link

1 Warning
⚠️ This PR may need tests.
12 Messages
📖 Linting Aerial with this PR took 1.2s vs 1.18s on main (1% slower)
📖 Linting Alamofire with this PR took 1.48s vs 1.47s on main (0% slower)
📖 Linting Firefox with this PR took 6.67s vs 6.66s on main (0% slower)
📖 Linting Kickstarter with this PR took 9.5s vs 9.52s on main (0% faster)
📖 Linting Moya with this PR took 0.67s vs 0.67s on main (0% slower)
📖 Linting Nimble with this PR took 0.65s vs 0.64s on main (1% slower)
📖 Linting Quick with this PR took 0.3s vs 0.29s on main (3% slower)
📖 Linting Realm with this PR took 11.27s vs 11.1s on main (1% slower)
📖 Linting SourceKitten with this PR took 0.53s vs 0.52s on main (1% slower)
📖 Linting Sourcery with this PR took 2.32s vs 2.34s on main (0% faster)
📖 Linting Swift with this PR took 4.41s vs 4.41s on main (0% slower)
📖 Linting WordPress with this PR took 10.96s vs 10.96s on main (0% slower)

Generated by 🚫 Danger

Copy link
Collaborator

@SimplyDanny SimplyDanny left a comment

Choose a reason for hiding this comment

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

There is a lot of redundancy in the configurations. BalancedXCTestLifecycleConfiguration and EmptyXCTestMethodConfiguration are identical, and SingleTestClassConfiguration differs only in including QuickSpec as an additional default parent class.

QuickSpec also inherits from XCTestCase and thus should be in the default set of all of the configurations. This is an improvement for a follow-up change, though.

I didn't roll anything up as yet, but obviously it would be trivial to either have an abstract parent class, or to have two of the configurations inherit from the other one, and to take the default parent classes as a constructor argument.

We could have one configuration struct used by all the rules when all have QuickSpec in their default set. For now, it's fine to have multiple configurations.

Also, I had to implement makeViolation(file: SwiftLintFile, position: AbsolutePosition) -> StyleViolation myself in EmptyXCTestMethodRule because my rule's configuration was no longer a SeverityConfiguration - I wonder if there some way I could have avoided having to do that.

Currently, there is no way to avoid it. Up to now, only a handful of rules require manual implementation.


public var consoleDescription: String {
return severityConfiguration.consoleDescription +
", test_parent_classes: [\(testParentClasses)]"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The brackets are superfluous. Sorting the set makes the output deterministic:

Suggested change
", test_parent_classes: [\(testParentClasses)]"
", test_parent_classes: \(testParentClasses.sorted())"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I added QuickSpec to all the defaults, and rolled the new ones up, with typealiases for the names you'd expect.

But I bodged rebasing, so I just moved to a new branch and PR - #4262

@mildm8nnered
Copy link
Collaborator Author

Replaced by #4262

@mildm8nnered mildm8nnered deleted the mildm8nnered-add-test-parent-class-option-to-more-test-rules branch October 16, 2022 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants