From e6b8af6d96638ecf5c78c847070e4a4bfd4b60ee Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Sun, 8 Oct 2023 21:55:04 -0700 Subject: [PATCH] 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 --- src/constants.ts | 18 ++++- src/controller/testController.ts | 3 +- src/controller/utils.ts | 42 +++++------ .../junitRunner/JUnitRunnerResultAnalyzer.ts | 74 +++++++++---------- test/suite/JUnitAnalyzer.test.ts | 42 ++++++++++- .../testController.handleInvocations.test.ts | 2 - .../src/test/java/junit5/TestFactoryTest.java | 12 +++ 7 files changed, 119 insertions(+), 74 deletions(-) diff --git a/src/constants.ts b/src/constants.ts index 6d5210fd..ea2924f8 100644 --- 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 index 044017bb..492f64cf 100644 --- 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 index 8ad9c9ee..df33ef41 100644 --- 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 index 1c513c97..f0d71c1e 100644 --- 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,8 +411,8 @@ 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, @@ -421,8 +420,7 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer { 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, @@ -430,7 +428,6 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer { // 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 { @@ -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 index 6b798800..3b8fdba5 100644 --- 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, 'junit@junit5.TestFactoryTest#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 index 08b6b175..d979e680 100644 --- 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 index cc6aab7a..19fde2f8 100644 --- 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 testDynamicTest() { final ThrowingConsumer test = input -> {assertEquals(1L, input);}; return DynamicTest.stream(parameters, testNames, test); } + + @TestFactory + public Stream testContainers() { + return Stream.of(dynamicContainer("Container", List.of( + dynamicTest("Test", () -> {assertTrue(true);}) + ))); + } }