From 357efe03489f6209114b53d078daebaeae8b7dac Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Mar 2026 20:31:03 -0700 Subject: [PATCH] Add `--test-case-access-control` option for `testSuiteAccessControl` rule (#2457) Co-authored-by: calda <1811727+calda@users.noreply.github.com> --- Rules.md | 6 +- Sources/Declaration.swift | 4 +- Sources/OptionDescriptor.swift | 7 + Sources/Options.swift | 3 + Sources/Rules/TestSuiteAccessControl.swift | 122 ++++++++----- Tests/Rules/TestSuiteAccessControlTests.swift | 166 ++++++++++++++++++ 6 files changed, 258 insertions(+), 50 deletions(-) diff --git a/Rules.md b/Rules.md index 1d2627ab..6fcc5264 100644 --- a/Rules.md +++ b/Rules.md @@ -3754,7 +3754,11 @@ Option | Description ## testSuiteAccessControl -Test methods should be internal, and other properties / functions in a test suite should be private. +Test methods should have the configured access control (default internal), and other properties / functions in a test suite should be private. + +Option | Description +--- | --- +`--test-case-access-control` | Access control for test methods: "open", "public", "package", "internal" (default), "fileprivate" or "private"
Examples diff --git a/Sources/Declaration.swift b/Sources/Declaration.swift index 7b811f42..fc35a908 100644 --- a/Sources/Declaration.swift +++ b/Sources/Declaration.swift @@ -450,7 +450,7 @@ extension Declaration { // MARK: - Visibility /// The visibility of a declaration -enum Visibility: String, CaseIterable, Comparable { +public enum Visibility: String, CaseIterable, Comparable { case open case `public` case package @@ -458,7 +458,7 @@ enum Visibility: String, CaseIterable, Comparable { case `fileprivate` case `private` - static func < (lhs: Visibility, rhs: Visibility) -> Bool { + public static func < (lhs: Visibility, rhs: Visibility) -> Bool { allCases.firstIndex(of: lhs)! > allCases.firstIndex(of: rhs)! } } diff --git a/Sources/OptionDescriptor.swift b/Sources/OptionDescriptor.swift index cc77d4a4..7eb51891 100644 --- a/Sources/OptionDescriptor.swift +++ b/Sources/OptionDescriptor.swift @@ -1475,6 +1475,13 @@ struct _Descriptors { keyPath: \.suiteNameFormat ) + let testCaseAccessControl = OptionDescriptor( + argumentName: "test-case-access-control", + displayName: "Test Case Access Control", + help: "Access control for test methods:", + keyPath: \.testCaseAccessControl + ) + // MARK: - Internal let fragment = OptionDescriptor( diff --git a/Sources/Options.swift b/Sources/Options.swift index 642b0b6c..dcb89544 100644 --- a/Sources/Options.swift +++ b/Sources/Options.swift @@ -909,6 +909,7 @@ public struct FormatOptions: CustomStringConvertible { public var preferSynthesizedInitForInternalStructs: PreferSynthesizedInitMode public var testCaseNameFormat: SwiftTestingNameFormat public var suiteNameFormat: SwiftTestingNameFormat + public var testCaseAccessControl: Visibility /// Deprecated public var indentComments: Bool @@ -1056,6 +1057,7 @@ public struct FormatOptions: CustomStringConvertible { preferSynthesizedInitForInternalStructs: PreferSynthesizedInitMode = .never, testCaseNameFormat: SwiftTestingNameFormat = .rawIdentifiers, suiteNameFormat: SwiftTestingNameFormat = .preserve, + testCaseAccessControl: Visibility = .internal, // Doesn't really belong here, but hard to put elsewhere fragment: Bool = false, ignoreConflictMarkers: Bool = false, @@ -1192,6 +1194,7 @@ public struct FormatOptions: CustomStringConvertible { self.preferSynthesizedInitForInternalStructs = preferSynthesizedInitForInternalStructs self.testCaseNameFormat = testCaseNameFormat self.suiteNameFormat = suiteNameFormat + self.testCaseAccessControl = testCaseAccessControl self.indentComments = indentComments self.fragment = fragment self.ignoreConflictMarkers = ignoreConflictMarkers diff --git a/Sources/Rules/TestSuiteAccessControl.swift b/Sources/Rules/TestSuiteAccessControl.swift index c55dc591..ba1e794a 100644 --- a/Sources/Rules/TestSuiteAccessControl.swift +++ b/Sources/Rules/TestSuiteAccessControl.swift @@ -10,31 +10,44 @@ import Foundation public extension FormatRule { static let testSuiteAccessControl = FormatRule( - help: "Test methods should be internal, and other properties / functions in a test suite should be private.", - disabledByDefault: true + help: "Test methods should have the configured access control (default internal), and other properties / functions in a test suite should be private.", + disabledByDefault: true, + options: ["test-case-access-control"] ) { formatter in guard let testFramework = formatter.detectTestingFramework() else { return } + // Determine the effective test visibility based on options and framework. + // XCTest requires test methods to be at least internal so the runtime can discover them. + let configuredVisibility = formatter.options.testCaseAccessControl + let effectiveTestVisibility: Visibility + if testFramework == .xcTest, + configuredVisibility == .private || configuredVisibility == .fileprivate + { + effectiveTestVisibility = .internal + } else { + effectiveTestVisibility = configuredVisibility + } + let declarations = formatter.parseDeclarations() let testClasses = declarations.compactMap(\.asTypeDeclaration).filter { typeDecl in formatter.isSimpleTestSuite(typeDecl, for: testFramework) } for testClass in testClasses { - // The test class itself should be internal unless marked as open - formatter.validateTestTypeAccessControl(testClass) + // The test class itself should have the configured visibility unless marked as open + testClass.ensureTestDeclarationAccessControl(visibility: effectiveTestVisibility) // Process each member of the test class for member in testClass.body { switch member.keyword { case "func": - formatter.validateTestFunctionAccessControl(member, for: testFramework) + formatter.validateTestFunctionAccessControl(member, for: testFramework, testCaseAccessControl: effectiveTestVisibility) case "init": - // Initializers should be internal unless marked as open - formatter.validateTestTypeAccessControl(member) + // Initializers should have the configured visibility unless marked as open + member.ensureTestDeclarationAccessControl(visibility: effectiveTestVisibility) case "let", "var": // Properties should be private unless they have special attributes @@ -83,19 +96,8 @@ public extension FormatRule { } extension Formatter { - /// Validates that a test type (class/struct) or its initializer has internal access control. - func validateTestTypeAccessControl(_ declaration: Declaration) { - // If marked as open, leave it as is - if declaration.modifiers.contains("open") { - return - } - - // Remove any non-internal, non-open ACL modifiers - removeACLModifiers(from: declaration, except: ["internal", "open"]) - } - /// Validates that a function in a test class has the correct access control. - func validateTestFunctionAccessControl(_ function: Declaration, for framework: TestingFramework) { + func validateTestFunctionAccessControl(_ function: Declaration, for framework: TestingFramework, testCaseAccessControl: Visibility) { guard let functionDecl = parseFunctionDeclaration(keywordIndex: function.keywordIndex) else { return } @@ -117,32 +119,20 @@ extension Formatter { if treatAsTestCase { // For XCTest: Skip if it's already private/fileprivate (respect explicit access control) - // For Swift Testing: Always make internal (private @Test functions are still executed) if framework == .xcTest, modifiers.contains("private") || modifiers.contains("fileprivate") { return } - // Test methods should be internal - validateTestMethodAccessControl(function) + // Test methods should have the configured test visibility + function.ensureTestDeclarationAccessControl(visibility: testCaseAccessControl) } else { // Non-test methods should be private (but skip if already private/fileprivate) if modifiers.contains("private") || modifiers.contains("fileprivate") { return } - ensurePrivateAccessControl(function) + function.ensurePrivateAccessControl() } } - /// Ensures a test method has internal access control (removes public/private modifiers). - func validateTestMethodAccessControl(_ declaration: Declaration) { - // If marked as open, leave it as is - if declaration.modifiers.contains("open") { - return - } - - // Remove any explicit ACL modifiers except internal and open - removeACLModifiers(from: declaration, except: ["internal", "open"]) - } - /// Validates that a property in a test class is private. func validateTestProperty(_ property: Declaration, for _: TestingFramework) { let modifiers = property.modifiers @@ -163,26 +153,64 @@ extension Formatter { } // Make it private - ensurePrivateAccessControl(property) + property.ensurePrivateAccessControl() + } +} + +extension Declaration { + /// Validates that a test type (class/struct) or its initializer has the required access control. + func ensureTestDeclarationAccessControl(visibility: Visibility) { + // If marked as open, leave it as is + if modifiers.contains("open") { + return + } + + ensureAccessControl(visibility: visibility) } - /// Removes ACL modifiers from a declaration, except for the specified exceptions. - func removeACLModifiers(from declaration: Declaration, except exceptions: [String]) { + /// Ensures this declaration has the specified access control level. + func ensureAccessControl(visibility: Visibility) { + // internal is the default (implicit) visibility in Swift + if visibility == .internal { + // Remove any explicit non-internal, non-open ACL modifiers + removeACLModifiers(except: ["internal", "open"]) + return + } + + // If already at the right visibility, do nothing + if modifiers.contains(visibility.rawValue) { + return + } + + // Look for an existing ACL modifier to replace + for aclModifier in _FormatRules.aclModifiers where aclModifier != "open" { + if let modifierIndex = formatter.indexOfModifier(aclModifier, forDeclarationAt: keywordIndex) { + formatter.replaceToken(at: modifierIndex, with: .keyword(visibility.rawValue)) + return + } + } + + // No ACL modifier exists, so add the visibility before the keyword + formatter.insert([.keyword(visibility.rawValue), .space(" ")], at: keywordIndex) + } + + /// Removes ACL modifiers from this declaration, except for the specified exceptions. + func removeACLModifiers(except exceptions: [String]) { for aclModifier in _FormatRules.aclModifiers where !exceptions.contains(aclModifier) { - if let modifierIndex = indexOfModifier(aclModifier, forDeclarationAt: declaration.keywordIndex) { + if let modifierIndex = formatter.indexOfModifier(aclModifier, forDeclarationAt: keywordIndex) { // Remove the modifier and its trailing space - if let nextIndex = index(of: .nonSpace, after: modifierIndex), nextIndex > modifierIndex + 1 { - removeTokens(in: modifierIndex ... (modifierIndex + 1)) + if let nextIndex = formatter.index(of: .nonSpace, after: modifierIndex), nextIndex > modifierIndex + 1 { + formatter.removeTokens(in: modifierIndex ... (modifierIndex + 1)) } else { - removeToken(at: modifierIndex) + formatter.removeToken(at: modifierIndex) } } } } - /// Ensures a declaration has private access control. - func ensurePrivateAccessControl(_ declaration: Declaration) { - let modifiers = declaration.modifiers + /// Ensures this declaration has private access control. + func ensurePrivateAccessControl() { + let modifiers = modifiers // If already private, do nothing if modifiers.contains("private") || modifiers.contains("fileprivate") { @@ -191,14 +219,14 @@ extension Formatter { // Remove any existing ACL modifier for aclModifier in _FormatRules.aclModifiers { - if let modifierIndex = indexOfModifier(aclModifier, forDeclarationAt: declaration.keywordIndex) { + if let modifierIndex = formatter.indexOfModifier(aclModifier, forDeclarationAt: keywordIndex) { // Replace the modifier with "private" - replaceToken(at: modifierIndex, with: .keyword("private")) + formatter.replaceToken(at: modifierIndex, with: .keyword("private")) return } } // No ACL modifier exists, so add "private" before the keyword - insert([.keyword("private"), .space(" ")], at: declaration.keywordIndex) + formatter.insert([.keyword("private"), .space(" ")], at: keywordIndex) } } diff --git a/Tests/Rules/TestSuiteAccessControlTests.swift b/Tests/Rules/TestSuiteAccessControlTests.swift index 0a5fc1a6..e376509b 100644 --- a/Tests/Rules/TestSuiteAccessControlTests.swift +++ b/Tests/Rules/TestSuiteAccessControlTests.swift @@ -648,4 +648,170 @@ final class TestSuiteAccessControlTests: XCTestCase { // No changes should be made - types with parameterized init are not test suites testFormatting(for: input, rule: .testSuiteAccessControl, exclude: [.unusedArguments, .validateTestCases, .redundantMemberwiseInit]) } + + func testSwiftTestingPrivateVisibilityOption() { + let input = """ + import Testing + + struct MyFeatureTests { + @Test func featureWorks() { + #expect(true) + } + + func helperMethod() { + // helper code + } + } + """ + + let output = """ + import Testing + + private struct MyFeatureTests { + @Test private func featureWorks() { + #expect(true) + } + + private func helperMethod() { + // helper code + } + } + """ + + let options = FormatOptions(testCaseAccessControl: .private) + testFormatting(for: input, output, rule: .testSuiteAccessControl, options: options, exclude: [.unusedArguments]) + } + + func testSwiftTestingPublicVisibilityOption() { + let input = """ + import Testing + + struct MyFeatureTests { + @Test func featureWorks() { + #expect(true) + } + + func helperMethod() { + // helper code + } + } + """ + + let output = """ + import Testing + + public struct MyFeatureTests { + @Test public func featureWorks() { + #expect(true) + } + + private func helperMethod() { + // helper code + } + } + """ + + let options = FormatOptions(testCaseAccessControl: .public) + testFormatting(for: input, output, rule: .testSuiteAccessControl, options: options, exclude: [.unusedArguments]) + } + + func testXCTestPrivateVisibilityFallsBackToInternal() { + let input = """ + import XCTest + + final class MyTests: XCTestCase { + public func testExample() { + XCTAssertTrue(true) + } + + func helperMethod() { + // helper code + } + } + """ + + let output = """ + import XCTest + + final class MyTests: XCTestCase { + func testExample() { + XCTAssertTrue(true) + } + + private func helperMethod() { + // helper code + } + } + """ + + // XCTest doesn't support private tests, so private/fileprivate falls back to internal + let options = FormatOptions(testCaseAccessControl: .private) + testFormatting(for: input, output, rule: .testSuiteAccessControl, options: options, exclude: [.unusedArguments]) + } + + func testXCTestFileprivateVisibilityFallsBackToInternal() { + let input = """ + import XCTest + + final class MyTests: XCTestCase { + public func testExample() { + XCTAssertTrue(true) + } + } + """ + + let output = """ + import XCTest + + final class MyTests: XCTestCase { + func testExample() { + XCTAssertTrue(true) + } + } + """ + + // XCTest doesn't support fileprivate tests, so fileprivate falls back to internal + let options = FormatOptions(testCaseAccessControl: .fileprivate) + testFormatting(for: input, output, rule: .testSuiteAccessControl, options: options, exclude: [.unusedArguments]) + } + + func testSwiftTestingFileprivateVisibilityOption() { + let input = """ + import Testing + + struct MyFeatureTests { + @Test func featureWorks() { + #expect(true) + } + } + """ + + let output = """ + import Testing + + fileprivate struct MyFeatureTests { + @Test fileprivate func featureWorks() { + #expect(true) + } + } + """ + + let options = FormatOptions(testCaseAccessControl: .fileprivate) + testFormatting(for: input, output, rule: .testSuiteAccessControl, options: options, exclude: [.unusedArguments]) + } + + func testSwiftTestingPrivateVisibilityPreservesExistingPrivate() { + let input = """ + import Testing + + private struct MyFeatureTests { + @Test private func featureWorks() { + #expect(true) + } + } + """ + + let options = FormatOptions(testCaseAccessControl: .private) + testFormatting(for: input, rule: .testSuiteAccessControl, options: options, exclude: [.unusedArguments]) + } }