From 5c4820f43ca60c78098f2a2d8889a8ede444bfd2 Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Mon, 10 Oct 2022 19:55:04 -0700 Subject: [PATCH] Improve `XCTest` import detection logic. Specifically, this will now find imports of specific decls (e.g., `import class XCTest.XCTestCase`) and imports inside `#if` blocks. Also add some tests specifically for this logic, instead of testing it only in the context of other rules. Fixes #403. --- .../ImportsXCTestVisitor.swift | 38 ++++++++----- .../AlwaysUseLowerCamelCaseTests.swift | 48 ++++++++++++++++ .../ImportsXCTestVisitorTests.swift | 57 +++++++++++++++++++ 3 files changed, 128 insertions(+), 15 deletions(-) create mode 100644 Tests/SwiftFormatRulesTests/ImportsXCTestVisitorTests.swift diff --git a/Sources/SwiftFormatRules/ImportsXCTestVisitor.swift b/Sources/SwiftFormatRules/ImportsXCTestVisitor.swift index 81d64f177..29b608180 100644 --- a/Sources/SwiftFormatRules/ImportsXCTestVisitor.swift +++ b/Sources/SwiftFormatRules/ImportsXCTestVisitor.swift @@ -13,8 +13,8 @@ import SwiftFormatCore import SwiftSyntax -/// Visitor that determines if the target source file imports XCTest -fileprivate class ImportsXCTestVisitor: SyntaxVisitor { +/// A visitor that determines if the target source file imports `XCTest`. +private class ImportsXCTestVisitor: SyntaxVisitor { private let context: Context init(context: Context) { @@ -22,30 +22,38 @@ fileprivate class ImportsXCTestVisitor: SyntaxVisitor { super.init(viewMode: .sourceAccurate) } - override func visit(_ node: SourceFileSyntax) -> SyntaxVisitorContinueKind { - for statement in node.statements { - guard let importDecl = statement.item.as(ImportDeclSyntax.self) else { continue } - for component in importDecl.path { - guard component.name.text == "XCTest" else { continue } - context.importsXCTest = .importsXCTest - return .skipChildren - } + override func visit(_ node: ImportDeclSyntax) -> SyntaxVisitorContinueKind { + // If we already know whether or not `XCTest` is imported, don't bother doing anything else. + guard context.importsXCTest == .notDetermined else { return .skipChildren } + + // If the first import path component is the `XCTest` module, record that fact. Checking in this + // way lets us catch `import XCTest` but also specific decl imports like + // `import class XCTest.XCTestCase`, if someone wants to do that. + if node.path.first!.name.tokenKind == .identifier("XCTest") { + context.importsXCTest = .importsXCTest } - context.importsXCTest = .doesNotImportXCTest + return .skipChildren } + + override func visitPost(_ node: SourceFileSyntax) { + // If we visited the entire source file and didn't find an `XCTest` import, record that fact. + if context.importsXCTest == .notDetermined { + context.importsXCTest = .doesNotImportXCTest + } + } } -/// Sets the appropriate value of the importsXCTest field in the Context class, which -/// indicates whether the file contains test code or not. +/// Sets the appropriate value of the `importsXCTest` field in the context, which approximates +/// whether the file contains test code or not. /// /// This setter will only run the visitor if another rule hasn't already called this function to -/// determine if the source file imports XCTest. +/// determine if the source file imports `XCTest`. /// /// - Parameters: /// - context: The context information of the target source file. /// - sourceFile: The file to be visited. -func setImportsXCTest(context: Context, sourceFile: SourceFileSyntax) { +public func setImportsXCTest(context: Context, sourceFile: SourceFileSyntax) { guard context.importsXCTest == .notDetermined else { return } let visitor = ImportsXCTestVisitor(context: context) visitor.walk(sourceFile) diff --git a/Tests/SwiftFormatRulesTests/AlwaysUseLowerCamelCaseTests.swift b/Tests/SwiftFormatRulesTests/AlwaysUseLowerCamelCaseTests.swift index 07bd0de48..0421e172e 100644 --- a/Tests/SwiftFormatRulesTests/AlwaysUseLowerCamelCaseTests.swift +++ b/Tests/SwiftFormatRulesTests/AlwaysUseLowerCamelCaseTests.swift @@ -124,6 +124,54 @@ final class AlwaysUseLowerCamelCaseTests: LintOrFormatRuleTestCase { .nameMustBeLowerCamelCase("test_HappyPath_Through_GoodCode_Throws", description: "function")) } + func testIgnoresUnderscoresInTestNamesWhenImportedConditionally() { + let input = + """ + #if SOME_FEATURE_FLAG + import XCTest + + let Test = 1 + class UnitTests: XCTestCase { + static let My_Constant_Value = 0 + func test_HappyPath_Through_GoodCode() {} + private func FooFunc() {} + private func helperFunc_For_HappyPath_Setup() {} + private func testLikeMethod_With_Underscores(_ arg1: ParamType) {} + private func testLikeMethod_With_Underscores2() -> ReturnType {} + func test_HappyPath_Through_GoodCode_ReturnsVoid() -> Void {} + func test_HappyPath_Through_GoodCode_ReturnsShortVoid() -> () {} + func test_HappyPath_Through_GoodCode_Throws() throws {} + } + #endif + """ + performLint(AlwaysUseLowerCamelCase.self, input: input) + XCTAssertDiagnosed( + .nameMustBeLowerCamelCase("Test", description: "constant"), line: 4, column: 7) + XCTAssertDiagnosed( + .nameMustBeLowerCamelCase("My_Constant_Value", description: "constant"), line: 6, column: 16) + XCTAssertNotDiagnosed( + .nameMustBeLowerCamelCase("test_HappyPath_Through_GoodCode", description: "function")) + XCTAssertDiagnosed( + .nameMustBeLowerCamelCase("FooFunc", description: "function"), line: 8, column: 18) + XCTAssertDiagnosed( + .nameMustBeLowerCamelCase("helperFunc_For_HappyPath_Setup", description: "function"), + line: 9, column: 18) + XCTAssertDiagnosed( + .nameMustBeLowerCamelCase("testLikeMethod_With_Underscores", description: "function"), + line: 10, column: 18) + XCTAssertDiagnosed( + .nameMustBeLowerCamelCase("testLikeMethod_With_Underscores2", description: "function"), + line: 11, column: 18) + XCTAssertNotDiagnosed( + .nameMustBeLowerCamelCase( + "test_HappyPath_Through_GoodCode_ReturnsVoid", description: "function")) + XCTAssertNotDiagnosed( + .nameMustBeLowerCamelCase( + "test_HappyPath_Through_GoodCode_ReturnsShortVoid", description: "function")) + XCTAssertNotDiagnosed( + .nameMustBeLowerCamelCase("test_HappyPath_Through_GoodCode_Throws", description: "function")) + } + func testIgnoresFunctionOverrides() { let input = """ diff --git a/Tests/SwiftFormatRulesTests/ImportsXCTestVisitorTests.swift b/Tests/SwiftFormatRulesTests/ImportsXCTestVisitorTests.swift new file mode 100644 index 000000000..964cc86c7 --- /dev/null +++ b/Tests/SwiftFormatRulesTests/ImportsXCTestVisitorTests.swift @@ -0,0 +1,57 @@ +import SwiftFormatCore +import SwiftFormatRules +import SwiftFormatTestSupport +import SwiftParser +import XCTest + +class ImportsXCTestVisitorTests: DiagnosingTestCase { + func testDoesNotImportXCTest() throws { + XCTAssertEqual( + try makeContextAndSetImportsXCTest(source: """ + import Foundation + """), + .doesNotImportXCTest + ) + } + + func testImportsXCTest() throws { + XCTAssertEqual( + try makeContextAndSetImportsXCTest(source: """ + import Foundation + import XCTest + """), + .importsXCTest + ) + } + + func testImportsSpecificXCTestDecl() throws { + XCTAssertEqual( + try makeContextAndSetImportsXCTest(source: """ + import Foundation + import class XCTest.XCTestCase + """), + .importsXCTest + ) + } + + func testImportsXCTestInsideConditional() throws { + XCTAssertEqual( + try makeContextAndSetImportsXCTest(source: """ + import Foundation + #if SOME_FEATURE_FLAG + import XCTest + #endif + """), + .importsXCTest + ) + } + + /// Parses the given source, makes a new `Context`, then populates and returns its `XCTest` + /// import state. + private func makeContextAndSetImportsXCTest(source: String) throws -> Context.XCTestImportState { + let sourceFile = try Parser.parse(source: source) + let context = makeContext(sourceFileSyntax: sourceFile) + setImportsXCTest(context: context, sourceFile: sourceFile) + return context.importsXCTest + } +}