Add test plan support within Tuist test #5135

stephanecopin · 2023-04-05T19:22:21Z

Resolves #5080

Short description 📝

Add 5 new flags to tuist test:

One to use test plan (--test-plan)
Two to specify which configurations to use (--only-test-configuration, --skip-test-configuration)
Two to specify which tests to use (--only-testing, --skip-testing)

The changes are integrated with the Tuist graph & cache system, and will be optimized to build only what's changed.

Open to any suggestions regarding to naming the flags, or if they should be handled differently that I've implemented in the PR.
Implementation-wide:

Would like people knowing how the test caching works to peek at it and let me know if there's anything that looks off. I based my changes mostly by analyzing what the code did and adapting it to work with Test Plans, while maintaining backward compatibility.
I've had to create new helpers in multiple places. Let me know if they don't belong in one place or the other and I'll move them.

How to test the changes locally 🧐

Changes are unit tested and can also be tested using ./fourier test tuist acceptance projects/tuist/features/test-plan.feature

Contributor checklist ✅

The code has been linted using run ./fourier lint tuist --fix
The change is tested via unit testing or acceptance testing, or both
The title of the PR is formulated in a way that is usable as a changelog entry
In case the PR introduces changes that affect users, the documentation has been updated

Reviewer checklist ✅

The code architecture and patterns are consistent with the rest of the codebase
Reviewer has checked that, if needed, the PR includes the label changelog:added, changelog:fixed, or changelog:changed, and the title is usable as a changelog entry

Sources/TuistCache/Mappers/Graph/FocusTargetsGraphMappers.swift

Sources/TuistCache/Mappers/Graph/TestsCacheGraphMapper.swift

shahzadmajeed · 2023-04-11T16:06:08Z

Sources/TuistCore/Graph/GraphTraverser.swift

+        basedOn targets: GraphTargets,
+        testPlan: String?,
+        includedTargets: Set<String>,
+        excludedTargets: Set<String>,


Is there a need to check for an intersection between includedTargets & excludedTargets so that we can throw an early error about misconfiguration?

The way it works right now, is that includeTargets takes precedence over excludedTargets in that case.
This is on purpose, I could update the behavior to throw instead but that could complicate the code in other places. What do you think?

As commented elsewhere, we should catch the error in the command, and map the input to an enum to only allow valid combinations

Let me know if you should we should still do that considering my comments below.

There could still be errors caught there for configurations that don't make sense, for example if includedTargets & excludedTargets's intersection is not empty, as it in this case the user would to include and exclude the same targets. Down to implement this check if we want it

yep, I would implement it in the command

@danyf90 I've added validation of that to the command

projects/docs/docs/commands/test.md

Sources/TuistAutomation/Utilities/BuildGraphInspector.swift

Sources/TuistKit/Commands/TestCommand.swift

Sources/TuistAutomation/Utilities/BuildGraphInspector.swift

Sources/TuistCache/Mappers/Graph/FocusTargetsGraphMappers.swift

danieleformichelli · 2023-04-28T20:51:47Z

Sources/TuistKit/Services/TestService.swift

+        testPlan: String?,
+        testTargets: [TestIdentifier],
+        skipTestTargets: [TestIdentifier],
+        testConfigurations: [String],
+        skipTestConfigurations: [String]


Aren't these all related (meaning if testPlan is nil the others are ignored)? If yes, we should model it as an enum with associated values

testTargets & skipTestTargets can be used without a test plan, however testConfigurations & skipTestConfigurations can only be used when a test plan is specified. I can perhaps refactor them as a struct in this case? Like TestPlanConfiguration with the 3 fields

danieleformichelli · 2023-04-28T20:58:01Z

Sources/TuistCore/Graph/GraphTraverser.swift

+        basedOn targets: GraphTargets,
+        testPlan: String?,
+        includedTargets: Set<String>,
+        excludedTargets: Set<String>,


As commented elsewhere, we should catch the error in the command, and map the input to an enum to only allow valid combinations

danieleformichelli · 2023-04-28T20:59:53Z

Sources/TuistCore/Graph/GraphTraverser.swift

@@ -52,6 +52,41 @@ public class GraphTraverser: GraphTraversing {
        allTargets(excludingExternalTargets: true)
    }

