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

no_magic_numbers should not trigger in test code #4896

Closed
SimplyDanny opened this issue Apr 15, 2023 · 4 comments · Fixed by #4897
Closed

no_magic_numbers should not trigger in test code #4896

SimplyDanny opened this issue Apr 15, 2023 · 4 comments · Fixed by #4897
Assignees
Labels
enhancement Ideas for improvements of existing features and rules. good first issue Issue to be taken up by new contributors yet unfamiliar with the project.

Comments

@SimplyDanny
Copy link
Collaborator

Description

In test code, it's reasonable to use raw numbers in assertions. The no_magic_numbers rule thus reports a lot of findings in tests.

class MyTest: XCTestCase {
  func test() {
    let x = 4
    XCTAssertEqual(x, 7)  // Violation here
  }
}

Defining another variable let expected = 7 would not improve style, but would rather make the code more opaque and difficult to reason about.

It's probably required to add a configuration option to specify which parent classes make a test class a test class. In the example, it's XCTestCase (the default of the option). This can be taken over from already existing rules specific for tests.

Environment

  • 0.51.0
  • Homebrew
  • Paste your configuration file:
only_rules:
  - no_magic_numbers
@SimplyDanny SimplyDanny added enhancement Ideas for improvements of existing features and rules. good first issue Issue to be taken up by new contributors yet unfamiliar with the project. labels Apr 15, 2023
@mildm8nnered
Copy link
Collaborator

What configuration options were you thinking of @SimplyDanny?

Presumably a test_parent_classes: [“QuickSpec”, “XCTestCase”] like the test related rules have. Would that be empty by default, or would there be a separate flag to say "ignore cases in test classes", and then test_parent_classes to determine which classes are tests?

@SimplyDanny
Copy link
Collaborator Author

Yes, the option test_parent_classes is what I was referring to. The default can be the same like in the other rules supporting this option. I think it's reasonable for this rule to ignore tests by default. If anyone likes to see violations in tests, the option can be cleared.

We would not need it if we have another reliable marker characterizing test classes. I have no idea though. Let me know if you see another way to recognize them.

@mildm8nnered
Copy link
Collaborator

I can't think of another way - I can have a go at adding that in - should be pretty easy, unless you've already started, in which case I can hang back ...

@SimplyDanny
Copy link
Collaborator Author

I've not started, so please go ahead.

@SimplyDanny SimplyDanny changed the title no_magic_numbers should not trigger test code no_magic_numbers should not trigger in test code Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Ideas for improvements of existing features and rules. good first issue Issue to be taken up by new contributors yet unfamiliar with the project.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants