Repository navigation
feat: support additional Java test source paths - #1939
Vinay Kumar Maheshwaram (sudovinay01) wants to merge 11 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
@microsoft-github-policy-service agree |
|
Could a maintainer please help triage this PR or assign a reviewer when available? It has been open since September 26, and I would appreciate guidance on whether any changes are needed before review. Thank you. |
|
Merged current upstream main into this branch in fcb779f and regenerated package-lock.json from the merged manifest, preserving glob as a production dependency. Validation passed: npm ci, npm run compile, npm run lint, npm run vscode:prepublish, npm run build-plugin, and npm test (165 extension tests plus the unmanaged-folder test). GitHub now reports the PR as mergeable. |
|
Hi Jinbo Wang (@testforstephen) Changyong Gong (@chagong) wenyt (@wenytang-ms) — could one of you please review this PR when you have time? I’d appreciate feedback on the change and validation, and I’d like to make sure it aligns with the project’s expectations. Thanks for your time and review. |
|
Thanks Vinay Kumar Maheshwaram (@sudovinay01) for raising this use case and contributing the implementation! Supporting test discovery in main source roots makes sense without requiring users to change their build configuration. The following design aspects need to be adjusted before this can be merged:
The discovery scope should be defined consistently in the Java backend, rather than maintained through a separate client-side scanning path. |
…test-source-paths
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
wenyt (@wenytang-ms) Thanks for the detailed feedback. I have updated the PR to address these points: project-level test discovery now runs through the Java backend and includes selected additional source roots, with the same resolved scope used for Test Explorer, editor actions, file watching, and Go to Test. The separate client-side Java-file scan has been removed; configured literals/globs are intersected with source roots from imported Java projects, and the setting description clarifies that discovery does not change source roots or build configuration. I retained glob support for selecting roots in multi-module projects, with duplicate matching roots deduplicated. Validation now includes |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Hi Jinbo Wang (@testforstephen) Changyong Gong (@chagong) wenyt (@wenytang-ms), could a code owner please review this PR when you have a chance? I’d appreciate your feedback. Thank you! |
| // JDT LS provides core ASM; load the support bundles before JaCoCo imports them. | ||
| 'org.objectweb.asm.commons_', | ||
| 'org.objectweb.asm.tree_' | ||
| 'org.objectweb.asm.tree_', | ||
| 'org.jacoco.core_' |
There was a problem hiding this comment.
Problem:
org.objectweb.asm_ was removed from bundleList and contributes.javaExtensions, and custom bundleOrder sorting was added.
- Removing
org.objectweb.asm_9.10.1.jarbreaks the fix from fix: ship org.objectweb.asm bundles to satisfy Jacoco 0.8.14 (#1866) #1872 (Test Runner for Java Extension downloads old JUnit standalone #1866).com.microsoft.java.test.pluginrequiresorg.objectweb.asm;bundle-version="[9.10.1,9.11.0)"(MANIFEST.MF), andredhat.javareleases are decoupled fromvscode-java-test. If a user'sredhat.javaships an older ASM version, the test plugin will fail to resolve at startup. - In JDT LS (
BundleUtils.loadBundles), all bundles injavaExtensionsare installed first and collected into aHashSet<Bundle>beforestartBundlesis called, so the array order inpackage.jsondoes not affect OSGi resolution or start order.
Solution:
Please revert the changes to scripts/buildJdtlsExt.js and contributes.javaExtensions in package.json so org.objectweb.asm_9.10.1.jar and the alphabetical .sort() are preserved.
| if (token) { | ||
| if (token.isCancellationRequested) { | ||
| return Promise.resolve([]); | ||
| } | ||
| return getSourcePaths([workspaceFolder.uri.toString()], token).then((sourcePaths: ITestSourcePath[]) => | ||
| token.isCancellationRequested | ||
| ? [] | ||
| : resolveAdditionalTestSourcePaths(workspaceFolder.uri.fsPath, configuredPaths, | ||
| sourcePaths.map((sourcePath: ITestSourcePath) => sourcePath.testSourcePath))); | ||
| } |
There was a problem hiding this comment.
Problem:
In getAdditionalTestSourcePaths(workspaceFolder, token), passing a token bypasses this.additionalTestSourceMapping and always calls getSourcePaths([workspaceFolder.uri.toString()], token). Because resolveTestItemChildren in testController.ts always passes a CancellationToken when resolving project items, expanding a multi-module workspace with N projects triggers N separate vscode.java.test.get.sourcepath requests for the same workspace folder instead of reusing the cache.
Solution:
Reuse this.additionalTestSourceMapping even when token is provided (only evicting the cached promise if the request fails or is cancelled before completion), and check token.isCancellationRequested before/after awaiting the cached promise.
| }, | ||
| "dependencies": { | ||
| "fs-extra": "^10.1.0", | ||
| "glob": "^13.0.6", |
There was a problem hiding this comment.
Problem:
glob was moved from devDependencies to production dependencies in package.json only to import hasMagic in src/provider/testSourceProvider.ts, while minimatch is already a direct production dependency.
Solution:
Keep glob in devDependencies and use minimatch directly (e.g., new minimatch.Minimatch(pattern).hasMagic()) instead of importing hasMagic from glob.
| const matchPath: string = path.isAbsolute(normalizedPattern) | ||
| ? sourcePath.replace(/\\/g, '/') | ||
| : path.relative(workspacePath, sourcePath).replace(/\\/g, '/'); | ||
| if (matchPath !== '..' && !matchPath.startsWith('../') && !path.isAbsolute(matchPath) && | ||
| matcher.match(matchPath)) { |
There was a problem hiding this comment.
Problem: When normalizedPattern is an absolute glob pattern (path.isAbsolute(normalizedPattern) is true), matchPath is set to sourcePath.replace(/\\/g, '/'), which is an absolute path. Because the if condition unconditionally requires !path.isAbsolute(matchPath), absolute glob patterns will always fail this check and never match any source root.
Solution: Only apply the relative-path boundary check (matchPath !== '..' && !matchPath.startsWith('../') && !path.isAbsolute(matchPath)) when !path.isAbsolute(normalizedPattern), and add a unit test covering absolute glob patterns.
| assertTrue(packages.get(0).getChildren().stream() | ||
| .noneMatch(item -> "AdditionalMainSourceTest".equals(item.getLabel()))); | ||
|
|
||
| final String mainSourcePath = javaProject.getPackageFragmentRoot(project.getFolder("src/main/java")) | ||
| .getResource().getLocation().toOSString(); | ||
| final List<JavaTestItem> packagesWithAdditionalSource = TestSearchUtils.findTestPackagesAndTypes( | ||
| Arrays.<Object>asList(javaProject.getHandleIdentifier(), Collections.singletonList(mainSourcePath)), | ||
| new NullProgressMonitor()); | ||
| assertTrue(packagesWithAdditionalSource.get(0).getChildren().stream() | ||
| .anyMatch(item -> "AdditionalMainSourceTest".equals(item.getLabel()))); | ||
| assertTrue(packagesWithAdditionalSource.get(0).getChildren().stream() | ||
| .anyMatch(item -> "MiniTest".equals(item.getLabel()))); | ||
|
|
||
| final String additionalTestUri = javaProject.findType("example.AdditionalMainSource") | ||
| .getResource().getLocationURI().toString(); | ||
| final Object defaultNavigation = TestNavigationUtils.findTestOrTarget( | ||
| Arrays.asList(additionalTestUri, true), new NullProgressMonitor()); | ||
| assertTrue(getNavigationItems(defaultNavigation).isEmpty()); | ||
| final Object navigationWithAdditionalSource = TestNavigationUtils.findTestOrTarget(Arrays.<Object>asList( | ||
| additionalTestUri, true, Collections.singletonList(mainSourcePath)), new NullProgressMonitor()); | ||
| assertEquals(1, getNavigationItems(navigationWithAdditionalSource).size()); |
There was a problem hiding this comment.
Problem: preview-junit was specifically set up as a plain Eclipse JDT project (javanature only) to test JUnit 5 discovery with older --enable-preview compiler settings. Converting preview-junit into a Maven project (pom.xml + m2e.core.maven2Nature) and appending additionalSourcePaths assertions into testDiscoverJUnit5TestsWithOlderPreviewSource() couples two unrelated test scenarios and risks m2e overriding the custom .classpath / compiler .settings that the preview test relies on.
Solution: Keep the preview-junit fixture and testDiscoverJUnit5TestsWithOlderPreviewSource() unchanged, and add a separate @Test method in TestSearchUtilsTest (using an existing Maven fixture or a dedicated fixture) to test additionalSourcePaths discovery and navigation.
Why this is needed
The Java Test Runner currently discovers test files from source roots classified as test roots by the Java project model. In Gradle projects,
src/main/javaremains a main source root, so TestNG classes there can compile but are not discovered for editor test actions or Test Explorer. Reclassifying that directory as a Gradle test source set would change build semantics and is not appropriate for projects that intentionally keep these classes in main sources.IntelliJ IDEA and Eclipse already discover tests in both main and test source folders and provide run/debug actions for them. This change brings that IDE-level discovery capability to VS Code as an opt-in, without changing Gradle source-set classification or task behavior.
What this changes
java.test.additionalTestSourcePathssetting. Its default is an empty array, preserving existing behavior.globlibrary is declared as a production dependency.Configuration example
{ "java.test.additionalTestSourcePaths": ["app/**/src/main/java"] }Reproducer
The manual acceptance project is available at https://github.com/sudovinay01/repro-java-issue.
Validation
npm test: 165 extension tests passed; activation test passed.npm run lintpassed.AppMainSourceTestandAppTestappeared in Test Explorer and both passed (2/2).