Running Kotlin and Spock tests#1829
Conversation
|
The test vsix files: |
0e69c15 to
f2d3ee5
Compare
There was a problem hiding this comment.
Pull request overview
This PR targets improved test discovery/execution for non-Java JVM test frameworks (notably Spock and Kotlin-generated tests) by enhancing how tests are discovered (server-side) and how runner output is parsed into VS Code test IDs (client-side).
Changes:
- Extend JUnit runner output parsing to recognize Spock engine output and Spock-specific parts (
spec:/feature:). - Enhance Java-side test discovery and test item construction to better support binary elements (e.g., Kotlin/Gradle scenarios) and add display-name plumbing for frameworks that provide richer names.
- Update bundled JUnit platform dependencies referenced by the extension.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/runners/junitRunner/JUnitRunnerResultAnalyzer.ts | Adds Spock engine + spec/feature parsing to map runner output to test IDs. |
| src/constants.ts | Introduces new JUnitTestPart tokens for Spock (SPEC, FEATURE). |
| package.json | Updates referenced JUnit jar versions in contributes.javaExtensions. |
| java-extension/.../util/TestSearchUtils.java | Refactors test discovery flow and adds binary-type handling for direct children resolution. |
| java-extension/.../util/TestItemUtils.java | Changes range fallback to always return a Range (no longer null). |
| java-extension/.../searcher/TestFrameworkSearcher.java | Adds getDisplayName(IMethodBinding) to support richer method labels. |
| java-extension/.../searcher/JUnit5TestSearcher.java | Treats Spock feature metadata as “test method” and extracts display name from annotations. |
| java-extension/.../searcher/JUnit4TestSearcher.java | Implements new interface method (returns null). |
| java-extension/.../searcher/TestNGTestSearcher.java | Implements new interface method (returns null). |
| java-extension/.../searcher/JUnit6TestFinder.java | Adjusts JUnit 6 test detection logic using additional annotation checks. |
| java-extension/.../model/builder/JavaTestItemBuilder.java | Adds displayName support and attempts to map binary elements back to source via ASM. |
| java-extension/.../launchers/JUnitLaunchConfigurationDelegate.java | Avoids failing on binary methods when attempting to compute JUnit5/6 parameter signatures. |
Comments suppressed due to low confidence (3)
java-extension/com.microsoft.java.test.plugin/src/main/java/com/microsoft/java/test/plugin/util/TestSearchUtils.java:206
JavaTestItemBuilder.build()can returnnull(e.g., when no workspaceIResourceexists), butclassItemis still used unconditionally afterwards. IfclassItemisnull,packageItem.addChild(classItem)will addnull(and any later traversal may NPE). Consider skipping the currenttypewhenclassItemisnull, or guardaddChildcalls so only non-null items are attached.
for (final IType type : testTypes) {
JavaTestItem classItem = testItemMapping.get(type.getHandleIdentifier());
if (classItem == null) {
classItem = new JavaTestItemBuilder().setJavaElement(type)
.setLevel(TestLevel.CLASS)
.setKind(kind)
.build();
if (classItem != null) {
testItemMapping.put(classItem.getJdtHandler(), classItem);
}
} else {
// 1. We suppose a class can only use one test framework
// 2. If more accurate kind is available, use it.
if (classItem.getTestKind() == TestKind.JUnit5 && kind == TestKind.JUnit) {
classItem.setTestKind(TestKind.JUnit);
}
}
final IType declaringType = type.getDeclaringType();
if (declaringType == null) {
// it's a root type, we find its declaring package
final IPackageFragment packageFragment = type.getPackageFragment();
final String packageIdentifier = packageFragment.getHandleIdentifier();
JavaTestItem packageItem = testItemMapping.get(packageIdentifier);
if (packageItem == null) {
packageItem = new JavaTestItemBuilder().setJavaElement(packageFragment)
.setLevel(TestLevel.PACKAGE)
.setKind(TestKind.None)
.build();
if (packageItem != null) {
testItemMapping.put(packageIdentifier, packageItem);
}
}
if (packageItem.getChildren() == null || !packageItem.getChildren().contains(classItem)) {
packageItem.addChild(classItem);
}
java-extension/com.microsoft.java.test.plugin/src/main/java/com/microsoft/java/test/plugin/util/TestSearchUtils.java:206
packageItemcan remainnullifnew JavaTestItemBuilder(...).build()returnsnull, but the code still callspackageItem.getChildren()/packageItem.addChild(...)unconditionally. This will throw aNullPointerException. Add a guard after the build so the method skips linking children whenpackageItemcould not be created.
JavaTestItem packageItem = testItemMapping.get(packageIdentifier);
if (packageItem == null) {
packageItem = new JavaTestItemBuilder().setJavaElement(packageFragment)
.setLevel(TestLevel.PACKAGE)
.setKind(TestKind.None)
.build();
if (packageItem != null) {
testItemMapping.put(packageIdentifier, packageItem);
}
}
if (packageItem.getChildren() == null || !packageItem.getChildren().contains(classItem)) {
packageItem.addChild(classItem);
}
java-extension/com.microsoft.java.test.plugin/src/main/java/com/microsoft/java/test/plugin/util/TestSearchUtils.java:222
declaringTypeItemcan remainnullif the builder returnsnull, but the code still dereferences it (declaringTypeItem.getChildren()/declaringTypeItem.addChild(...)). This can lead to aNullPointerExceptionduring test discovery. Add a guard to only attachclassItemwhendeclaringTypeItemis non-null.
JavaTestItem declaringTypeItem = testItemMapping.get(declaringTypeIdentifier);
if (declaringTypeItem == null) {
declaringTypeItem = new JavaTestItemBuilder().setJavaElement(declaringType)
.setLevel(TestLevel.CLASS)
.setKind(kind)
.build();
if (declaringTypeItem != null) {
testItemMapping.put(declaringTypeIdentifier, declaringTypeItem);
}
}
if (declaringTypeItem.getChildren() == null ||
!declaringTypeItem.getChildren().contains(classItem)) {
declaringTypeItem.addChild(classItem);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import org.eclipse.jdt.internal.core.BinaryMember; | ||
| import org.eclipse.jdt.internal.core.BinaryMethod; | ||
| import org.eclipse.jdt.internal.core.BinaryType; | ||
| import org.eclipse.jdt.internal.core.PackageFragmentRoot; | ||
| import org.eclipse.jdt.internal.core.manipulation.JavaElementLabelsCore; | ||
| import org.eclipse.jdt.ls.core.internal.JDTUtils; | ||
| import org.eclipse.jdt.ls.core.internal.JavaLanguageServerPlugin; | ||
| import org.eclipse.jdt.ls.core.internal.ProjectUtils; |
There was a problem hiding this comment.
This file now relies on several org.eclipse.jdt.internal.* types (BinaryMember, BinaryMethod, BinaryType, PackageFragmentRoot). Elsewhere in this codebase, files that touch internal Eclipse APIs are annotated with @SuppressWarnings("restriction") (e.g., TestSearchUtils, TestItemUtils). Consider adding the same suppression (or refactoring to public JDT APIs) to avoid Tycho/Eclipse "Discouraged access" build warnings or failures.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| protected getTestId(message: string): string { | ||
| if (message.includes('engine:junit5') || message.includes('engine:junit-jupiter') || message.includes('engine:jqwik')) { | ||
| if (message.includes('engine:junit5') || message.includes('engine:junit-jupiter') || message.includes('engine:jqwik') || message.includes('engine:spock')) { | ||
| return this.getTestIdForJunit5Method(message); | ||
| } else { |
There was a problem hiding this comment.
New parsing paths were added for Spock (engine:spock, spec:, feature:). The analyzer already has a comprehensive unit-test suite (e.g., for test-factory, dynamic tests, templates), but there are no corresponding tests exercising Spock-formatted runner output. Add a unit test with representative %TSTTREE... lines containing [engine:spock]/[spec:...]/[feature:...] to lock in the expected IDs and prevent regressions.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| } 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(JUnitTestPart.TEST_FACTORY)) { | ||
| const rawMethodName: string = part.substring(JUnitTestPart.TEST_FACTORY.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(JUnitTestPart.FEATURE)) { | ||
| const rawMethodName: string = part.substring(JUnitTestPart.FEATURE.length); | ||
| // If the method name exists then we want to include the '#' qualifier. | ||
| methodName = '#' + rawMethodName + '()'; |
There was a problem hiding this comment.
In getTestIdForJunit5Method, the TEST_FACTORY case appears to have been accidentally replaced with a duplicate METHOD branch. As a result, [test-factory:...] parts from JUnit5 output will no longer be parsed into methodName, which will break existing @TestFactory/dynamic test mapping (and will likely regress existing analyzer tests covering test-factory). Restore the JUnitTestPart.TEST_FACTORY branch and remove the duplicate METHOD condition.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| } else if (element instanceof BinaryMethod) { | ||
| final String methodDescriptor = ((BinaryMethod) element).getSignature(); | ||
| cr.accept(new ClassVisitor(Opcodes.ASM9) { | ||
| @Override | ||
| public void visitSource(String source, String debug) { | ||
| sources[0] = source; | ||
| } | ||
|
|
||
| public MethodVisitor visitMethod(int access, String name, String descriptor, | ||
| String signature, String[] exceptions) { | ||
| if (name.equals(element.getElementName()) && descriptor.equals(methodDescriptor)) { | ||
| return new MethodVisitor(Opcodes.ASM9) { |
There was a problem hiding this comment.
When matching a BinaryMethod in ASM, BinaryMethod#getSignature() returns a JDT method signature, which typically does not match ASM's JVM descriptor string (visitMethod's descriptor). This comparison (descriptor.equals(methodDescriptor)) will often fail, preventing line number extraction for binary methods. Consider converting the JDT signature into a JVM descriptor (or matching by name + parameter types using the classfile model) before comparing.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| if (lines[0] > 0) { | ||
| final Position line = new Position(lines[0] - 1, 0); |
There was a problem hiding this comment.
The line-number-to-Position conversion appears off by one: visitLineNumber decrements lines[0] and then later new Position(lines[0] - 1, 0) subtracts again. This can shift ranges up by an extra line and also causes line 1 to be dropped entirely (lines[0] becomes 0, failing the lines[0] > 0 check). Use a single 1-based→0-based conversion at one point (preferably when constructing the Position).
| if (lines[0] > 0) { | |
| final Position line = new Position(lines[0] - 1, 0); | |
| if (lines[0] >= 0) { | |
| final Position line = new Position(lines[0], 0); |
Fixes #425
The related issues:
I have tested Kotlin plugin 2.3.0, Spock 2.4 and VS Code Gradle buildship.
Steps to reproduce:
See Support Kotlin,Groovy in Gradle projects redhat-developer/vscode-java#4312
Spock tests
Kotlin tests