From 024983ab42d5d542b9cf53e5dd567202b5dc9a3f Mon Sep 17 00:00:00 2001 From: Jaewon-Yun Date: Sun, 9 Jun 2024 02:25:20 +0900 Subject: [PATCH] Improve error message (#6377) --- .../TuistGenerator/Linter/ProjectLinter.swift | 6 +- .../TuistGenerator/Linter/TargetLinter.swift | 12 +- Sources/XcodeGraph/Models/Product.swift | 4 +- .../Linter/Mocks/MockTargetLinter.swift | 2 +- .../Linter/TargetLinterTests.swift | 169 ++++++++++++++---- .../XcodeGraphTests/Models/ProductTests.swift | 2 +- 6 files changed, 150 insertions(+), 45 deletions(-) diff --git a/Sources/TuistGenerator/Linter/ProjectLinter.swift b/Sources/TuistGenerator/Linter/ProjectLinter.swift index 25f65da75f1..6bc2bfa790e 100644 --- a/Sources/TuistGenerator/Linter/ProjectLinter.swift +++ b/Sources/TuistGenerator/Linter/ProjectLinter.swift @@ -48,8 +48,8 @@ class ProjectLinter: ProjectLinting { } private func lintTargets(project: Project) -> [LintingIssue] { - var issues: [LintingIssue] = [] - issues.append(contentsOf: project.targets.values.flatMap(targetLinter.lint)) - return issues + return project.targets.values.flatMap { target in + targetLinter.lint(target: target, options: project.options) + } } } diff --git a/Sources/TuistGenerator/Linter/TargetLinter.swift b/Sources/TuistGenerator/Linter/TargetLinter.swift index 028f23b9c50..ab4d6cf30de 100644 --- a/Sources/TuistGenerator/Linter/TargetLinter.swift +++ b/Sources/TuistGenerator/Linter/TargetLinter.swift @@ -4,7 +4,7 @@ import TuistSupport import XcodeGraph protocol TargetLinting: AnyObject { - func lint(target: Target) -> [LintingIssue] + func lint(target: Target, options: Project.Options) -> [LintingIssue] } class TargetLinter: TargetLinting { @@ -25,7 +25,7 @@ class TargetLinter: TargetLinting { // MARK: - TargetLinting - func lint(target: Target) -> [LintingIssue] { + func lint(target: Target, options: Project.Options) -> [LintingIssue] { var issues: [LintingIssue] = [] issues.append(contentsOf: lintProductName(target: target)) issues.append(contentsOf: lintProductNameBuildSettings(target: target)) @@ -33,7 +33,7 @@ class TargetLinter: TargetLinting { issues.append(contentsOf: lintBundleIdentifier(target: target)) issues.append(contentsOf: lintHasSourceFiles(target: target)) issues.append(contentsOf: lintCopiedFiles(target: target)) - issues.append(contentsOf: lintLibraryHasNoResources(target: target)) + issues.append(contentsOf: lintLibraryHasNoResources(target: target, options: options)) issues.append(contentsOf: lintDeploymentTarget(target: target)) issues.append(contentsOf: settingsLinter.lint(target: target)) issues.append(contentsOf: lintDuplicateDependency(target: target)) @@ -220,15 +220,15 @@ class TargetLinter: TargetLinting { return issues } - private func lintLibraryHasNoResources(target: Target) -> [LintingIssue] { + private func lintLibraryHasNoResources(target: Target, options: Project.Options) -> [LintingIssue] { if target.supportsResources { return [] } - if target.resources.resources.isEmpty == false { + if target.resources.resources.isEmpty == false, options.disableBundleAccessors { return [ LintingIssue( - reason: "Target \(target.name) cannot contain resources. \(target.product) targets do not support resources", + reason: "Target \(target.name) cannot contain resources. For \(target.product) targets to support resources, 'Bundle Accessors' feature should be enabled.", severity: .error ), ] diff --git a/Sources/XcodeGraph/Models/Product.swift b/Sources/XcodeGraph/Models/Product.swift index c0c38986466..322a0683d6b 100644 --- a/Sources/XcodeGraph/Models/Product.swift +++ b/Sources/XcodeGraph/Models/Product.swift @@ -88,9 +88,9 @@ public enum Product: String, CustomStringConvertible, CaseIterable, Codable { case .dynamicLibrary: return "dynamic library" case .framework: - return "framework" + return "dynamic framework" case .staticFramework: - return "staticFramework" + return "static framework" case .unitTests: return "unit tests" case .uiTests: diff --git a/Tests/TuistGeneratorTests/Linter/Mocks/MockTargetLinter.swift b/Tests/TuistGeneratorTests/Linter/Mocks/MockTargetLinter.swift index 68c9aef6f78..4e32776d6eb 100644 --- a/Tests/TuistGeneratorTests/Linter/Mocks/MockTargetLinter.swift +++ b/Tests/TuistGeneratorTests/Linter/Mocks/MockTargetLinter.swift @@ -6,7 +6,7 @@ import XcodeGraphTesting @testable import TuistGenerator class MockTargetLinter: TargetLinting { - func lint(target _: Target) -> [LintingIssue] { + func lint(target _: Target, options _: Project.Options) -> [LintingIssue] { [] } } diff --git a/Tests/TuistGeneratorTests/Linter/TargetLinterTests.swift b/Tests/TuistGeneratorTests/Linter/TargetLinterTests.swift index 6d2cf3533a8..06c61c6ee52 100644 --- a/Tests/TuistGeneratorTests/Linter/TargetLinterTests.swift +++ b/Tests/TuistGeneratorTests/Linter/TargetLinterTests.swift @@ -24,7 +24,7 @@ final class TargetLinterTests: TuistUnitTestCase { func test_lint_when_target_has_invalid_product_name() { let XCTAssertInvalidProductNameApp: (Target) -> Void = { target in - let got = self.subject.lint(target: target) + let got = self.subject.lint(target: target, options: .test()) let reason: String switch target.product { case .framework, .staticFramework: @@ -39,7 +39,7 @@ final class TargetLinterTests: TuistUnitTestCase { } let XCTAssertValidProductNameApp: (Target) -> Void = { target in - let got = self.subject.lint(target: target) + let got = self.subject.lint(target: target, options: .test()) XCTAssertNil(got.first(where: { $0.description.contains("Invalid product name") })) } @@ -65,7 +65,7 @@ final class TargetLinterTests: TuistUnitTestCase { )) // When - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) // Then XCTContainsLintingIssue( @@ -86,7 +86,7 @@ final class TargetLinterTests: TuistUnitTestCase { )) // When - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) // Then XCTContainsLintingIssue( @@ -101,14 +101,14 @@ final class TargetLinterTests: TuistUnitTestCase { func test_lint_when_target_has_invalid_bundle_identifier() { let XCTAssertInvalidBundleId: (String) -> Void = { bundleId in let target = Target.test(bundleId: bundleId) - let got = self.subject.lint(target: target) + let got = self.subject.lint(target: target, options: .test()) let reason = "Invalid bundle identifier '\(bundleId)'. This string must be a uniform type identifier (UTI) that contains only alphanumeric (A-Z,a-z,0-9), hyphen (-), and period (.) characters." self.XCTContainsLintingIssue(got, LintingIssue(reason: reason, severity: .error)) } let XCTAssertValidBundleId: (String) -> Void = { bundleId in let target = Target.test(bundleId: bundleId) - let got = self.subject.lint(target: target) + let got = self.subject.lint(target: target, options: .test()) XCTAssertNil(got.first(where: { $0.description.contains("Invalid bundle identifier") })) } @@ -121,7 +121,7 @@ final class TargetLinterTests: TuistUnitTestCase { func test_lint_when_target_no_source_files() { let target = Target.test(sources: []) - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) XCTContainsLintingIssue( got, @@ -133,7 +133,7 @@ final class TargetLinterTests: TuistUnitTestCase { let target = Target.test(sources: [], dependencies: [ TargetDependency.sdk(name: "libc++.tbd", status: .optional), ]) - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) XCTAssertEqual(0, got.count) } @@ -142,7 +142,7 @@ final class TargetLinterTests: TuistUnitTestCase { let target = Target.test(sources: [], scripts: [ TargetScript(name: "Test script", order: .post, script: .embedded("echo 'This is a test'")), ]) - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) XCTAssertEqual(0, got.count) } @@ -161,7 +161,7 @@ final class TargetLinterTests: TuistUnitTestCase { ) ) - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) XCTContainsLintingIssue( got, @@ -183,7 +183,7 @@ final class TargetLinterTests: TuistUnitTestCase { let path = try! AbsolutePath(validating: "/App.entitlements") let target = Target.test(resources: .init([.file(path: path)])) - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) XCTContainsLintingIssue( got, @@ -199,7 +199,7 @@ final class TargetLinterTests: TuistUnitTestCase { let path = temporaryPath.appending(component: "Info.plist") let target = Target.test(infoPlist: .file(path: path)) - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) XCTContainsLintingIssue( got, @@ -212,7 +212,7 @@ final class TargetLinterTests: TuistUnitTestCase { let path = temporaryPath.appending(component: "App.entitlements") let target = Target.test(entitlements: .file(path: path)) - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) XCTContainsLintingIssue( got, @@ -228,20 +228,125 @@ final class TargetLinterTests: TuistUnitTestCase { let staticLibrary = Target.test(product: .staticLibrary, resources: .init([element])) let dynamicLibrary = Target.test(product: .dynamicLibrary, resources: .init([element])) - let staticResult = subject.lint(target: staticLibrary) + let staticLibraryResult = subject.lint( + target: staticLibrary, + options: .test() + ) + XCTDoesNotContainLintingIssue( + staticLibraryResult, + LintingIssue( + reason: "Target \(staticLibrary.name) cannot contain resources. For \(staticLibrary.product) targets to support resources, 'Bundle Accessors' feature should be enabled.", + severity: .error + ) + ) + + let dynamicLibraryResult = subject.lint( + target: dynamicLibrary, + options: .test() + ) + XCTDoesNotContainLintingIssue( + dynamicLibraryResult, + LintingIssue( + reason: "Target \(dynamicLibrary.name) cannot contain resources. For \(dynamicLibrary.product) targets to support resources, 'Bundle Accessors' feature should be enabled.", + severity: .error + ) + ) + } + + func test_lint_when_library_has_resources_with_disable_bundle_accessors() throws { + let temporaryPath = try temporaryPath() + let path = temporaryPath.appending(component: "Image.png") + let element = ResourceFileElement.file(path: path) + + let staticLibrary = Target.test(product: .staticLibrary, resources: .init([element])) + let dynamicLibrary = Target.test(product: .dynamicLibrary, resources: .init([element])) + + let staticLibraryResult = subject.lint( + target: staticLibrary, + options: .test(disableBundleAccessors: true) + ) + XCTContainsLintingIssue( + staticLibraryResult, + LintingIssue( + reason: "Target \(staticLibrary.name) cannot contain resources. For \(staticLibrary.product) targets to support resources, 'Bundle Accessors' feature should be enabled.", + severity: .error + ) + ) + + let dynamicLibraryResult = subject.lint( + target: dynamicLibrary, + options: .test(disableBundleAccessors: true) + ) XCTContainsLintingIssue( - staticResult, + dynamicLibraryResult, + LintingIssue( + reason: "Target \(dynamicLibrary.name) cannot contain resources. For \(dynamicLibrary.product) targets to support resources, 'Bundle Accessors' feature should be enabled.", + severity: .error + ) + ) + } + + func test_lint_when_framework_has_resources() throws { + let temporaryPath = try temporaryPath() + let path = temporaryPath.appending(component: "Image.png") + let element = ResourceFileElement.file(path: path) + + let staticFramework = Target.test(product: .staticFramework, resources: .init([element])) + let dynamicFramework = Target.test(product: .framework, resources: .init([element])) + + let staticFrameworkResult = subject.lint( + target: staticFramework, + options: .test() + ) + XCTDoesNotContainLintingIssue( + staticFrameworkResult, + LintingIssue( + reason: "Target \(staticFramework.name) cannot contain resources. For \(staticFramework.product) targets to support resources, 'Bundle Accessors' feature should be enabled.", + severity: .error + ) + ) + + let dynamicFrameworkResult = subject.lint( + target: dynamicFramework, + options: .test() + ) + XCTDoesNotContainLintingIssue( + dynamicFrameworkResult, LintingIssue( - reason: "Target \(staticLibrary.name) cannot contain resources. static library targets do not support resources", + reason: "Target \(dynamicFramework.name) cannot contain resources. For \(dynamicFramework.product) targets to support resources, 'Bundle Accessors' feature should be enabled.", severity: .error ) ) + } + + func test_lint_when_framework_has_resources_with_disable_bundle_accessors() throws { + let temporaryPath = try temporaryPath() + let path = temporaryPath.appending(component: "Image.png") + let element = ResourceFileElement.file(path: path) + + let staticFramework = Target.test(product: .staticFramework, resources: .init([element])) + let dynamicFramework = Target.test(product: .framework, resources: .init([element])) - let dynamicResult = subject.lint(target: dynamicLibrary) + let staticFrameworkResult = subject.lint( + target: staticFramework, + options: .test(disableBundleAccessors: true) + ) XCTContainsLintingIssue( - dynamicResult, + staticFrameworkResult, + LintingIssue( + reason: "Target \(staticFramework.name) cannot contain resources. For \(staticFramework.product) targets to support resources, 'Bundle Accessors' feature should be enabled.", + severity: .error + ) + ) + + let dynamicFrameworkResult = subject.lint( + target: dynamicFramework, + options: .test(disableBundleAccessors: true) + ) + XCTDoesNotContainLintingIssue( + dynamicFrameworkResult, LintingIssue( - reason: "Target \(dynamicLibrary.name) cannot contain resources. dynamic library targets do not support resources", + reason: "Target \(dynamicFramework.name) cannot contain resources. For \(dynamicFramework.product) targets to support resources, 'Bundle Accessors' feature should be enabled.", severity: .error ) ) @@ -259,7 +364,7 @@ final class TargetLinterTests: TuistUnitTestCase { ) // When - let result = subject.lint(target: bundle) + let result = subject.lint(target: bundle, options: .test()) // Then XCTContainsLintingIssue( @@ -281,7 +386,7 @@ final class TargetLinterTests: TuistUnitTestCase { ) // When - let result = subject.lint(target: bundle) + let result = subject.lint(target: bundle, options: .test()) // Then XCTAssertTrue(result.isEmpty) @@ -300,7 +405,7 @@ final class TargetLinterTests: TuistUnitTestCase { ) // When - let result = subject.lint(target: bundle) + let result = subject.lint(target: bundle, options: .test()) // Then XCTAssertTrue(result.isEmpty) @@ -313,7 +418,7 @@ final class TargetLinterTests: TuistUnitTestCase { let target = Target.test(platform: .macOS, deploymentTarget: .macOS(version)) // When - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) // Then XCTDoesNotContainLintingIssue( @@ -346,7 +451,7 @@ final class TargetLinterTests: TuistUnitTestCase { ] for target in targets { - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) // Then XCTDoesNotContainLintingIssue( @@ -366,7 +471,7 @@ final class TargetLinterTests: TuistUnitTestCase { let target = Target.test(platform: .macOS, deploymentTarget: .macOS(version)) // When - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) // Then XCTContainsLintingIssue(got, LintingIssue(reason: "The version of deployment target is incorrect", severity: .error)) @@ -385,7 +490,7 @@ final class TargetLinterTests: TuistUnitTestCase { let target = Target.test(platform: combinations.0, deploymentTarget: combinations.1) // When - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) let expectedPlatform = try XCTUnwrap(combinations.1.configuredVersions.first?.platform.caseValue) // Then @@ -407,7 +512,7 @@ final class TargetLinterTests: TuistUnitTestCase { ] // When - let got = invalidTargets.flatMap { subject.lint(target: $0) } + let got = invalidTargets.flatMap { subject.lint(target: $0, options: .test()) } // Then let expectedIssues: [LintingIssue] = [ @@ -430,7 +535,7 @@ final class TargetLinterTests: TuistUnitTestCase { let target = Target.test(dependencies: .init(repeating: testDependency, count: 2)) // When - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) // Then XCTContainsLintingIssue(got, .init( @@ -448,7 +553,7 @@ final class TargetLinterTests: TuistUnitTestCase { ]) // When - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) // Then XCTContainsLintingIssue(got, .init( @@ -468,7 +573,7 @@ final class TargetLinterTests: TuistUnitTestCase { ]) // When - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) // Then XCTContainsLintingIssue(got, .init( @@ -491,7 +596,7 @@ final class TargetLinterTests: TuistUnitTestCase { ) // When - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) // Then XCTContainsLintingIssue(got, .init( @@ -510,7 +615,7 @@ final class TargetLinterTests: TuistUnitTestCase { ) // When - let got = subject.lint(target: target) + let got = subject.lint(target: target, options: .test()) // Then XCTContainsLintingIssue(got, .init( diff --git a/Tests/XcodeGraphTests/Models/ProductTests.swift b/Tests/XcodeGraphTests/Models/ProductTests.swift index d7b34943505..da4b1f85637 100644 --- a/Tests/XcodeGraphTests/Models/ProductTests.swift +++ b/Tests/XcodeGraphTests/Models/ProductTests.swift @@ -36,7 +36,7 @@ final class ProductTests: XCTestCase { XCTAssertEqual(Product.app.description, "application") XCTAssertEqual(Product.staticLibrary.description, "static library") XCTAssertEqual(Product.dynamicLibrary.description, "dynamic library") - XCTAssertEqual(Product.framework.description, "framework") + XCTAssertEqual(Product.framework.description, "dynamic framework") XCTAssertEqual(Product.unitTests.description, "unit tests") XCTAssertEqual(Product.uiTests.description, "ui tests") XCTAssertEqual(Product.appExtension.description, "app extension")