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

extension of test class should NOT trigger no_magic_numbers rule #5137

Closed
JulyKikuAkita opened this issue Jul 20, 2023 · 5 comments · Fixed by #5146
Closed

extension of test class should NOT trigger no_magic_numbers rule #5137

JulyKikuAkita opened this issue Jul 20, 2023 · 5 comments · Fixed by #5146
Labels
acceptable-false-positive False positives caused by rules that are unavoidable due to missing type information. bug Unexpected and reproducible misbehavior.

Comments

@JulyKikuAkita
Copy link

New Issue Checklist

Describe the bug

Complete output when running SwiftLint, including the stack trace and command used
$ swiftlint lint --path /Tests/LintTests/MyTests.swift  --no-cache --enable-all-rules
warning: The --path option is deprecated. Pass the path(s) to lint last to the swiftlint command.
Linting Swift files at paths Tests/LintTests/MyTests.swift
Linting 'MyTests.swift' (1/1)
warning: The `anyobject_protocol` rule is now deprecated and will be completely removed in a future release.
warning: The `inert_defer` rule is now deprecated and will be completely removed in a future release.
warning: The `unused_capture_list` rule is now deprecated and will be completely removed in a future release.
/Tests/LintTests/MyTests.swift:4:5: warning: Explicit ACL Violation: All declarations should specify Access Control Level keywords explicitly (explicit_acl)
/Tests/LintTests/MyTests.swift:5:13: warning: Explicit Type Interface Violation: Properties should have a type interface (explicit_type_interface)
/Tests/LintTests/MyTests.swift:12:13: warning: Explicit Type Interface Violation: Properties should have a type interface (explicit_type_interface)
/Tests/LintTests/MyTests.swift:11:5: warning: Missing Docs Violation: public declarations should be documented (missing_docs)
/Tests/LintTests/MyTests.swift:10:1: error: No Extension Access Modifier Violation: Prefer not to use extension access modifiers (no_extension_access_modifier)
/Tests/LintTests/MyTests.swift:10:8: warning: No Grouping Extension Violation: Extensions shouldn't be used to group code within the same source file (no_grouping_extension)

/Tests/LintTests/MyTests.swift:13:42: warning: No Magic Numbers Violation: Magic numbers should be replaced by named constants (no_magic_numbers)

/Tests/LintTests/MyTests.swift:6:9: warning: Prefer Nimble Violation: Prefer Nimble matchers over XCTAssert functions (prefer_nimble)
/Tests/LintTests/MyTests.swift:13:9: warning: Prefer Nimble Violation: Prefer Nimble matchers over XCTAssert functions (prefer_nimble)
/Tests/LintTests/MyTests.swift:3:14: warning: Required Deinit Violation: Classes should have an explicit deinit method (required_deinit)
Done linting! Found 10 violations, 1 serious in 1 file.

Environment

  • SwiftLint version: 0.52.4
  • both homebrew and swiftcommandplugins: both use the same swiftlint version and can reproduce the bug
  • Paste your configuration file:
opt_in_rules:
- no_magic_numbers

We use default settings so "XCTestCase" should by default ignored by no_magic_number rule

  • Are you using nested configurations?
    no
  • Which Xcode version are you using (check xcodebuild -version)?
    Xcode 14.3 Build version 14E222b
  • Do you have a sample that shows the issue? Run echo "[string here]" | swiftlint lint --no-cache --use-stdin --enable-all-rules
    to quickly test if your example is really demonstrating the issue. If your example is more
    complex, you can use swiftlint lint --path [file here] --no-cache --enable-all-rules.
import XCTest

final class MyTests: XCTestCase {
    func testMagicNumberRuleNotApplied() {
        let collection = []
        XCTAssertEqual(collection.count, 2) 
    }
}

extension MyTests {
    func testMagicNumberRuleApplied() {
        let collection = []
        XCTAssertEqual(collection.count, 2) // This triggers a no-magic-rule violation:
    }
}
@johnkaplantech
Copy link

We're seeing this too on a scaled up commercial system where putting a swiftlint:disable in each test extension is impractical. Please expedite a fix!

@SimplyDanny
Copy link
Collaborator

As normal (and fast) SwiftLint rules operate on syntax-level only, it's impossible to know to which class, struct, ... declaration an extension belongs to. It only sees an extension called MyTest and just doesn't know it implements XCTestCase.

With some effort, this can perhaps be improved by tracking declarations in a single file but this will definitely not be supported across file boundaries.

@SimplyDanny SimplyDanny added bug Unexpected and reproducible misbehavior. acceptable-false-positive False positives caused by rules that are unavoidable due to missing type information. labels Jul 21, 2023
@mildm8nnered
Copy link
Collaborator

@JulyKikuAkita @johnkaplantech - are your extensions in the same file, or different files?

I can probably get this to work for single files, but if that's not going to help you at all ...

@JulyKikuAkita
Copy link
Author

-Thank you for addressing that, work for single files will definitely help most of the scenarios. Our extensions for tests are usually at the same file, but for readability, there's some scenarios that we moved the extension to a different file.

  • Rethink of adopting the no_magic_number lint rules at a large code base, wondering if it's more practical to request a feature to allow custom configuration for exclude directories/files. (some projects are using nested configurations to exclude Tests folders for no_magic_numbers)

@mildm8nnered
Copy link
Collaborator

-Thank you for addressing that, work for single files will definitely help most of the scenarios. Our extensions for tests are usually at the same file, but for readability, there's some scenarios that we moved the extension to a different file.

  • Rethink of adopting the no_magic_number lint rules at a large code base, wondering if it's more practical to request a feature to allow custom configuration for exclude directories/files. (some projects are using nested configurations to exclude Tests folders for no_magic_numbers)

Nested configurations are probably the easiest way to resolve this for extensions that are not in the same source file as the class declaration.

Alternatively, you could just run SwiftLint twice - once with one config for your source files, and with another config for your test files.

I think this no_magic_numbers is the most obvious case to which "normal" and "test" code would want different treatment though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acceptable-false-positive False positives caused by rules that are unavoidable due to missing type information. bug Unexpected and reproducible misbehavior.
Projects
None yet
4 participants