fix: JUnit5 DynamicContainer are not wokring (#1619)

- Store the test data for the test invocations as well. - Remove the logic that adding prefix to the invocations. Instead, - Using the information returned from the runner to compose the ids. Signed-off-by: Sheng Chen <[email protected]>
microsoft · Oct 9, 2023 · e6b8af6 · e6b8af6
1 parent 69aaa5c
commit e6b8af6
Show file tree

Hide file tree

Showing 7 changed files with 119 additions and 74 deletions.
diff --git a/src/constants.ts b/src/constants.ts
@@ -66,8 +66,18 @@ export namespace Context {
 }
 
 /**
- * This is the prefix of the invocation test item's id.
- * Invocation test items are created during test run.
- * For example, the invocations from a parameterized test.
+ * The different part keys returned by the JUnit test runner,
+ * which are used to identify the test cases.
  */
-export const INVOCATION_PREFIX: string = '[__INVOCATION__]-';
+export namespace JUnitTestPart {
+    export const CLASS: string = 'class:';
+    export const NESTED_CLASS: string = 'nested-class:';
+    export const METHOD: string = 'method:';
+    export const TEST_FACTORY: string = 'test-factory:';
+    // Property id is for jqwik
+    export const PROPERTY: string = 'property:';
+    export const TEST_TEMPLATE: string = 'test-template:';
+    export const TEST_TEMPLATE_INVOCATION: string = 'test-template-invocation:';
+    export const DYNAMIC_CONTAINER: string = 'dynamic-container:';
+    export const DYNAMIC_TEST: string = 'dynamic-test:';
+}
diff --git a/src/controller/testController.ts b/src/controller/testController.ts
@@ -6,7 +6,6 @@ import * as path from 'path';
 import { CancellationToken, DebugConfiguration, Disposable, FileSystemWatcher, RelativePattern, TestController, TestItem, TestRun, TestRunProfileKind, TestRunRequest, tests, TestTag, Uri, window, workspace, WorkspaceFolder } from 'vscode';
 import { instrumentOperation, sendError, sendInfo } from 'vscode-extension-telemetry-wrapper';
 import { refreshExplorer } from '../commands/testExplorerCommands';
-import { INVOCATION_PREFIX } from '../constants';
 import { IProgressReporter } from '../debugger.api';
 import { progressProvider } from '../extension';
 import { testSourceProvider } from '../provider/testSourceProvider';
@@ -420,7 +419,7 @@ function removeNonRerunTestInvocations(testItems: TestItem[]): void {
         if (rerunMethods.includes(item)) {
             continue;
         }
-        if (item.id.startsWith(INVOCATION_PREFIX)) {
+        if (dataCache.get(item)?.testLevel === TestLevel.Invocation) {
             item.parent?.children.delete(item.id);
             continue;
         }

diff --git a/src/controller/utils.ts b/src/controller/utils.ts
@@ -7,7 +7,7 @@ import * as fse from 'fs-extra';
 import { performance } from 'perf_hooks';
 import { CancellationToken, commands, Range, TestItem, Uri, workspace, WorkspaceFolder } from 'vscode';
 import { sendError } from 'vscode-extension-telemetry-wrapper';
-import { INVOCATION_PREFIX, JavaTestRunnerDelegateCommands } from '../constants';
+import { JavaTestRunnerDelegateCommands } from '../constants';
 import { IJavaTestItem, ProjectType, TestKind, TestLevel } from '../types';
 import { executeJavaLanguageServerCommand } from '../utils/commandUtils';
 import { getRequestDelay, lruCache, MovingAverage } from './debouncing';
@@ -85,7 +85,7 @@ export function synchronizeItemsRecursively(parent: TestItem, childrenData: IJav
     if (childrenData) {
         // remove the out-of-date children
         parent.children.forEach((child: TestItem) => {
-            if (child.id.startsWith(INVOCATION_PREFIX)) {
+            if (dataCache.get(child)?.testLevel === TestLevel.Invocation) {
                 // only remove the invocation items before a new test session starts
                 return;
             }
@@ -118,15 +118,13 @@ export function updateOrCreateTestItem(parent: TestItem, childData: IJavaTestIte
 function updateTestItem(testItem: TestItem, metaInfo: IJavaTestItem): void {
     testItem.range = asRange(metaInfo.range);
     testItem.label = `${getCodiconLabel(metaInfo.testLevel)} ${metaInfo.label}`;
-    if (metaInfo.testLevel !== TestLevel.Invocation) {
-        dataCache.set(testItem, {
-            jdtHandler: metaInfo.jdtHandler,
-            fullName: metaInfo.fullName,
-            projectName: metaInfo.projectName,
-            testLevel: metaInfo.testLevel,
-            testKind: metaInfo.testKind,
-        });
-    }
+    dataCache.set(testItem, {
+        jdtHandler: metaInfo.jdtHandler,
+        fullName: metaInfo.fullName,
+        projectName: metaInfo.projectName,
+        testLevel: metaInfo.testLevel,
+        testKind: metaInfo.testKind,
+    });
 }
 
 /**
@@ -145,19 +143,15 @@ export function createTestItem(metaInfo: IJavaTestItem, parent?: TestItem): Test
         metaInfo.uri ? Uri.parse(metaInfo.uri) : undefined,
     );
     item.range = asRange(metaInfo.range);
-    if (metaInfo.testLevel !== TestLevel.Invocation
-        // invocations of JUnit5 parameterized tests can be run again using their uniqueId:
-        || (metaInfo.testKind === TestKind.JUnit5 && metaInfo.uniqueId)) {
-        item.tags = [runnableTag];
-        dataCache.set(item, {
-            jdtHandler: metaInfo.jdtHandler,
-            fullName: metaInfo.fullName,
-            projectName: metaInfo.projectName,
-            testLevel: metaInfo.testLevel,
-            testKind: metaInfo.testKind,
-            uniqueId: metaInfo.uniqueId
-        });
-    }
+    item.tags = [runnableTag];
+    dataCache.set(item, {
+        jdtHandler: metaInfo.jdtHandler,
+        fullName: metaInfo.fullName,
+        projectName: metaInfo.projectName,
+        testLevel: metaInfo.testLevel,
+        testKind: metaInfo.testKind,
+        uniqueId: metaInfo.uniqueId
+    });
 
     if (metaInfo.testLevel === TestLevel.Invocation) {
         item.sortText = metaInfo.sortText ?? metaInfo.id;

diff --git a/src/runners/junitRunner/JUnitRunnerResultAnalyzer.ts b/src/runners/junitRunner/JUnitRunnerResultAnalyzer.ts
@@ -2,12 +2,12 @@
 // Licensed under the MIT license.
 
 import { Location, MarkdownString, TestItem, TestMessage } from 'vscode';
-import { INVOCATION_PREFIX } from '../../constants';
 import { dataCache, ITestItemData } from '../../controller/testItemDataCache';
 import { createTestItem, updateOrCreateTestItem } from '../../controller/utils';
 import { IJavaTestItem, IRunTestContext, TestKind, TestLevel } from '../../types';
 import { RunnerResultAnalyzer } from '../baseRunner/RunnerResultAnalyzer';
 import { findTestLocation, setTestState, TestResultState } from '../utils';
+import { JUnitTestPart } from '../../constants';
 
 
 export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
@@ -192,20 +192,13 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
     }
 
     protected getTestIdForJunit5Method(message: string): string {
-        const classId: string = 'class:';
-        const nestedClassId: string = 'nested-class:';
-        const methodId: string = 'method:';
-        const testFactoryId: string = 'test-factory:';
-        // Property id is for jqwik
-        const propertyId: string = 'property:';
-        const testTemplateId: string = 'test-template:';
-
         // We're expecting something like:
         // [engine:junit5]/[class:com.example.MyTest]/[method:myTest]/[test-template:myTest(String\, int)]
         const parts: string[] = message.split('/');
 
         let className: string = '';
         let methodName: string = '';
+        let InvocationSuffix: string = '';
 
         if (!parts || parts.length === 0) {
             // The pattern doesn't match what we're expecting, so we'll go ahead and defer to the non JUnit5 method.
@@ -216,38 +209,44 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
             // Remove the leading and trailing brackets.
             part = part.trim().replace(/\[/g, '').replace(/\]/g, '');
 
-            if (part.startsWith(classId)) {
-                className = part.substring(classId.length);
-            } else if (part.startsWith(methodId)) {
-                const rawMethodName: string = part.substring(methodId.length);
+            if (part.startsWith(JUnitTestPart.CLASS)) {
+                className = part.substring(JUnitTestPart.CLASS.length);
+            } else if (part.startsWith(JUnitTestPart.METHOD)) {
+                const rawMethodName: string = part.substring(JUnitTestPart.METHOD.length);
                 // If the method name exists then we want to include the '#' qualifier.
                 methodName = `#${this.getJUnit5MethodName(rawMethodName)}`;
-            } else if (part.startsWith(testFactoryId)) {
-                const rawMethodName: string = part.substring(testFactoryId.length);
+            } else if (part.startsWith(JUnitTestPart.TEST_FACTORY)) {
+                const rawMethodName: string = part.substring(JUnitTestPart.TEST_FACTORY.length);
                 // If the method name exists then we want to include the '#' qualifier.
                 methodName = `#${this.getJUnit5MethodName(rawMethodName)}`;
-            } else if (part.startsWith(nestedClassId)) {
-                const nestedClassName: string = part.substring(nestedClassId.length);
+            } else if (part.startsWith(JUnitTestPart.NESTED_CLASS)) {
+                const nestedClassName: string = part.substring(JUnitTestPart.NESTED_CLASS.length);
                 className = `${className}$${nestedClassName}`;
-            } else if (part.startsWith(testTemplateId)) {
-                const rawMethodName: string = part.substring(testTemplateId.length)
+            } else if (part.startsWith(JUnitTestPart.TEST_TEMPLATE)) {
+                const rawMethodName: string = part.substring(JUnitTestPart.TEST_TEMPLATE.length)
                     .replace(/\\,/g, ',')
                     .replace(/ /g, '');
                 // If the method name exists then we want to include the '#' qualifier.
                 methodName = `#${this.getJUnit5MethodName(rawMethodName)}`;
-            } else if (part.startsWith(propertyId)) {
-                const rawMethodName: string = part.substring(propertyId.length)
+            } else if (part.startsWith(JUnitTestPart.PROPERTY)) {
+                const rawMethodName: string = part.substring(JUnitTestPart.PROPERTY.length)
                     .replace(/\\,/g, ',')
                     .replace(/ /g, '');
                 // If the method name exists then we want to include the '#' qualifier.
                 methodName = `#${this.getJUnit5MethodName(rawMethodName)}`;
+            } else if (part.startsWith(JUnitTestPart.TEST_TEMPLATE_INVOCATION)) {
+                InvocationSuffix += `[${part.substring(JUnitTestPart.TEST_TEMPLATE_INVOCATION.length)}]`;
+            } else if (part.startsWith(JUnitTestPart.DYNAMIC_CONTAINER)) {
+                InvocationSuffix += `[${part.substring(JUnitTestPart.DYNAMIC_CONTAINER.length)}]`;
+            } else if (part.startsWith(JUnitTestPart.DYNAMIC_TEST)) {
+                InvocationSuffix += `[${part.substring(JUnitTestPart.DYNAMIC_TEST.length)}]`;
             }
         });
 
         if (className) {
-            return `${this.projectName}@${className}${methodName}`;
+            return `${this.projectName}@${className}${methodName}${InvocationSuffix}`;
         } else {
-            return `${this.projectName}@${message}`;
+            return `${this.projectName}@${message}${InvocationSuffix}`;
         }
     }
 
@@ -354,8 +353,8 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
                 const parent: TestItem | undefined = parentInfo?.testItem;
                 if (parent) {
                     const parentData: ITestItemData | undefined = dataCache.get(parent);
-                    if (parentData?.testLevel === TestLevel.Method) {
-                        testItem = this.enlistDynamicMethodToTestMapping(testItem, parent, parentData, displayName, uniqueId);
+                    if (parentData?.testLevel === TestLevel.Method || parentData?.testLevel === TestLevel.Invocation) {
+                        testItem = this.enlistDynamicMethodToTestMapping(testId, parent, parentData, displayName, uniqueId);
                     }
                 }
             } else {
@@ -376,7 +375,7 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
                             jdtHandler: '',
                             fullName: testId.substr(testId.indexOf('@') + 1),
                             label: this.getTestMethodName(displayName),
-                            id: `${INVOCATION_PREFIX}${testId}`,
+                            id: testId,
                             projectName: this.projectName,
                             testKind: this.testContext.kind,
                             testLevel: TestLevel.Invocation,
@@ -412,25 +411,23 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
     }
 
     // Leaving this public so that it can be mocked when testing.
-    public enlistDynamicMethodToTestMapping(testItem: TestItem | undefined, parent: TestItem, parentData: ITestItemData, displayName: string, uniqueId: string | undefined): TestItem {
-        testItem = updateOrCreateTestItem(parent, {
+    public enlistDynamicMethodToTestMapping(testId: string, parent: TestItem, parentData: ITestItemData, displayName: string, uniqueId: string | undefined): TestItem {
+        return updateOrCreateTestItem(parent, {
             children: [],
             uri: parent.uri?.toString(),
             range: parent.range,
             jdtHandler: parentData.jdtHandler,
             fullName: parentData.fullName,
             label: this.getTestMethodName(displayName),
             // prefer uniqueId, as it does not change when re-running only a single invocation:
-            id: uniqueId ? `${INVOCATION_PREFIX}${uniqueId}`
-                : `${INVOCATION_PREFIX}${parent.id}[#${parent.children.size + 1}]`,
+            id: uniqueId ? uniqueId : testId,
             projectName: parentData.projectName,
             testKind: parentData.testKind,
             testLevel: TestLevel.Invocation,
             uniqueId,
             // We will just create a string padded with the character "a" to provide easy sorting.
             sortText: ''.padStart(parent.children.size + 1, 'a')
         });
-        return testItem;
     }
 
     private async tryAppendMessage(item: TestItem, testMessage: TestMessage, testState: TestResultState): Promise<void> {
@@ -440,15 +437,12 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
             testMessage.location = new Location(item.uri, item.range);
         } else {
             let id: string = item.id;
-            if (id.startsWith(INVOCATION_PREFIX)) {
-                id = id.substring(INVOCATION_PREFIX.length);
-                if (this.testContext.kind === TestKind.JUnit) {
-                    // change test[0] -> test
-                    // to fix: https://github.com/microsoft/vscode-java-test/issues/1296
-                    const indexOfParameterizedTest: number = id.lastIndexOf('[');
-                    if (indexOfParameterizedTest > -1) {
-                        id = id.substring(0, id.lastIndexOf('['));
-                    }
+            if (this.testContext.kind === TestKind.JUnit) {
+                // change test[0] -> test
+                // to fix: https://github.com/microsoft/vscode-java-test/issues/1296
+                const indexOfParameterizedTest: number = id.lastIndexOf('[');
+                if (indexOfParameterizedTest > -1) {
+                    id = id.substring(0, id.lastIndexOf('['));
                 }
             }
             const location: Location | undefined = await findTestLocation(id);

diff --git a/test/suite/JUnitAnalyzer.test.ts b/test/suite/JUnitAnalyzer.test.ts
@@ -430,7 +430,7 @@ org.opentest4j.AssertionFailedError: expected: <1> but was: <2>
         // We need to stub this method to avoid issues with the TestController
         // not being set up in the non-test version of the utils file.
         const stub = sinon.stub(analyzer, "enlistDynamicMethodToTestMapping");
-        const dummy = generateTestItem(testController, '[__INVOCATION__]-dummy', TestKind.JUnit5, new Range(10, 0, 16, 0));
+        const dummy = generateTestItem(testController, 'dummy', TestKind.JUnit5, new Range(10, 0, 16, 0));
         stub.returns(dummy);
         analyzer.analyzeData(testRunnerOutput);
 
@@ -498,7 +498,45 @@ org.opentest4j.AssertionFailedError: expected: <1> but was: <2>
         // We need to stub this method to avoid issues with the TestController
         // not being set up in the non-test version of the utils file.
         const stub = sinon.stub(analyzer, "enlistDynamicMethodToTestMapping");
-        const dummy = generateTestItem(testController, '[__INVOCATION__]-dummy', TestKind.JUnit5, new Range(10, 0, 16, 0));
+        const dummy = generateTestItem(testController, 'dummy', TestKind.JUnit5, new Range(10, 0, 16, 0));
+        stub.returns(dummy);
+        analyzer.analyzeData(testRunnerOutput);
+
+        sinon.assert.calledWith(startedSpy, dummy);
+        sinon.assert.calledWith(passedSpy, dummy);
+    });
+
+    test("can handle DynamicContainer", () => {
+        const testItem = generateTestItem(testController, '[email protected]#testContainers()', TestKind.JUnit5, new Range(10, 0, 16, 0));
+        const testRunRequest = new TestRunRequest([testItem], []);
+        const testRun = testController.createTestRun(testRunRequest);
+        const startedSpy = sinon.spy(testRun, 'started');
+        const passedSpy = sinon.spy(testRun, 'passed');
+        const testRunnerOutput = `%TESTC  0 v2
+%TSTTREE2,junit5.TestFactoryTest,true,1,false,1,TestFactoryTest,,[engine:junit-jupiter]/[class:junit5.TestFactoryTest]
+%TSTTREE3,testContainers(junit5.TestFactoryTest),true,0,false,2,testContainers(),,[engine:junit-jupiter]/[class:junit5.TestFactoryTest]/[test-factory:testContainers()]
+%TSTTREE4,testContainers(junit5.TestFactoryTest),true,0,true,3,Container,,[engine:junit-jupiter]/[class:junit5.TestFactoryTest]/[test-factory:testContainers()]/[dynamic-container:#1]
+%TSTTREE5,testContainers(junit5.TestFactoryTest),false,1,true,4,Test,,[engine:junit-jupiter]/[class:junit5.TestFactoryTest]/[test-factory:testContainers()]/[dynamic-container:#1]/[dynamic-test:#1]
+%TESTS  5,testContainers(junit5.TestFactoryTest)
+
+%TESTE  5,testContainers(junit5.TestFactoryTest)
+
+%RUNTIME103`;
+        const runnerContext: IRunTestContext = {
+            isDebug: false,
+            kind: TestKind.JUnit5,
+            projectName: 'junit',
+            testItems: [testItem],
+            testRun: testRun,
+            workspaceFolder: workspace.workspaceFolders?.[0]!,
+        };
+
+        const analyzer = new JUnitRunnerResultAnalyzer(runnerContext);
+
+        // We need to stub this method to avoid issues with the TestController
+        // not being set up in the non-test version of the utils file.
+        const stub = sinon.stub(analyzer, "enlistDynamicMethodToTestMapping");
+        const dummy = generateTestItem(testController, 'dummy', TestKind.JUnit5, new Range(10, 0, 16, 0));
         stub.returns(dummy);
         analyzer.analyzeData(testRunnerOutput);
 

diff --git a/test/suite/testController.handleInvocations.test.ts b/test/suite/testController.handleInvocations.test.ts
@@ -1,14 +1,12 @@
 import * as assert from 'assert';
 import * as sinon from 'sinon';
 import { TestController, TestItem, tests, window } from "vscode";
-import { INVOCATION_PREFIX } from '../../src/constants';
 import { handleInvocations } from '../../src/controller/testController';
 import { dataCache } from "../../src/controller/testItemDataCache";
 import { TestKind, TestLevel } from "../../src/types";
 import { setupTestEnv } from './utils';
 
 function generateTestItem(testController: TestController, id: string, testKind: TestKind, testLevel: TestLevel, uniqueId?: string): TestItem {
-    id = testLevel === TestLevel.Invocation ? INVOCATION_PREFIX + id : id;
     const testItem = testController.createTestItem(id, id + '_label');
     dataCache.set(testItem, {
         jdtHandler: id + '_jdtHandler',

diff --git a/test/test-projects/junit/src/test/java/junit5/TestFactoryTest.java b/test/test-projects/junit/src/test/java/junit5/TestFactoryTest.java
@@ -1,10 +1,15 @@
 package junit5;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.DynamicContainer.dynamicContainer;
+import static org.junit.jupiter.api.DynamicTest.dynamicTest;
 
+import java.util.List;
 import java.util.function.Function;
 import java.util.stream.Stream;
 
+import org.junit.jupiter.api.DynamicContainer;
 import org.junit.jupiter.api.DynamicTest;
 import org.junit.jupiter.api.TestFactory;
 import org.junit.jupiter.api.function.ThrowingConsumer;
@@ -17,4 +22,11 @@ Stream<DynamicTest> testDynamicTest() {
         final ThrowingConsumer<Long> test = input -> {assertEquals(1L, input);};
         return DynamicTest.stream(parameters, testNames, test);
     }
+
+    @TestFactory
+    public Stream<DynamicContainer> testContainers() {
+        return Stream.of(dynamicContainer("Container", List.of(
+            dynamicTest("Test", () -> {assertTrue(true);})
+        )));
+    }
 }