+    public func filterIncludedTargets<GraphTargets: Collection>(


This logic is related to caching, so if possible I would define it in TuistCache module, possibly as an extension of the GraphTraverser

Works for me, will move it in the module.

Sources/TuistCore/Graph/GraphTraverser.swift

danieleformichelli · 2023-05-31T06:18:55Z

Sources/TuistCache/Mappers/Graph/FocusTargetsGraphMappers.swift

+    public init(
+        testPlan: String? = nil,
+        includedTargets: Set<String>,
+        excludedTargets: Set<String> = []
+    ) {


avoid defaults in internal methods, they just risk you forget passing parameters

Suggested change

public init(

testPlan: String? = nil,

includedTargets: Set<String>,

excludedTargets: Set<String> = []

) {

public init(testPlan: String?, includedTargets: Set<String>, excludedTargets: Set<String>) {

danieleformichelli · 2023-05-31T06:21:54Z

Sources/TuistKit/Commands/TestCommand.swift

+    @Option(
+        name: .long,
+        parsing: .upToNextOption,
+        help: "The list of test identifiers you want to test. Takes precedence over --skip-testing",


Suggested change

help: "The list of test identifiers you want to test. Takes precedence over --skip-testing",

help: "The list of test identifiers you want to test. Expected format is TestTarget[/TestClass[/TestMethod]]. It is applied before --skip-testing",

danieleformichelli · 2023-05-31T06:22:02Z

Sources/TuistKit/Commands/TestCommand.swift

+    @Option(
+        name: .long,
+        parsing: .upToNextOption,
+        help: "The list of test identifiers you want to skip testing.",


Suggested change

help: "The list of test identifiers you want to skip testing.",

help: "The list of test identifiers you want to skip testing. Expected format is TestTarget[/TestClass[/TestMethod]].",

danieleformichelli · 2023-05-31T06:22:21Z

Sources/TuistKit/Commands/TestCommand.swift

+    @Option(
+        name: .long,
+        parsing: .upToNextOption,
+        help: "The list of configurations you want to test. Takes precedence over --skip-test-configuration"


Suggested change

help: "The list of configurations you want to test. Takes precedence over --skip-test-configuration"

help: "The list of configurations you want to test. It is applied before --skip-test-configuration"

danieleformichelli · 2023-05-31T06:23:04Z

Sources/TuistKit/Commands/TestCommand.swift

+    @Option(
+        name: .long,
+        parsing: .upToNextOption,
+        help: "The list of configurations you want to test. Takes precedence over --skip-test-configuration"
+    )
+    var testConfigurations: [String] = []
+
+    @Option(
+        name: .long,
+        parsing: .upToNextOption,
+        help: "The list of configurations you want to skip testing."
+    )
+    var skipTestConfigurations: [String] = []


How does it work if you specify both of these?

Not sure, I'll double check

danieleformichelli · 2023-05-31T06:23:57Z

Sources/TuistKit/Commands/TestCommand.swift

+    func validate() throws {
+        let intersection = Set(testTargets).intersection(skipTestTargets)
+        if !intersection.isEmpty {
+            throw ValidationError.invalidTestTargetsOptions(intersection)
+        }
+    }


There are also other invalid scenario. For example, if testTargets is [TargetName] it is invalid/wrong to have skipTestTarget as [AnotherTargetName/ClassName], as it doesn't skip anything from the selected set

Indeed, though I'm not sure this would be considered an error as technically AnotherTargetName/ClassName will be skipped. Maybe we could output some some of warning instead?

It's cleary an error in passing the parameter, better to let the user know by making the command fail

danieleformichelli · 2023-05-31T06:26:08Z

projects/docs/docs/commands/test.md

+| `--result-bundle-path`       | `-T`  | `Path where test result bundle will be saved`                                                                                          |                   | No       |
+| `--retry-count`              | n/a   | `Tests will retry <number> of times until they succeed.`                                                                               | 0                 |          |
+| `--test-plan`                | n/a   | `When passed, the test plan to run. The scheme parameter is required when a test plan is passed.`                                      |                   | No       |
+| `--test-targets`             | n/a   | `Specify which tests should run in the test-identifier* format. Takes precedence over --skip-testing`                                  |                   | No       |


Suggested change

| `--test-targets` | n/a | `Specify which tests should run in the test-identifier* format. Takes precedence over --skip-testing` | | No |

| `--test-targets` | n/a | `Specify which tests should run in the test-identifier* format. It is applied before --skip-testing` | | No |

danieleformichelli · 2023-05-31T06:26:18Z

projects/docs/docs/commands/test.md

+| `--test-plan`                | n/a   | `When passed, the test plan to run. The scheme parameter is required when a test plan is passed.`                                      |                   | No       |
+| `--test-targets`             | n/a   | `Specify which tests should run in the test-identifier* format. Takes precedence over --skip-testing`                                  |                   | No       |
+| `--skip-test-targets`        | n/a   | `Specify which tests should not run be passed in the test-identifier* format.`                                                         |                   | No       |
+| `--test-configurations`      | n/a   | `When a test plan is specified, specify which test configurations to use for the run. Takes precedence over --skip-test-configuration` |                   | No       |


Suggested change

| `--test-configurations` | n/a | `When a test plan is specified, specify which test configurations to use for the run. Takes precedence over --skip-test-configuration` | | No |

| `--test-configurations` | n/a | `When a test plan is specified, specify which test configurations to use for the run. It is applied before --skip-test-configuration` | | No |

pepicrft

Thanks for contributing this one @stephanecopin 🙏🏼. I went through it and left a handful of suggestions. Let me know when they are address and I'll give the PR another pass.

pepicrft · 2023-07-06T10:35:42Z

Sources/TuistKit/Commands/TestCommand.swift

+    @Option(
+        name: .long,
+        parsing: .upToNextOption,
+        help: "The list of test identifiers you want to test. Expected format is TestTarget[/TestClass[/TestMethod]]. It is applied before --skip-testing",
+        transform: TestIdentifier.init(string:)
+    )
+    var testTargets: [TestIdentifier] = []
+
+    @Option(
+        name: .long,
+        parsing: .upToNextOption,
+        help: "The list of test identifiers you want to skip testing. Expected format is TestTarget[/TestClass[/TestMethod]].",
+        transform: TestIdentifier.init(string:)
+    )
+    var skipTestTargets: [TestIdentifier] = []


What about merging these two options into one using the glob convention? Something along the lines of:

tuist test --filter-targets Target*,!OtherTarget

If it's technically possible, I'd go down this path. Otherwise we have to explain people through the help of the flag in which order the filters are applied, which will most likely go unnoticed for many users.
If it's technically possible, I'd do the same with test-configuration:

tuist test --filter-targets ... --filter-test-configurations ...

@pepicrft this would be ideal, but unfortunately is not actually possible with the current granularity. The value of the options is passed through xcodebuild as-is which handles running the right tests.

In order to use globs, we would need to know 3 pieces of info:

The list of targets, which we have access to through the project file (and test plan if provided)

The list of test cases in the target, which is not easy to figure out.
We could:

Approximate that by assuming that all files ending in Tests.swift have a test case of the same name, and use the project file to enumerate all tests

Use SwiftSyntax to parse all swift files in the test target and infer the test cases from there

The list of individual tests in a test case. The only way I could see would be to use SwiftSyntax there as well.

TL;DR using globs would basically have us recreate functionality build-in xcodebuild.

pepicrft · 2023-07-06T10:36:45Z

Sources/TuistKit/Commands/TestCommand.swift

+        parsing: .upToNextOption,
+        help: "The list of configurations you want to test. It is applied before --skip-test-configuration"
+    )
+    var testConfigurations: [String] = []


Since this flag is used as part of the test command, would it make sense to remove some redundancy in the naming?

tuist test --filter-configurations # instead of tuist test --filter-test-configurations

That works for me!

pepicrft · 2023-07-06T10:47:06Z

Sources/TuistKit/Services/TestService.swift

+        case let .testPlanNotFound(scheme, testPlan, existing):
+            let existingMessage: String
+            if existing.isEmpty {
+                existingMessage = "No test plans are defined for this scheme"


Nitpick: For someone writing or reading this code it's obvious what "this" refers to, but it might not be for the user receiving the following error:

No test plans are defined for this scheme

Which scheme? Since you have it already in the enum case, I'd include it:

We could not execute the test plan X because the scheme Y doesn't have test plans defined.

pepicrft · 2023-07-06T10:48:19Z

Sources/TuistKit/Services/TestService.swift

+            if existing.isEmpty {
+                existingMessage = "No test plans are defined for this scheme"
+            } else {
+                existingMessage = "The available test plans are: \(existing.joined(separator: ","))"


And same here, add as much context as possible for the user to understand the error. What scheme are we talking about? What if we match the testPlan against all the existing and suggest one of them if it's very close to the one given by the user? It's possible they made a typo while invoking the CLI.

Works for me, I'll add the info about the scheme.

As for matching test plan against existing ones using some sort a fuzzy search, I'd rather keep that as a separate improvement as the same logic could be applied in other places (e.g. the scheme above, but probably other places within Tuist), let me know what you think! We can add an improvement issue so we don't forgot about it.

pepicrft · 2023-07-06T10:51:32Z

Sources/TuistKit/Services/TestService.swift

                logger.log(level: .info, "There are no tests to run, finishing early")
                return
+            case (_?, _, true), (_?, _, nil):
+                logger.log(level: .info, "There are no test plans to run, finishing early")


This and the above case are not the same so I'd expect the message to be different. Assuming I read the code well, this one should be something more like:

The scheme X's test action Y has no test plans to run, finishing early.

pepicrft · 2023-07-06T11:24:14Z

Sources/TuistAutomation/Utilities/BuildGraphInspector.swift

+    public func testableTarget(
+        scheme: Scheme,
+        testPlan: String?,
+        testTargets: [TestIdentifier],
+        skipTestTargets: [TestIdentifier],
+        graphTraverser: GraphTraversing
+    ) -> GraphTarget? {
+        func isIncluded(_ testTarget: TestPlan.TestTarget) -> Bool {
+            if !testTarget.isEnabled {
+                return false
+            } else if testTargets.isEmpty {
+                return !skipTestTargets.contains { $0.target == testTarget.target.name }
+            } else {
+              return testTargets.contains { $0.target == testTarget.target.name }
+            }
+        }
+
+        if let testPlanName = testPlan,
+           let testPlan = scheme.testAction?.testPlans?.first(where: { $0.name == testPlanName }),
+           let target = testPlan.testTargets.first(where: { isIncluded($0) })?.target
+        {
+            return graphTraverser.target(path: target.projectPath, name: target.name)
+        } else if let testTarget = scheme.testAction?.targets.first {
+            return graphTraverser.target(path: testTarget.target.projectPath, name: testTarget.target.name)
+        } else {
+            return nil
+        }


I'd add unit tests that cover the various scenarios in this method.

pepicrft · 2023-07-06T11:25:09Z

Sources/TuistAutomation/Utilities/BuildGraphInspector.swift

@@ -122,7 +153,7 @@ public final class BuildGraphInspector: BuildGraphInspecting {

    public func testableSchemes(graphTraverser: GraphTraversing) -> [Scheme] {
        graphTraverser.schemes()
-            .filter { $0.testAction?.targets.isEmpty == false }
+            .filter { $0.testAction?.targets.isEmpty == false || $0.testAction?.testPlans?.isEmpty == false }


I'd also add a unit test that covers this one.

pepicrft · 2023-07-06T11:25:28Z

Sources/TuistCore/Graph/GraphTraverser.swift

@@ -52,6 +52,10 @@ public class GraphTraverser: GraphTraversing {
        allTargets(excludingExternalTargets: true)
    }

+    public func allTestPlans() -> Set<TestPlan> {


I'd add tests for the new functions in this class.

pepicrft · 2023-07-06T11:27:32Z

Sources/TuistLoader/Models/XCTestPlan.swift

@@ -0,0 +1,49 @@
+struct XCTestPlan: Decodable {


I'd import Foundation here. Is there a reason why this file is not in the TuistGraph target where all the other modules live?

I'd also add coddle tests like we are doing with the other models.

This is an internal object representing the JSON schema for .xctestplan files. It's used exclusively within TuistLoader (which loads it from the disk), and then TuistLoader uses TuistGraph's TestPlan to build a Tuist-representation of the test plan (see the file right above).
Let me know if that makes sense to you, I can add some comments to clarify if needed!

There is discussion to improve that so Test Plan are generated by Tuist directly (so it wouldn't need to be loaded anymore) here, and I think this should be done eventually but also think it's out of scope for this PR.

pepicrft · 2023-07-06T11:29:53Z

Sources/TuistLoader/Models+ManifestMappers/TestAction+ManifestMapper.swift

@@ -89,3 +89,26 @@ extension TuistGraph.TestAction {
        )
    }
 }
+
+extension TestPlan {


Let's be consistent with the other {Model}+ManifestMapper.swift and extract this one into a TestPlan+ManifestMapper.swift file.

stephanecopin

Thanks @pepicrft for your review! I've answer to the relevant comments, and will address your feedback. Not sure when I'll be able to, I'll keep you posted!

stephanecopin · 2023-07-06T18:49:26Z

Sources/TuistCore/Models/TestIdentifier.swift

+        var description: String {
+            switch self {
+            case let .invalidTestIdentifier(value):
+                return "Invalid test identifiers \(value). The expected format is TestTarget[/TestClass[/TestMethod]]."


It is indeed

stephanecopin · 2023-07-06T18:51:13Z

Sources/TuistKit/Commands/TestCommand.swift

+    @Option(
+        name: .long,
+        parsing: .upToNextOption,
+        help: "The list of test identifiers you want to test. Expected format is TestTarget[/TestClass[/TestMethod]]. It is applied before --skip-testing",
+        transform: TestIdentifier.init(string:)
+    )
+    var testTargets: [TestIdentifier] = []
+
+    @Option(
+        name: .long,
+        parsing: .upToNextOption,
+        help: "The list of test identifiers you want to skip testing. Expected format is TestTarget[/TestClass[/TestMethod]].",
+        transform: TestIdentifier.init(string:)
+    )
+    var skipTestTargets: [TestIdentifier] = []


@pepicrft this would be ideal, but unfortunately is not actually possible with the current granularity. The value of the options is passed through xcodebuild as-is which handles running the right tests.

In order to use globs, we would need to know 3 pieces of info:

The list of targets, which we have access to through the project file (and test plan if provided)

The list of test cases in the target, which is not easy to figure out.
We could:

Approximate that by assuming that all files ending in Tests.swift have a test case of the same name, and use the project file to enumerate all tests

Use SwiftSyntax to parse all swift files in the test target and infer the test cases from there

The list of individual tests in a test case. The only way I could see would be to use SwiftSyntax there as well.

TL;DR using globs would basically have us recreate functionality build-in xcodebuild.

stephanecopin · 2023-07-06T18:51:58Z

Sources/TuistKit/Commands/TestCommand.swift

+        parsing: .upToNextOption,
+        help: "The list of configurations you want to test. It is applied before --skip-test-configuration"
+    )
+    var testConfigurations: [String] = []


That works for me!

stephanecopin · 2023-07-06T18:52:10Z

Sources/TuistKit/Commands/TestCommand.swift

+    func validate() throws {
+        let targetsIntersection = Set(testTargets)
+            .intersection(skipTestTargets)
+        if !targetsIntersection.isEmpty {
+            throw ValidationError.duplicatedTestTargets(targetsIntersection)
+        }
+        if !testTargets.isEmpty {
+            // --test-targets Test --skip-test-targets AnotherTest
+            let skipTestTargetsOnly = Set(skipTestTargets.map { TestIdentifier(target: $0.target) })
+            let testTargetsOnly = testTargets.map { TestIdentifier(target: $0.target) }
+            let targetsOnlyIntersection = skipTestTargetsOnly.intersection(testTargetsOnly)
+            if targetsOnlyIntersection.isEmpty {
+                throw ValidationError.nothingToSkip(skipped: skipTestTargets.filter { skipTarget in !testTargetsOnly.contains(TestIdentifier(target: skipTarget.target)) }, included: testTargets)
+            }
+
+            // --test-targets Test/MyTest --skip-test-targets Test/AnotherTest
+            let skipTestTargetsClasses = Set(skipTestTargets.map { TestIdentifier(target: $0.target, class: $0.class) })
+            let testTargetsClasses = testTargets.map { TestIdentifier(target: $0.target, class: $0.class) }
+            let targetsClassesIntersection = skipTestTargetsClasses.intersection(testTargetsClasses)
+                .intersection(testTargetsClasses.map { TestIdentifier(target: $0.target, class: $0.class) })
+            if targetsClassesIntersection.isEmpty {
+                throw ValidationError.nothingToSkip(skipped: skipTestTargets.filter { skipTarget in !testTargetsClasses.contains { $0 == TestIdentifier(target: skipTarget.target, class: skipTarget.class) } }, included: testTargets)
+            }
+        }
+    }


I can extract it to another place indeed

stephanecopin · 2023-07-06T18:56:59Z

Sources/TuistCache/Mappers/Graph/FocusTargetsGraphMappers.swift

@@ -5,19 +5,33 @@ import TuistGraph

 /// `FocusTargetsGraphMappers` is used to filter out some targets and their dependencies and tests targets.
 public final class FocusTargetsGraphMappers: GraphMapping {
+    // When specified, if includedTargets is empty it will automatically include all targets in the test plan


Indeed, why does that make you wary?

In this context includedTargets is a filter onto either the entire test suite (if no test plans are available) or a filter onto the test plan (if provided), a test plan being itself a subset of the entire test suite.

Does that make sense to you?

stephanecopin · 2023-07-06T19:26:50Z

Sources/TuistCache/Extensions/GraphTraverser+Extras.swift

+        return Set(
+            targets.filter { target in
+                if !includedTargets.isEmpty {
+                    return includedTargets.contains(target.target.name)
+                }
+                if excludedTargets.contains(target.target.name) {
+                    return false
+                }
+                if let allTestPlansTargetNames = allTestPlansTargetNames {
+                    return allTestPlansTargetNames.contains(target.target.name)
+                }
+                return excludingExternalTargets ? allInternalTargets.contains(target.target.name) : true
+            }
+        )
+    }


Ah, nice catch! Indeed, the logic should be:

1. If a test plan is specified, if the target target is not in the test plan, exclude it
2. Include the target target if it's included in in includedTargets.
3. Exclude it if it's included in excludedTargets.
4. If we're excluding external targets, only include the target if internal. If we're including external targets, include the target.

Does that make more sense?

As for the name of the function, is it fine with that new logic, and otherwise what would you suggest? That's the best I had 😅

stephanecopin · 2023-07-06T19:33:27Z

Sources/TuistKit/Services/TestService.swift

+            if existing.isEmpty {
+                existingMessage = "No test plans are defined for this scheme"
+            } else {
+                existingMessage = "The available test plans are: \(existing.joined(separator: ","))"


Works for me, I'll add the info about the scheme.

As for matching test plan against existing ones using some sort a fuzzy search, I'd rather keep that as a separate improvement as the same logic could be applied in other places (e.g. the scheme above, but probably other places within Tuist), let me know what you think! We can add an improvement issue so we don't forgot about it.

stephanecopin · 2023-07-06T19:42:50Z

Sources/TuistLoader/Models/XCTestPlan.swift

@@ -0,0 +1,49 @@
+struct XCTestPlan: Decodable {


This is an internal object representing the JSON schema for .xctestplan files. It's used exclusively within TuistLoader (which loads it from the disk), and then TuistLoader uses TuistGraph's TestPlan to build a Tuist-representation of the test plan (see the file right above).
Let me know if that makes sense to you, I can add some comments to clarify if needed!

There is discussion to improve that so Test Plan are generated by Tuist directly (so it wouldn't need to be loaded anymore) here, and I think this should be done eventually but also think it's out of scope for this PR.

stephanecopin · 2023-07-06T19:44:59Z

Tests/TuistCacheTests/Extensions/GraphTraverser+ExtrasTests.swift

+@testable import TuistCache
+
+class GraphTraverserExtrasTests: XCTestCase {
+    func test_filterIncludedTargets_includeExcluded() throws {


Yeah, my intent here was to test all the possible scenarii involved using filterIncludedTargets. I can split them up

stephanecopin · 2023-07-06T19:48:26Z

Tests/TuistCacheTests/Extensions/GraphTraverser+ExtrasTests.swift

+        // When
+        filteredTargets = subject.filterIncludedTargets(
+            basedOn: subject.allTargets(),
+            testPlan: nil,
+            includedTargets: [tests1.name],
+            excludedTargets: []
+        )
+
+        // Then
+        XCTAssertEqual(filteredTargets.map(\.target), [tests1])
+
+        // When
+        filteredTargets = subject.filterIncludedTargets(
+            basedOn: subject.allTargets(),
+            testPlan: nil,
+            includedTargets: [tests1.name],
+            excludedTargets: [tests1.name]
+        )


Works for me! My intent initially was to avoid duplicating the inputs of tests too much. I'll see if I can replace them with properties and not make it too confusing.

fortmarek

Thanks a lot for this PR 👏 It's a great contribution

fortmarek · 2023-07-10T19:16:17Z

Sources/TuistGraph/Models/TestPlan.swift

@@ -1,12 +1,26 @@
 import Foundation
 import TSCBasic

-public struct TestPlan: Equatable, Codable {
+public struct TestPlan: Hashable, Codable {
+    public struct TestTarget: Hashable, Codable {


Can we reuse the TestableTarget model instead of creating a new one?

I'm fine with using it, reversing isEnabled with !isSkipped. I'll do the change

fortmarek · 2023-07-10T20:19:02Z

Sources/TuistKit/Commands/TestCommand.swift

+    func validate() throws {
+        let targetsIntersection = Set(testTargets)
+            .intersection(skipTestTargets)
+        if !targetsIntersection.isEmpty {
+            throw ValidationError.duplicatedTestTargets(targetsIntersection)
+        }
+        if !testTargets.isEmpty {
+            // --test-targets Test --skip-test-targets AnotherTest
+            let skipTestTargetsOnly = Set(skipTestTargets.map { TestIdentifier(target: $0.target) })
+            let testTargetsOnly = testTargets.map { TestIdentifier(target: $0.target) }
+            let targetsOnlyIntersection = skipTestTargetsOnly.intersection(testTargetsOnly)
+            if targetsOnlyIntersection.isEmpty {
+                throw ValidationError.nothingToSkip(skipped: skipTestTargets.filter { skipTarget in !testTargetsOnly.contains(TestIdentifier(target: skipTarget.target)) }, included: testTargets)
+            }
+
+            // --test-targets Test/MyTest --skip-test-targets Test/AnotherTest
+            let skipTestTargetsClasses = Set(skipTestTargets.map { TestIdentifier(target: $0.target, class: $0.class) })
+            let testTargetsClasses = testTargets.map { TestIdentifier(target: $0.target, class: $0.class) }
+            let targetsClassesIntersection = skipTestTargetsClasses.intersection(testTargetsClasses)
+                .intersection(testTargetsClasses.map { TestIdentifier(target: $0.target, class: $0.class) })
+            if targetsClassesIntersection.isEmpty {
+                throw ValidationError.nothingToSkip(skipped: skipTestTargets.filter { skipTarget in !testTargetsClasses.contains { $0 == TestIdentifier(target: skipTarget.target, class: skipTarget.class) } }, included: testTargets)
+            }
+        }
+    }


TestService would most likely be the right place. Or a new utility class.

fortmarek · 2023-07-10T20:21:54Z

Sources/TuistCache/Extensions/GraphTraverser+Extras.swift

+            allTestPlansTargetNames = nil
+        }
+
+        lazy var allInternalTargets = allInternalTargets().map(\.target.name)


Why is this var lazy?

It is lazy as it is not used in all code path, so it's initialized only when used.

fortmarek · 2023-07-10T20:26:01Z

Sources/TuistCore/Models/TestIdentifier.swift

+    }
+
+    public let target: String
+    public let `class`: String?


Maybe we should rename this to className , so we don't have to escape it everywhere?

fortmarek · 2023-07-10T20:26:48Z

Sources/TuistCore/Models/TestIdentifier.swift

+        self.method = method
+    }
+
+    public init(string: String) throws {


Might be worth testing this init

fortmarek · 2023-07-10T20:31:26Z

projects/tuist/features/step_definitions/test.rb

@@ -1,47 +1,54 @@
 # frozen_string_literal: true

-require "xcodeproj"
+require 'xcodeproj'


nit: I don't see a reason for the change of quotation marks? Makes it harder to follow the actual changes here. In the other step_definitions files, we tend to use " instead of '

@fortmarek ah sorry about that, I have rubocop installed globally which do these changes automatically. I'll revert them. As an improvement, it may be good to such rules for Tuist so the style is kept consistency across all Ruby files?

fortmarek · 2023-07-10T20:38:57Z

projects/tuist/fixtures/app_with_test_plan/All.xctestplan

Is this something we expect developers to be able to generate themselves? Does this PR provide value to tuist users until we deliver on generating the test plan functionality (#5214)?

@fortmarek these will be generated by the user via Xcode. The PR allows to use test plans when using tuist test (which is not possible currently), so this is the value the PR provide. This is independent from being able to create test plans using Tuist.

stephanecopin · 2023-08-04T21:07:09Z

Hey @pepicrft @fortmarek @danyf90, I've finally got around to updating the PR, and should have addressed all of your comments. Please let me know if I missed anything! There was a bunch of additions, especially related to unit tests 😅

stephanecopin force-pushed the stephane/test-plan-support branch from db0c71b to 3d346e2 Compare April 11, 2023 13:57

shahzadmajeed reviewed Apr 11, 2023

View reviewed changes

danieleformichelli mentioned this pull request Apr 14, 2023

Add --additional-parameters option to passthrough following arguments/options/flags to the xcodebuild invocation #5133

Closed

6 tasks

danieleformichelli requested changes Apr 28, 2023

View reviewed changes

stephanecopin force-pushed the stephane/test-plan-support branch from 3d346e2 to f4fbb11 Compare April 30, 2023 16:21

stephanecopin marked this pull request as ready for review May 1, 2023 09:23

danieleformichelli requested changes May 31, 2023

View reviewed changes

danieleformichelli requested review from pepicrft, fortmarek and kwridan June 7, 2023 08:55

danieleformichelli approved these changes Jul 3, 2023

View reviewed changes

stephanecopin mentioned this pull request Jul 3, 2023

Update test command documentation tuist/old-docs#9

Merged

pepicrft requested changes Jul 6, 2023

View reviewed changes

stephanecopin commented Jul 6, 2023

View reviewed changes

danieleformichelli requested a review from pepicrft July 7, 2023 07:27

fortmarek reviewed Jul 10, 2023

View reviewed changes

stephanecopin mentioned this pull request Jul 11, 2023

Support XCTest Plan use cases in tuist test #5080

Closed

stephanecopin added 13 commits August 4, 2023 11:32

Add test plan support within Tuist test

9f5033b

Add fixture for test plan

1274230

Refactor Test Plan

90b5da6

Lint files

452e14f

Update mocks

629d2f7

Rename options according to Daniele Formichelli's recommendations

0191a31

Add missing word

1f4dd38

Slight refactor to use intermediate variable in TestsCacheGraphMapper

4085019

Apply nit suggestions

8c43e9b

Refactor filterIncludedTargets in TuistCache

d7116c9

Move XCTestPlan to its own file

f2a0415

Add unit tests for filterIncludedTargets()

4bfc140

Add validation error when 2 identical test identifiers are passed

2137a2f

stephanecopin added 11 commits August 4, 2023 11:32

Apply suggestions

74da6d9

Implement validation

e3c5ed2

Refactor TestPlan parameters using TestPlanConfiguration

25beaaf

Rename flags to remove redundant test prefix

ec5bf63

Improve error messages

b2cb30b

Update filter logic

9735e36

Add missing unit tests

e95a143

Add Foundation import

47ebbc2

Refactor TestPlan manifest init into its own file

094b171

Add validation unit tests

87f7199

Add tests for TestIdentifier

2cee88d

stephanecopin force-pushed the stephane/test-plan-support branch from 5a19642 to 2cee88d Compare August 4, 2023 20:50

stephanecopin added 2 commits August 4, 2023 17:01

Replace TestPlan.TestTarget with TestableTarget

a57d15e

Fix lint issue

4e5dae6

Fix tests

479b41e

stephanecopin force-pushed the stephane/test-plan-support branch from 37194a8 to 479b41e Compare August 7, 2023 14:37

stephanecopin added 2 commits August 7, 2023 10:38

Add include external targets test

e213817

Fix lint issues

911d4a8

pepicrft approved these changes Sep 11, 2023

View reviewed changes

pepicrft added the changelog:added PR will be listed in the Added section of CHANGELOG label Sep 11, 2023

pepicrft merged commit a696648 into tuist:main Sep 11, 2023
36 checks passed

	help: "The list of test identifiers you want to test. Takes precedence over --skip-testing",
	help: "The list of test identifiers you want to test. Expected format is TestTarget[/TestClass[/TestMethod]]. It is applied before --skip-testing",

	help: "The list of configurations you want to test. Takes precedence over --skip-test-configuration"
	help: "The list of configurations you want to test. It is applied before --skip-test-configuration"

	\| `--test-targets` \| n/a \| `Specify which tests should run in the test-identifier* format. Takes precedence over --skip-testing` \| \| No \|
	\| `--test-targets` \| n/a \| `Specify which tests should run in the test-identifier* format. It is applied before --skip-testing` \| \| No \|

	\| `--test-configurations` \| n/a \| `When a test plan is specified, specify which test configurations to use for the run. Takes precedence over --skip-test-configuration` \| \| No \|
	\| `--test-configurations` \| n/a \| `When a test plan is specified, specify which test configurations to use for the run. It is applied before --skip-test-configuration` \| \| No \|

Add test plan support within Tuist test #5135

Add test plan support within Tuist test #5135

Conversation

stephanecopin commented Apr 5, 2023 • edited Loading

Short description 📝

How to test the changes locally 🧐

Contributor checklist ✅

Reviewer checklist ✅

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanecopin Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepicrft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanecopin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortmarek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanecopin Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

stephanecopin commented Aug 4, 2023 • edited Loading

stephanecopin commented Apr 5, 2023 •

edited

Loading

stephanecopin Apr 30, 2023 •

edited

Loading

stephanecopin Jul 13, 2023 •

edited

Loading

stephanecopin commented Aug 4, 2023 •

edited

Loading