Add rule to check for private unit tests

[cfilipov]

If an XCTest method is marked private it will be silently skipped, no warning.

Example:

class MyTest: XCTestCase {
    private func testFoo() {
        // this test will not run
    }
    func testBar() {
        // this test will run
    }
}

Likewise, if the XCTest subclass is marked private all test methods on that class will be skipped.

private class MyOtherTest: XCTestCase {
    // none of the tests from `MyOtherTest` will run
}

It is bad practice to intentionally commit a private test for the same reason that it is a bad practice to commit commented-out code. This type of mistake is very easy to miss, so a SwiftLint rule has been created to catch instances of private tests.

[codecov-io]

Current coverage is 88.63% (diff: 67.79%)

Merging #761 into master will decrease coverage by 0.88%

@@             master       #761   diff @@
==========================================
  Files            77         79     +2   
  Lines          2633       2745   +112   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2357       2433    +76   
- Misses          276        312    +36   
  Partials          0          0          

Powered by Codecov. Last update 8389aa2...caa6d5e

[masters3d]

Does this need to be public?

[cfilipov]

Does this need to be public?

I don't know, it was already public, I didn't change this line, just changed the raw values.

[masters3d]

You need to update the changelog.
Does Linux XCTest act the same way?
What about filePrivate? Is the story the same?
https://github.com/apple/swift-evolution/blob/master/proposals/0025-scoped-access-level.md

[cfilipov]

Does Linux XCTest act the same way?

Surprisingly, yes. For Linux, XCTest needs to explictely return all tests, so you would think it would at least result in an error. But if you mark the test as private, it just doesn't get run, also no warning or error.

What about filePrivate? Is the story the same?

Yes. However, that's a Swift 3 feature, so not part of this PR.

[jpsim]

Actually the distinction between rawValue and sourcekitValue() was intentional here. We sometimes use AccessControlLevel.description in violation messages (I think).

[cfilipov]

I didn't realize that, but indeed it looks like this is the case in the init method of MissingDocsRule where the configuration is mapped over. I'll change the rawValue back and add a new init fromSourcekitValue. It's worth noting however: AccessControlLevel seems to be inconsistent with how it's done in other parts of the code. KindType and SwiftDeclarationKind both use the sourcekit value strings as their rawValue.

[jpsim]

It may be better to keep your changes but add explicit conformance to CustomStringConvertible and declare var description: String with the short/lowercase member names.

[cfilipov]

I could't actually find specific instances where description was used on AccessControlLevel, but that's easy to miss since there are several way description can be used implicitly that's hard to search for. I went ahead and made it conform to CustomStringConvertible as suggested which returns the short strings. I also had to modify MissingDocsRule.init(configuration:) because that relied on the rawValue being the shorter version of the string:

let acl = array.flatMap(AccessControlLevel.init(description:))

Since AccessControlLevel needs to be able to be instantiated from the short strings I also head to create an init method for that. Couldn't think of a better name so hopefully init(description:) will suffice.

[jpsim]

Thanks for the PR @cfilipov! Looks great. I just had two minor comments, and this will need to be rebased.

[jpsim]

This is fantastic @cfilipov. I really appreciate your work on this.

[cfilipov]

Wonderful! Thanks for merging this in! I should probably create a ticket to track the inclusion of the check for fileprivate when Swiftlint is ready for Swift 3.