From 7c2ef8b9e8d7e6778a7bd5e4acf41296fd340d72 Mon Sep 17 00:00:00 2001
From: Pavel Yaskevich <xedin@apache.org>
Date: Tue, 13 Aug 2024 16:55:09 -0700
Subject: [PATCH] [Build] BuildPlan: Always discover test modules through their
 aggregate products

Fixes a situation when tests were build for both host and target
even though they should have been built only for the host (i.e. when
one of the modules depends on a macro).
---
 Sources/Build/BuildPlan/BuildPlan.swift             |  8 ++++----
 .../_InternalTestSupport/MockPackageGraphs.swift    |  6 ++++++
 .../BuildTests/CrossCompilationBuildPlanTests.swift |  8 +++++++-
 .../CrossCompilationPackageGraphTests.swift         | 13 ++++++++++++-
 4 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/Sources/Build/BuildPlan/BuildPlan.swift b/Sources/Build/BuildPlan/BuildPlan.swift
index c5648a2419e..2a9ae61390f 100644
--- a/Sources/Build/BuildPlan/BuildPlan.swift
+++ b/Sources/Build/BuildPlan/BuildPlan.swift
@@ -986,9 +986,9 @@ extension BuildPlan {
             }
 
             for module in package.modules {
-                if case .test = module.underlying.type,
-                   !graph.rootPackages.contains(id: package.id)
-                {
+                // Tests are discovered through an aggregate product which also
+                // informs their destination.
+                if case .test = module.underlying.type {
                     continue
                 }
 
@@ -1002,7 +1002,7 @@ extension BuildPlan {
             for product: ResolvedProduct,
             destination: Destination
         ) -> [TraversalNode] {
-            guard destination == .host else {
+            guard destination == .host || product.underlying.type == .test else {
                 return []
             }
 
diff --git a/Sources/_InternalTestSupport/MockPackageGraphs.swift b/Sources/_InternalTestSupport/MockPackageGraphs.swift
index ba576b84b4c..6d5a92a2f1e 100644
--- a/Sources/_InternalTestSupport/MockPackageGraphs.swift
+++ b/Sources/_InternalTestSupport/MockPackageGraphs.swift
@@ -137,6 +137,7 @@ package func macrosTestsPackageGraph() throws -> MockPackageGraph {
         "/swift-mmio/Sources/MMIOMacros/source.swift",
         "/swift-mmio/Sources/MMIOMacrosTests/source.swift",
         "/swift-mmio/Sources/MMIOMacro+PluginTests/source.swift",
+        "/swift-mmio/Sources/NOOPTests/source.swift",
         "/swift-syntax/Sources/SwiftSyntax/source.swift",
         "/swift-syntax/Sources/SwiftSyntaxMacrosTestSupport/source.swift",
         "/swift-syntax/Sources/SwiftSyntaxMacros/source.swift",
@@ -203,6 +204,11 @@ package func macrosTestsPackageGraph() throws -> MockPackageGraph {
                             .target(name: "MMIOMacros")
                         ],
                         type: .test
+                    ),
+                    TargetDescription(
+                        name: "NOOPTests",
+                        dependencies: [],
+                        type: .test
                     )
                 ]
             ),
diff --git a/Tests/BuildTests/CrossCompilationBuildPlanTests.swift b/Tests/BuildTests/CrossCompilationBuildPlanTests.swift
index d053b876987..6cdc02c4fec 100644
--- a/Tests/BuildTests/CrossCompilationBuildPlanTests.swift
+++ b/Tests/BuildTests/CrossCompilationBuildPlanTests.swift
@@ -295,9 +295,15 @@ final class CrossCompilationBuildPlanTests: XCTestCase {
             fileSystem: fs,
             observabilityScope: scope
         )
+
+        // Make sure that build plan doesn't have any "target" tests.
+        for (_, description) in plan.targetMap where description.module.underlying.type == .test {
+            XCTAssertEqual(description.buildParameters.destination, .host)
+        }
+
         let result = try BuildPlanResult(plan: plan)
         result.checkProductsCount(2)
-        result.checkTargetsCount(16)
+        result.checkTargetsCount(17)
 
         XCTAssertTrue(try result.allTargets(named: "SwiftSyntax")
             .map { try $0.swift() }
diff --git a/Tests/PackageGraphTests/CrossCompilationPackageGraphTests.swift b/Tests/PackageGraphTests/CrossCompilationPackageGraphTests.swift
index c749ea28aba..790c62e114a 100644
--- a/Tests/PackageGraphTests/CrossCompilationPackageGraphTests.swift
+++ b/Tests/PackageGraphTests/CrossCompilationPackageGraphTests.swift
@@ -116,7 +116,11 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
                 "SwiftSyntaxMacrosTestSupport",
                 "SwiftSyntaxMacrosTestSupport"
             )
-            result.check(testModules: "MMIOMacrosTests", "MMIOMacro+PluginTests")
+            // TODO: NOOPTests are mentioned twice because in the graph they appear
+            // as if they target both "tools" and "destination", see the test below.
+            // Once the `buildTriple` is gone, there is going to be only one mention
+            // left.
+            result.check(testModules: "MMIOMacrosTests", "MMIOMacro+PluginTests", "NOOPTests", "NOOPTests")
             result.checkTarget("MMIO") { result in
                 result.check(buildTriple: .destination)
                 result.check(dependencies: "MMIOMacros")
@@ -180,6 +184,13 @@ final class CrossCompilationPackageGraphTests: XCTestCase {
                     XCTAssertEqual(graph.package(for: result.target)?.identity, .plain("swift-syntax"))
                 }
             }
+
+            result.checkTargets("NOOPTests") { results in
+                XCTAssertEqual(results.count, 2)
+
+                XCTAssertEqual(results.filter({ $0.target.buildTriple == .tools }).count, 1)
+                XCTAssertEqual(results.filter({ $0.target.buildTriple == .destination }).count, 1)
+            }
         }
     }