Skip to content

Commit e6b8af6

Browse files
jdneojdneo
authored
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 <sheche@microsoft.com>
1 parent 69aaa5c commit e6b8af6

File tree

7 files changed

+119
-74
lines changed

7 files changed

+119
-74
lines changed

‎src/constants.ts‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,18 @@ export namespace Context {
6666
}
6767

6868
/**
69-
* This is the prefix of the invocation test item's id.
70-
* Invocation test items are created during test run.
71-
* For example, the invocations from a parameterized test.
69+
* The different part keys returned by the JUnit test runner,
70+
* which are used to identify the test cases.
7271
*/
73-
export const INVOCATION_PREFIX: string = '[__INVOCATION__]-';
72+
export namespace JUnitTestPart {
73+
export const CLASS: string = 'class:';
74+
export const NESTED_CLASS: string = 'nested-class:';
75+
export const METHOD: string = 'method:';
76+
export const TEST_FACTORY: string = 'test-factory:';
77+
// Property id is for jqwik
78+
export const PROPERTY: string = 'property:';
79+
export const TEST_TEMPLATE: string = 'test-template:';
80+
export const TEST_TEMPLATE_INVOCATION: string = 'test-template-invocation:';
81+
export const DYNAMIC_CONTAINER: string = 'dynamic-container:';
82+
export const DYNAMIC_TEST: string = 'dynamic-test:';
83+
}

