Private Unit Test Rule: Rule is being evaluated on every function, not just tests

[cfilipov]

Related to #761:

This is embarrassing... This new rule has a bug where it tries to validate instance methods even if they aren't part of an XCTestCase subclass. Unlike #782 I think this is unquestionably not what people would expect this rule to do so it should be fixed ASAP. I have a working fix but won't be able to create a pull request until I get home tonight (PST). Here's a brain dump for when I get back to this:

I discovered this when trying to enable this rule for my custom subclass:

private_unit_tst: #typo
  severity: error
  regex: "(XCTestCase|MyUnitTestCase)"

The regex in that rule is supposed to limit the scope of this rule to classes that inherit from XCTestCase or MyUnitTestCase. However, I had a typo in the config, so this rule should never have caught any style violations in MyUnitTestCase, but it did!

In fact, in its current form, this rule will detect any instance method that begins with the word "test" and is marked private as a violation. It just so happens that this is uncommon enough that I never ran into it because methods beginning with "test" outside of unit tests are pretty rare.

Knowing this, the bug is pretty easy to spot in the source code for this rule: The validateClass() method checks to see if this class is a test class before checking the visibility modifier, and bails out early if it's not a test class. But validateClass() has no way of signaling its caller (validateFile()) that it bailed out early because this is not an applicable subclass, so validateFile() just continues on looking at the substructure.

The fix is simple, move the logic for matching the inherited types with the regex to a isTestClass() helper function and call that explictely in validateFile(). At this point the validateClass() implementation becomes trivial so it can be moved into validateFile(), just after the isTestClass() check.

Side Note

I noticed that severity: error in the YAML example above had no effect unless I also included the regex key. I suspect I'm failing when reading that field in the config file and that's probably happening before the severity key is read.

Edit: Yep. Missing regex causes applyConfiguration() to bail out at the beginning.

This happens before any other parts of the config are applied.

guard let configurationDict = configuration as? [String: AnyObject],
    regexString = configurationDict["regex"] as? String else {
        throw ConfigurationError.UnknownConfiguration
}

Easy to fix, will include that too.

[jpsim]

Nice find and nice work @cfilipov! It's an honour to have such strong contributors to this project 😊