[PackageBuilder] Fix test target ref in manifest for external packages

Sources/PackageGraph/PackageGraphLoader.swift

@@ -49,7 +49,7 @@ public struct PackageGraphLoader {
     public init() { }
 
     /// Load the package graph for the given package path.
-    public func load(rootManifest: Manifest, externalManifests: [Manifest]) throws -> PackageGraph {
+    public func load(rootManifest: Manifest, externalManifests: [Manifest], fileSystem: FileSystem = localFileSystem) throws -> PackageGraph {
         let allManifests = externalManifests + [rootManifest]
 
         // Create the packages and convert to modules.
@@ -68,7 +68,7 @@ public struct PackageGraphLoader {
             // FIXME: We should always load the tests, but just change which
             // tests we build based on higher-level logic. This would make it
             // easier to allow testing of external package tests.
-            let builder = PackageBuilder(manifest: manifest, path: packagePath)
+            let builder = PackageBuilder(manifest: manifest, path: packagePath, fileSystem: fileSystem)
             let package = try builder.construct(includingTestModules: isRootPackage)
             packages.append(package)
 

Sources/PackageLoading/PackageBuilder.swift

@@ -208,15 +208,11 @@ public struct PackageBuilder {
     ///   - includingTestModules: Whether the package's test modules should be loaded.
     public func construct(includingTestModules: Bool) throws -> Package {
         let modules = try constructModules()
-        let testModules: [Module]
-        if includingTestModules {
-            testModules = try constructTestModules(modules: modules)
-        } else {
-            testModules = []
-        }
+        let testModules = try constructTestModules(modules: modules)
         try fillDependencies(modules: modules + testModules)
-        let products = try constructProducts(modules, testModules: testModules)
-        return Package(manifest: manifest, path: packagePath, modules: modules, testModules: testModules, products: products)
+        // FIXME: Lift includingTestModules into a higher module.
+        let products = try constructProducts(modules, testModules: includingTestModules ? testModules : [])
+        return Package(manifest: manifest, path: packagePath, modules: modules, testModules: includingTestModules ? testModules : [], products: products)
     }
 
     // MARK: Utility Predicates

Tests/PackageGraphTests/PackageGraphTests.swift

@@ -10,14 +10,104 @@
 
 import XCTest
 
+import Basic
 import PackageGraph
+import PackageDescription
+import PackageLoading
+import PackageModel
+
+private let v1: Version = "1.0.0"
 
 class PackageGraphTests: XCTestCase {
-    func testBasics() {
-        // Implement.
+
+    func testBasic() throws {
+        let fs = InMemoryFileSystem(emptyFiles:
+            "/Foo/source.swift",
+            "/Foo/Tests/FooTests/source.swift",
+            "/Bar/source.swift",
+            "/Bar/Tests/BarTests/source.swift"
+        )
+
+        let g = try loadMockPackageGraph([
+            "/Foo": Package(name: "Foo"),
+            "/Bar": Package(name: "Bar", dependencies: [.Package(url: "/Foo", majorVersion: 1)]),
+        ], root: "/Bar", in: fs)
+
+        PackageGraphTester(g) { result in
+            result.check(packages: "Bar", "Foo")
+            result.check(modules: "Bar", "Foo")
+            result.check(testModules: "BarTests")
+        }
+    }
+
+    // Make sure there is no error when we reference Test targets in a package and then
+    // use it as a dependency to another package. SR-2353
+    func testTestTargetDeclInExternalPackage() throws {
+        let fs = InMemoryFileSystem(emptyFiles:
+            "/Foo/source.swift",
+            "/Foo/Tests/SomeTests/source.swift",
+            "/Bar/source.swift",
+            "/Bar/Tests/BarTests/source.swift"
+        )
+
+        let g = try loadMockPackageGraph([
+            "/Foo": Package(name: "Foo", targets: [Target(name: "SomeTests", dependencies: ["Foo"])]),
+            "/Bar": Package(name: "Bar", dependencies: [.Package(url: "/Foo", majorVersion: 1)]),
+        ], root: "/Bar", in: fs)
+
+        PackageGraphTester(g) { result in
+            result.check(packages: "Bar", "Foo")
+            result.check(modules: "Bar", "Foo")
+            result.check(testModules: "BarTests")
+        }
     }
 
     static var allTests = [
-        ("testBasics", testBasics),
+        ("testBasic", testBasic),
+        ("testTestTargetDeclInExternalPackage", testTestTargetDeclInExternalPackage),
     ]
 }
+
+private func PackageGraphTester(_ graph: PackageGraph, _ result: (PackageGraphResult) -> Void) {
+    result(PackageGraphResult(graph))
+}
+
+private class PackageGraphResult {
+    let graph: PackageGraph
+
+    init(_ graph: PackageGraph) {
+        self.graph = graph
+    }
+
+    func check(packages: String..., file: StaticString = #file, line: UInt = #line) {
+        XCTAssertEqual(graph.packages.map {$0.name}, packages, file: file, line: line)
+    }
+
+    func check(modules: String..., file: StaticString = #file, line: UInt = #line) {
+        XCTAssertEqual(graph.packages.flatMap {$0.modules.map {$0.name} }, modules, file: file, line: line)
+    }
+
+    func check(testModules: String..., file: StaticString = #file, line: UInt = #line) {
+        XCTAssertEqual(graph.packages.flatMap {$0.testModules.map {$0.name} }, testModules, file: file, line: line)
+    }
+}
+
+private func loadMockPackageGraph(_ packageMap: [String: PackageDescription.Package], root: String, in fs: FileSystem) throws -> PackageGraph {
+    var externalManifests = [Manifest]()
+    var rootManifest: Manifest!
+    for (url, package) in packageMap {
+        let manifest = Manifest(
+            path: AbsolutePath(url).appending(component: Manifest.filename),
+            url: url,
+            package: package,
+            products: [],
+            version: v1
+        )
+        if url == root {
+            rootManifest = manifest
+        } else {
+            externalManifests.append(manifest)
+        }
+    }
+    return try PackageGraphLoader().load(rootManifest: rootManifest, externalManifests: externalManifests, fileSystem: fs)
+}