‎src/controller/testController.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import * as path from 'path';
66
import { CancellationToken, DebugConfiguration, Disposable, FileSystemWatcher, RelativePattern, TestController, TestItem, TestRun, TestRunProfileKind, TestRunRequest, tests, TestTag, Uri, window, workspace, WorkspaceFolder } from 'vscode';
77
import { instrumentOperation, sendError, sendInfo } from 'vscode-extension-telemetry-wrapper';
88
import { refreshExplorer } from '../commands/testExplorerCommands';
9-
import { INVOCATION_PREFIX } from '../constants';
109
import { IProgressReporter } from '../debugger.api';
1110
import { progressProvider } from '../extension';
1211
import { testSourceProvider } from '../provider/testSourceProvider';
@@ -420,7 +419,7 @@ function removeNonRerunTestInvocations(testItems: TestItem[]): void {
420419
if (rerunMethods.includes(item)) {
421420
continue;
422421
}
423-
if (item.id.startsWith(INVOCATION_PREFIX)) {
422+
if (dataCache.get(item)?.testLevel === TestLevel.Invocation) {
424423
item.parent?.children.delete(item.id);
425424
continue;
426425
}

‎src/controller/utils.ts‎

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import * as fse from 'fs-extra';
77
import { performance } from 'perf_hooks';
88
import { CancellationToken, commands, Range, TestItem, Uri, workspace, WorkspaceFolder } from 'vscode';
99
import { sendError } from 'vscode-extension-telemetry-wrapper';
10-
import { INVOCATION_PREFIX, JavaTestRunnerDelegateCommands } from '../constants';
10+
import { JavaTestRunnerDelegateCommands } from '../constants';
1111
import { IJavaTestItem, ProjectType, TestKind, TestLevel } from '../types';
1212
import { executeJavaLanguageServerCommand } from '../utils/commandUtils';
1313
import { getRequestDelay, lruCache, MovingAverage } from './debouncing';
@@ -85,7 +85,7 @@ export function synchronizeItemsRecursively(parent: TestItem, childrenData: IJav
8585
if (childrenData) {
8686
// remove the out-of-date children
8787
parent.children.forEach((child: TestItem) => {
88-
if (child.id.startsWith(INVOCATION_PREFIX)) {
88+
if (dataCache.get(child)?.testLevel === TestLevel.Invocation) {
8989
// only remove the invocation items before a new test session starts
9090
return;
9191
}
@@ -118,15 +118,13 @@ export function updateOrCreateTestItem(parent: TestItem, childData: IJavaTestIte
118118
function updateTestItem(testItem: TestItem, metaInfo: IJavaTestItem): void {
119119
testItem.range = asRange(metaInfo.range);
120120
testItem.label = `${getCodiconLabel(metaInfo.testLevel)} ${metaInfo.label}`;
121-
if (metaInfo.testLevel !== TestLevel.Invocation) {
122-
dataCache.set(testItem, {
123-
jdtHandler: metaInfo.jdtHandler,
124-
fullName: metaInfo.fullName,
125-
projectName: metaInfo.projectName,
126-
testLevel: metaInfo.testLevel,
127-
testKind: metaInfo.testKind,
128-
});
129-
}
121+
dataCache.set(testItem, {
122+
jdtHandler: metaInfo.jdtHandler,
123+
fullName: metaInfo.fullName,
124+
projectName: metaInfo.projectName,
125+
testLevel: metaInfo.testLevel,
126+
testKind: metaInfo.testKind,
127+
});
130128
}
131129

132130
/**
@@ -145,19 +143,15 @@ export function createTestItem(metaInfo: IJavaTestItem, parent?: TestItem): Test
145143
metaInfo.uri ? Uri.parse(metaInfo.uri) : undefined,
146144
);
147145
item.range = asRange(metaInfo.range);
148-
if (metaInfo.testLevel !== TestLevel.Invocation
149-
// invocations of JUnit5 parameterized tests can be run again using their uniqueId:
150-
|| (metaInfo.testKind === TestKind.JUnit5 && metaInfo.uniqueId)) {
151-
item.tags = [runnableTag];
152-
dataCache.set(item, {
153-
jdtHandler: metaInfo.jdtHandler,
154-
fullName: metaInfo.fullName,
155-
projectName: metaInfo.projectName,
156-
testLevel: metaInfo.testLevel,
157-
testKind: metaInfo.testKind,
158-
uniqueId: metaInfo.uniqueId
159-
});
160-
}
146+
item.tags = [runnableTag];
147+
dataCache.set(item, {
148+
jdtHandler: metaInfo.jdtHandler,
149+
fullName: metaInfo.fullName,
150+
projectName: metaInfo.projectName,
151+
testLevel: metaInfo.testLevel,
152+
testKind: metaInfo.testKind,
153+
uniqueId: metaInfo.uniqueId
154+
});
161155

162156
if (metaInfo.testLevel === TestLevel.Invocation) {
163157
item.sortText = metaInfo.sortText ?? metaInfo.id;

‎src/runners/junitRunner/JUnitRunnerResultAnalyzer.ts‎

Lines changed: 34 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,12 @@
22
// Licensed under the MIT license.
33

44
import { Location, MarkdownString, TestItem, TestMessage } from 'vscode';
5-
import { INVOCATION_PREFIX } from '../../constants';
65
import { dataCache, ITestItemData } from '../../controller/testItemDataCache';
76
import { createTestItem, updateOrCreateTestItem } from '../../controller/utils';
87
import { IJavaTestItem, IRunTestContext, TestKind, TestLevel } from '../../types';
98
import { RunnerResultAnalyzer } from '../baseRunner/RunnerResultAnalyzer';
109
import { findTestLocation, setTestState, TestResultState } from '../utils';
10+
import { JUnitTestPart } from '../../constants';
1111

1212

1313
export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
@@ -192,20 +192,13 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
192192
}
193193

194194
protected getTestIdForJunit5Method(message: string): string {
195-
const classId: string = 'class:';
196-
const nestedClassId: string = 'nested-class:';
197-
const methodId: string = 'method:';
198-
const testFactoryId: string = 'test-factory:';
199-
// Property id is for jqwik
200-
const propertyId: string = 'property:';
201-
const testTemplateId: string = 'test-template:';
202-
203195
// We're expecting something like:
204196
// [engine:junit5]/[class:com.example.MyTest]/[method:myTest]/[test-template:myTest(String\, int)]
205197
const parts: string[] = message.split('/');
206198

207199
let className: string = '';
208200
let methodName: string = '';
201+
let InvocationSuffix: string = '';
209202

210203
if (!parts || parts.length === 0) {
211204
// 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 {
216209
// Remove the leading and trailing brackets.
217210
part = part.trim().replace(/\[/g, '').replace(/\]/g, '');
218211

219-
if (part.startsWith(classId)) {
220-
className = part.substring(classId.length);
221-
} else if (part.startsWith(methodId)) {
222-
const rawMethodName: string = part.substring(methodId.length);
212+
if (part.startsWith(JUnitTestPart.CLASS)) {
213+
className = part.substring(JUnitTestPart.CLASS.length);
214+
} else if (part.startsWith(JUnitTestPart.METHOD)) {
215+
const rawMethodName: string = part.substring(JUnitTestPart.METHOD.length);
223216
// If the method name exists then we want to include the '#' qualifier.
224217
methodName = `#${this.getJUnit5MethodName(rawMethodName)}`;
225-
} else if (part.startsWith(testFactoryId)) {
226-
const rawMethodName: string = part.substring(testFactoryId.length);
218+
} else if (part.startsWith(JUnitTestPart.TEST_FACTORY)) {
219+
const rawMethodName: string = part.substring(JUnitTestPart.TEST_FACTORY.length);
227220
// If the method name exists then we want to include the '#' qualifier.
228221
methodName = `#${this.getJUnit5MethodName(rawMethodName)}`;
229-
} else if (part.startsWith(nestedClassId)) {
230-
const nestedClassName: string = part.substring(nestedClassId.length);
222+
} else if (part.startsWith(JUnitTestPart.NESTED_CLASS)) {
223+
const nestedClassName: string = part.substring(JUnitTestPart.NESTED_CLASS.length);
231224
className = `${className}$${nestedClassName}`;
232-
} else if (part.startsWith(testTemplateId)) {
233-
const rawMethodName: string = part.substring(testTemplateId.length)
225+
} else if (part.startsWith(JUnitTestPart.TEST_TEMPLATE)) {
226+
const rawMethodName: string = part.substring(JUnitTestPart.TEST_TEMPLATE.length)
234227
.replace(/\\,/g, ',')
235228
.replace(/ /g, '');
236229
// If the method name exists then we want to include the '#' qualifier.
237230
methodName = `#${this.getJUnit5MethodName(rawMethodName)}`;
238-
} else if (part.startsWith(propertyId)) {
239-
const rawMethodName: string = part.substring(propertyId.length)
231+
} else if (part.startsWith(JUnitTestPart.PROPERTY)) {
232+
const rawMethodName: string = part.substring(JUnitTestPart.PROPERTY.length)
240233
.replace(/\\,/g, ',')
241234
.replace(/ /g, '');
242235
// If the method name exists then we want to include the '#' qualifier.
243236
methodName = `#${this.getJUnit5MethodName(rawMethodName)}`;
237+
} else if (part.startsWith(JUnitTestPart.TEST_TEMPLATE_INVOCATION)) {
238+
InvocationSuffix += `[${part.substring(JUnitTestPart.TEST_TEMPLATE_INVOCATION.length)}]`;
239+
} else if (part.startsWith(JUnitTestPart.DYNAMIC_CONTAINER)) {
240+
InvocationSuffix += `[${part.substring(JUnitTestPart.DYNAMIC_CONTAINER.length)}]`;
241+
} else if (part.startsWith(JUnitTestPart.DYNAMIC_TEST)) {
242+
InvocationSuffix += `[${part.substring(JUnitTestPart.DYNAMIC_TEST.length)}]`;
244243
}
245244
});
246245

247246
if (className) {
248-
return `${this.projectName}@${className}${methodName}`;
247+
return `${this.projectName}@${className}${methodName}${InvocationSuffix}`;
249248
} else {
250-
return `${this.projectName}@${message}`;
249+
return `${this.projectName}@${message}${InvocationSuffix}`;
251250
}
252251
}
253252

@@ -354,8 +353,8 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
354353
const parent: TestItem | undefined = parentInfo?.testItem;
355354
if (parent) {
356355
const parentData: ITestItemData | undefined = dataCache.get(parent);
357-
if (parentData?.testLevel === TestLevel.Method) {
358-
testItem = this.enlistDynamicMethodToTestMapping(testItem, parent, parentData, displayName, uniqueId);
356+
if (parentData?.testLevel === TestLevel.Method || parentData?.testLevel === TestLevel.Invocation) {
357+
testItem = this.enlistDynamicMethodToTestMapping(testId, parent, parentData, displayName, uniqueId);
359358
}
360359
}
361360
} else {
@@ -376,7 +375,7 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
376375
jdtHandler: '',
377376
fullName: testId.substr(testId.indexOf('@') + 1),
378377
label: this.getTestMethodName(displayName),
379-
id: `${INVOCATION_PREFIX}${testId}`,
378+
id: testId,
380379
projectName: this.projectName,
381380
testKind: this.testContext.kind,
382381
testLevel: TestLevel.Invocation,
@@ -412,25 +411,23 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
412411
}
413412

414413
// Leaving this public so that it can be mocked when testing.
415-
public enlistDynamicMethodToTestMapping(testItem: TestItem | undefined, parent: TestItem, parentData: ITestItemData, displayName: string, uniqueId: string | undefined): TestItem {
416-
testItem = updateOrCreateTestItem(parent, {
414+
public enlistDynamicMethodToTestMapping(testId: string, parent: TestItem, parentData: ITestItemData, displayName: string, uniqueId: string | undefined): TestItem {
415+
return updateOrCreateTestItem(parent, {
417416
children: [],
418417
uri: parent.uri?.toString(),
419418
range: parent.range,
420419
jdtHandler: parentData.jdtHandler,
421420
fullName: parentData.fullName,
422421
label: this.getTestMethodName(displayName),
423422
// prefer uniqueId, as it does not change when re-running only a single invocation:
424-
id: uniqueId ? `${INVOCATION_PREFIX}${uniqueId}`
425-
: `${INVOCATION_PREFIX}${parent.id}[#${parent.children.size + 1}]`,
423+
id: uniqueId ? uniqueId : testId,
426424
projectName: parentData.projectName,
427425
testKind: parentData.testKind,
428426
testLevel: TestLevel.Invocation,
429427
uniqueId,
430428
// We will just create a string padded with the character "a" to provide easy sorting.
431429
sortText: ''.padStart(parent.children.size + 1, 'a')
432430
});
433-
return testItem;
434431
}
435432

436433
private async tryAppendMessage(item: TestItem, testMessage: TestMessage, testState: TestResultState): Promise<void> {
@@ -440,15 +437,12 @@ export class JUnitRunnerResultAnalyzer extends RunnerResultAnalyzer {
440437
testMessage.location = new Location(item.uri, item.range);
441438
} else {
442439
let id: string = item.id;
443-
if (id.startsWith(INVOCATION_PREFIX)) {
444-
id = id.substring(INVOCATION_PREFIX.length);
445-
if (this.testContext.kind === TestKind.JUnit) {
446-
// change test[0] -> test
447-
// to fix: https://github.com/microsoft/vscode-java-test/issues/1296
448-
const indexOfParameterizedTest: number = id.lastIndexOf('[');
449-
if (indexOfParameterizedTest > -1) {
450-
id = id.substring(0, id.lastIndexOf('['));
451-
}
440+
if (this.testContext.kind === TestKind.JUnit) {
441+
// change test[0] -> test
442+
// to fix: https://github.com/microsoft/vscode-java-test/issues/1296
443+
const indexOfParameterizedTest: number = id.lastIndexOf('[');
444+
if (indexOfParameterizedTest > -1) {
445+
id = id.substring(0, id.lastIndexOf('['));
452446
}
453447
}
454448
const location: Location | undefined = await findTestLocation(id);

‎test/suite/JUnitAnalyzer.test.ts‎

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -430,7 +430,7 @@ org.opentest4j.AssertionFailedError: expected: <1> but was: <2>
430430
// We need to stub this method to avoid issues with the TestController
431431
// not being set up in the non-test version of the utils file.
432432
const stub = sinon.stub(analyzer, "enlistDynamicMethodToTestMapping");
433-
const dummy = generateTestItem(testController, '[__INVOCATION__]-dummy', TestKind.JUnit5, new Range(10, 0, 16, 0));
433+
const dummy = generateTestItem(testController, 'dummy', TestKind.JUnit5, new Range(10, 0, 16, 0));
434434
stub.returns(dummy);
435435
analyzer.analyzeData(testRunnerOutput);
436436

@@ -498,7 +498,45 @@ org.opentest4j.AssertionFailedError: expected: <1> but was: <2>
498498
// We need to stub this method to avoid issues with the TestController
499499
// not being set up in the non-test version of the utils file.
500500
const stub = sinon.stub(analyzer, "enlistDynamicMethodToTestMapping");
501-
const dummy = generateTestItem(testController, '[__INVOCATION__]-dummy', TestKind.JUnit5, new Range(10, 0, 16, 0));
501+
const dummy = generateTestItem(testController, 'dummy', TestKind.JUnit5, new Range(10, 0, 16, 0));
502+
stub.returns(dummy);
503+
analyzer.analyzeData(testRunnerOutput);
504+
505+
sinon.assert.calledWith(startedSpy, dummy);
506+
sinon.assert.calledWith(passedSpy, dummy);
507+
});
508+
509+
test("can handle DynamicContainer", () => {
510+
const testItem = generateTestItem(testController, 'junit@junit5.TestFactoryTest#testContainers()', TestKind.JUnit5, new Range(10, 0, 16, 0));
511+
const testRunRequest = new TestRunRequest([testItem], []);
512+
const testRun = testController.createTestRun(testRunRequest);
513+
const startedSpy = sinon.spy(testRun, 'started');
514+
const passedSpy = sinon.spy(testRun, 'passed');
515+
const testRunnerOutput = `%TESTC 0 v2
516+
%TSTTREE2,junit5.TestFactoryTest,true,1,false,1,TestFactoryTest,,[engine:junit-jupiter]/[class:junit5.TestFactoryTest]
517+
%TSTTREE3,testContainers(junit5.TestFactoryTest),true,0,false,2,testContainers(),,[engine:junit-jupiter]/[class:junit5.TestFactoryTest]/[test-factory:testContainers()]
518+
%TSTTREE4,testContainers(junit5.TestFactoryTest),true,0,true,3,Container,,[engine:junit-jupiter]/[class:junit5.TestFactoryTest]/[test-factory:testContainers()]/[dynamic-container:#1]
519+
%TSTTREE5,testContainers(junit5.TestFactoryTest),false,1,true,4,Test,,[engine:junit-jupiter]/[class:junit5.TestFactoryTest]/[test-factory:testContainers()]/[dynamic-container:#1]/[dynamic-test:#1]
520+
%TESTS 5,testContainers(junit5.TestFactoryTest)
521+
522+
%TESTE 5,testContainers(junit5.TestFactoryTest)
523+
524+
%RUNTIME103`;
525+
const runnerContext: IRunTestContext = {
526+
isDebug: false,
527+
kind: TestKind.JUnit5,
528+
projectName: 'junit',
529+
testItems: [testItem],
530+
testRun: testRun,
531+
workspaceFolder: workspace.workspaceFolders?.[0]!,
532+
};
533+
534+
const analyzer = new JUnitRunnerResultAnalyzer(runnerContext);
535+
536+
// We need to stub this method to avoid issues with the TestController
537+
// not being set up in the non-test version of the utils file.
538+
const stub = sinon.stub(analyzer, "enlistDynamicMethodToTestMapping");
539+
const dummy = generateTestItem(testController, 'dummy', TestKind.JUnit5, new Range(10, 0, 16, 0));
502540
stub.returns(dummy);
503541
analyzer.analyzeData(testRunnerOutput);
504542

‎test/suite/testController.handleInvocations.test.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,12 @@
11
import * as assert from 'assert';
22
import * as sinon from 'sinon';
33
import { TestController, TestItem, tests, window } from "vscode";
4-
import { INVOCATION_PREFIX } from '../../src/constants';
54
import { handleInvocations } from '../../src/controller/testController';
65
import { dataCache } from "../../src/controller/testItemDataCache";
76
import { TestKind, TestLevel } from "../../src/types";
87
import { setupTestEnv } from './utils';
98

109
function generateTestItem(testController: TestController, id: string, testKind: TestKind, testLevel: TestLevel, uniqueId?: string): TestItem {
11-
id = testLevel === TestLevel.Invocation ? INVOCATION_PREFIX + id : id;
1210
const testItem = testController.createTestItem(id, id + '_label');
1311
dataCache.set(testItem, {
1412
jdtHandler: id + '_jdtHandler',

‎test/test-projects/junit/src/test/java/junit5/TestFactoryTest.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,15 @@
11
package junit5;
22

33
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertTrue;
5+
import static org.junit.jupiter.api.DynamicContainer.dynamicContainer;
6+
import static org.junit.jupiter.api.DynamicTest.dynamicTest;
47

8+
import java.util.List;
59
import java.util.function.Function;
610
import java.util.stream.Stream;
711

12+
import org.junit.jupiter.api.DynamicContainer;
813
import org.junit.jupiter.api.DynamicTest;
914
import org.junit.jupiter.api.TestFactory;
1015
import org.junit.jupiter.api.function.ThrowingConsumer;
@@ -17,4 +22,11 @@ Stream<DynamicTest> testDynamicTest() {
1722
final ThrowingConsumer<Long> test = input -> {assertEquals(1L, input);};
1823
return DynamicTest.stream(parameters, testNames, test);
1924
}
25+
26+
@TestFactory
27+
public Stream<DynamicContainer> testContainers() {
28+
return Stream.of(dynamicContainer("Container", List.of(
29+
dynamicTest("Test", () -> {assertTrue(true);})
30+
)));
31+
}
2032
}

0 commit comments

Comments
 (0)