Fix improper parenting of tests w/ identical names in explorer (#1664)

* Fix improper parenting of tests w/ identical names in explorer

Two tests in swift-testing can share the same identifier, for instance
if you have two identically named tests with the same name but that take
arguments of different types, i.e:

```
@test(arguments: [1]) func a(_ x: Int) {}
@test(arguments: ["a"]) func a(_ x: String) {}
```

In this case sourcekit-lsp provides a disambiguated id with the
filename/line number at the end.

The code to synthesize parents in the test explorer when provided with
test items that had none didn't take in to account this form of
disambiguated id, which lead to tests being incorrectly synthesized and
parented to the wrong test.

Author: plemarquand

src/TestExplorer/TestDiscovery.ts

@@ -73,6 +73,12 @@ export async function updateTestsFromClasses(
     updateTests(testController, results);
 }
 
+function isFileDisambiguated(id: string): boolean {
+    // a regex to check if the id ends with a string like "filename.swift:line:column"
+    const regex = /^(.*\/)?([^/]+\.swift):(\d+):(\d+)$/;
+    return regex.test(id);
+}
+
 export function updateTestsForTarget(
     testController: vscode.TestController,
     testTarget: { id: string; label: string },
@@ -91,13 +97,23 @@ export function updateTestsForTarget(
             return testItem;
         }
 
+        const fileDisambiguated = isFileDisambiguated(testItem.id);
         const item = { ...testItem };
         // To determine if any root level test items are missing a parent we check how many
         // components there are in the ID. If there are more than one (the test target) then
         // we synthesize all the intermediary test items.
         const idComponents = testItem.id.split(/\.|\//);
-        idComponents.pop(); // Remove the last component to get the parent ID components
-        if (idComponents.length > 1) {
+
+        // Remove the last component to get the parent ID components
+        idComponents.pop();
+
+        // If this is a file disambiguated id (ends in <file>.swift:<line>:<column>),
+        // remove both the filename and line info.
+        if (fileDisambiguated) {
+            idComponents.pop();
+        }
+
+        if (idComponents.length > (fileDisambiguated ? 2 : 1)) {
             let newId = idComponents.slice(0, 2).join(".");
             const remainingIdComponents = idComponents.slice(2);
             if (remainingIdComponents.length) {

src/TestExplorer/TestParsers/SwiftTestingOutputParser.ts

@@ -347,6 +347,15 @@ export class SwiftTestingOutputParser {
         }));
     }
 
+    private testItemIndexFromTestID(testID: string, runState: ITestRunState): number {
+        const testName = this.testName(testID);
+        const id = runState.getTestItemIndex(testName, undefined);
+        if (id === -1) {
+            return runState.getTestItemIndex(testID, undefined);
+        }
+        return id;
+    }
+
     private parse(item: SwiftTestEvent, runState: ITestRunState) {
         if (
             item.kind === "test" &&
@@ -358,8 +367,7 @@ export class SwiftTestingOutputParser {
             // map an event.payload.testID back to a test case.
             this.buildTestCaseMapForParameterizedTest(item);
 
-            const testName = this.testName(item.payload.id);
-            const testIndex = runState.getTestItemIndex(testName, undefined);
+            const testIndex = this.testItemIndexFromTestID(item.payload.id, runState);
             // If a test has test cases it is paramterized and we need to notify
             // the caller that the TestClass should be added to the vscode.TestRun
             // before it starts.
@@ -385,8 +393,7 @@ export class SwiftTestingOutputParser {
                 this.testRunStarted();
                 return;
             } else if (item.payload.kind === "testStarted") {
-                const testName = this.testName(item.payload.testID);
-                const testIndex = runState.getTestItemIndex(testName, undefined);
+                const testIndex = this.testItemIndexFromTestID(item.payload.testID, runState);
                 runState.started(testIndex, item.payload.instant.absolute);
                 return;
             } else if (item.payload.kind === "testCaseStarted") {
@@ -398,8 +405,7 @@ export class SwiftTestingOutputParser {
                 runState.started(testIndex, item.payload.instant.absolute);
                 return;
             } else if (item.payload.kind === "testSkipped") {
-                const testName = this.testName(item.payload.testID);
-                const testIndex = runState.getTestItemIndex(testName, undefined);
+                const testIndex = this.testItemIndexFromTestID(item.payload.testID, runState);
                 runState.skipped(testIndex);
                 return;
             } else if (item.payload.kind === "issueRecorded") {
@@ -444,8 +450,7 @@ export class SwiftTestingOutputParser {
                 }
                 return;
             } else if (item.payload.kind === "testEnded") {
-                const testName = this.testName(item.payload.testID);
-                const testIndex = runState.getTestItemIndex(testName, undefined);
+                const testIndex = this.testItemIndexFromTestID(item.payload.testID, runState);
 
                 // When running a single test the testEnded and testCaseEnded events
                 // have the same ID, and so we'd end the same test twice.

src/TestExplorer/TestRunArguments.ts

@@ -14,6 +14,7 @@
 
 import * as vscode from "vscode";
 import { reduceTestItemChildren } from "./TestUtils";
+import { TestRunProxy } from "./TestRunner";
 
 type ProcessResult = {
     testItems: vscode.TestItem[];
@@ -93,7 +94,7 @@ export class TestRunArguments {
                 const terminator = hasChildren ? "/" : "$";
                 // Debugging XCTests requires exact matches, so we don't need a trailing terminator.
                 return isDebug ? arg.id : `${arg.id}${terminator}`;
-            } else if (hasChildren) {
+            } else if (hasChildren && !this.hasParameterizedTestChildren(arg)) {
                 // Append a trailing slash to match a suite name exactly.
                 // This prevents TestTarget.MySuite matching TestTarget.MySuite2.
                 return `${arg.id}/`;
@@ -102,6 +103,12 @@ export class TestRunArguments {
         });
     }
 
+    private hasParameterizedTestChildren(testItem: vscode.TestItem): boolean {
+        return Array.from(testItem.children).some(arr =>
+            arr[1].tags.some(tag => tag.id === TestRunProxy.Tags.PARAMETERIZED_TEST_RESULT)
+        );
+    }
+
     private createTestItemReducer(
         include: readonly vscode.TestItem[],
         exclude: readonly vscode.TestItem[],

src/TestExplorer/TestRunner.ts

@@ -164,7 +164,7 @@ export class TestRunProxy {
                 // https://github.com/swiftlang/swift-testing/issues/671 is resolved.
                 testClass.tags = compactMap(parent.tags, t =>
                     t.id === runnableTag.id ? null : new vscode.TestTag(t.id)
-                );
+                ).concat(new vscode.TestTag(TestRunProxy.Tags.PARAMETERIZED_TEST_RESULT));
 
                 const added = upsertTestItem(this.controller, testClass, parent);
 
@@ -406,9 +406,10 @@ export class TestRunProxy {
         await this.coverage.computeCoverage(this.testRun);
     }
 
-    private static Tags = {
+    static Tags = {
         SKIPPED: "skipped",
         HAS_ATTACHMENT: "hasAttachment",
+        PARAMETERIZED_TEST_RESULT: "parameterizedTestResult",
     };
 
     // Remove any tags that were added due to test results

test/integration-tests/testexplorer/TestDiscovery.test.ts

@@ -160,6 +160,35 @@ suite("TestDiscovery Suite", () => {
         assert.deepStrictEqual(testController.items.get("foo")?.label, "New Label");
     });
 
+    test("handles adding tests that are disambiguated by a file/location", () => {
+        const child1 = testItem("AppTarget.example(_:)/AppTarget.swift:4:2", "swift-testing");
+        const child2 = testItem("AppTarget.example(_:)/AppTarget.swift:16:2", "swift-testing");
+
+        updateTestsForTarget(testController, { id: "AppTarget", label: "AppTarget" }, [
+            child1,
+            child2,
+        ]);
+
+        assert.deepStrictEqual(testControllerChildren(testController.items), [
+            {
+                id: "AppTarget",
+                tags: [{ id: "test-target" }, { id: "runnable" }],
+                children: [
+                    {
+                        id: "AppTarget.example(_:)/AppTarget.swift:4:2",
+                        tags: [{ id: "swift-testing" }, { id: "runnable" }],
+                        children: [],
+                    },
+                    {
+                        id: "AppTarget.example(_:)/AppTarget.swift:16:2",
+                        tags: [{ id: "swift-testing" }, { id: "runnable" }],
+                        children: [],
+                    },
+                ],
+            },
+        ]);
+    });
+
     test("handles adding a test to an existing parent when updating with a partial tree", () => {
         const child = testItem("AppTarget.AppTests/ChildTests/SubChildTests", "swift-testing");