Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClasspathScanner.getRootUrisForPackage returns empty list when package name conflicts with module name #2500

Closed
oobles opened this issue Dec 20, 2020 · 14 comments · Fixed by #2531 or #2613

Comments

@oobles
Copy link

oobles commented Dec 20, 2020

When developing a multi-module project that has similar package names it is possible that getRootUrisForPackage will fail to return a valid list.

Steps to reproduce

Project1.

module io.litterat.pep {
	exports io.litterat.pep;
	exports io.litterat.pep.mapper;
}

Project2.

open module io.litterat.pep.test {
        exports io.litterat.pep.test;
	requires io.litterat.pep;
	requires org.junit.jupiter.api;
}

In eclipse (2020-09,2020-12 and other versions) when selecting the package io.litterat.pep.test no JUnit 5 tests are found or executed. In 2020-12 no errors or console output is shown.

The root cause is that getRootUrisForPackage( "io.litterat.pep.test" ) results in calling getClassLoader().getResources( "io/litterat/pep/test" ). This results in calling ClassLoaders$BootClassLoader findResources which calls Resources.toPackageName("io/litterat/pep/test" ) which results in package name "io.litterat.pep" (the name of the first module). The loader then resolves the name to the wrong LoadedModule which does not have the selected package. This results in no resources being returned.

Using Eclipse and attempting to execute all tests on either source directory or project results in all tests in the io.litterat.pep.test package being skipped. However, tests in sub-packages (e.g. io.litterat.pep.test.extra) are executed correctly.

Changing the name of the package so that the second project does not use the other module package as a base corrects the problem (e.g. io.litterat.peptest).

Context

  • Used versions (Jupiter/Vintage/Platform): junit-platform-commons-1.7.0.jar
  • Build Tool/IDE: Eclipse

Deliverables

It's not clear if this is a JDK bug or an issue with the way getResources is being called. Given that no output or feedback is provided this has taken quite a long time to figure out. Either issue should be documented or fixed.

oobles added a commit to litterat/litterat that referenced this issue Dec 21, 2020
@marcphilipp
Copy link
Member

@oobles Thanks for the report! Could you please provide a small reproducer project?

@oobles
Copy link
Author

oobles commented Dec 22, 2020

Here's a couple of modules created in eclipse that demonstrates the issue. Running ProjectTest directly results in the test running. Running tests on com.project.test results in no test executing. This was run on Eclipse 2020-12 using JDK15 (installed by eclipse).

junit-error.zip

@sbrannen
Copy link
Member

@noopur2507, can you provide any input for this?

Is it perhaps an issue due to the module-path?

@sormuras
Copy link
Member

sormuras commented Dec 22, 2020

FYI: running the JUnit Console Launcher on the module-path and various test-related IDEA run configurations that make use of the module-path do run fine. With modules that packages that use the same name as their modules.

How does the command line generated by Eclipse look like?

@sormuras
Copy link
Member

When searching test classes within a Java module, a tool (here Eclipse) should make use of the --scan-module option offered by the Console Launcher or class ModuleSelector if it uses the Platform API directly.

@oobles
Copy link
Author

oobles commented Dec 28, 2020

Just to demonstrate this a little clearer, the following fails.

Enumeration<URL> urlPackage = this.getClass().getClassLoader().getResources("com/project/test");
Assertions.assertTrue(urlPackage.hasMoreElements());

This fails because Java's BuiltinClassLoader.findResources starts with these two lines:

String pn = Resources.toPackageName(name);
LoadedModule module = packageToModule.get(pn);

pn becomes "com.project" instead of "com.project.test". It picks up the wrong module.

It succeeds when you put a / at the end.

Enumeration<URL> urlPackage = this.getClass().getClassLoader().getResources("com/project/test/");
Assertions.assertTrue(urlPackage.hasMoreElements());

@sormuras
Copy link
Member

Thanks for the details, David.

Seems that appending a missing / before calling getClassLoader().getResources(...) should fix this issue without breaking existing working code.

@sormuras
Copy link
Member

sormuras commented Jan 4, 2021

I can not reproduce it using Java's module API and JUnit's internal scanner support.

Having this modular JAR file (jar --list --file com.greetings@1-ea.jar):

META-INF/
META-INF/MANIFEST.MF
module-info.class
com/
com/greetings/
com/greetings/Main.class

Yields a successful test like this:

@Test
// #2500
void scanForClassesInPackageWithinModuleSharingNames() throws Exception {
  var jarfile = getClass().getResource("/com.greetings@1-ea.jar");
  
  var module = "com.greetings";
  var before = ModuleFinder.of();
  var finder = ModuleFinder.of(Path.of(jarfile.toURI()));
  var boot = ModuleLayer.boot();
  var configuration = boot.configuration().resolveAndBind(before, finder, Set.of(module));
  var parent = ClassLoader.getPlatformClassLoader();
  var layer = ModuleLayer.defineModulesWithOneLoader(configuration, List.of(boot), parent).layer();
  
  var classpathScanner = new ClasspathScanner(() -> layer.findLoader(module), ReflectionUtils::tryToLoadClass);
  
  var classes = classpathScanner.scanForClassesInPackage("com.greetings", allClasses);
  var classNames = classes.stream().map(Class::getName).collect(Collectors.toList());
  assertThat(classNames).hasSize(1).contains("com.greetings.Main");
}

Perhaps, I didn't reproduce your setup correctly or it's maybe an issue within Eclipse's way of handling (exploded) modules?

@marcphilipp
Copy link
Member

@oobles @sormuras Any updates? I don't want to block the 5.7.1 release because of this issue.

@oobles
Copy link
Author

oobles commented Jan 16, 2021

@sormuras it doesn't look like your test includes two modules. Create another module with the package com.greetings.test package that requires com.greetings.

@sormuras
Copy link
Member

sormuras commented Jan 16, 2021

Well. Now I'm able to reproduce this bug.

With a module named com.greetings.test containing following entries:

com.greetings.test@0-ea.jar
META-INF/
META-INF/MANIFEST.MF
module-info.class
com/
com/greetings/
com/greetings/test/
com/greetings/test/Tests.class

...the updated test case:

@Test
// #2500
void scanForClassesInPackageWithinModuleSharingNames() throws Exception {
	var greetings = getClass().getResource("/com.greetings@1-ea.jar").toURI();
	var tests = getClass().getResource("/com.greetings.test@0-ea.jar").toURI();

	var module = "com.greetings.test";
	var before = ModuleFinder.of();
	var finder = ModuleFinder.of(Path.of(greetings), Path.of(tests));
	var boot = ModuleLayer.boot();
	var configuration = boot.configuration().resolveAndBind(before, finder, Set.of(module));
	var parent = ClassLoader.getPlatformClassLoader();
	var layer = ModuleLayer.defineModulesWithOneLoader(configuration, List.of(boot), parent).layer();

	var classpathScanner = new ClasspathScanner(() -> layer.findLoader(module), ReflectionUtils::tryToLoadClass);
	{
		var classes = classpathScanner.scanForClassesInPackage("com.greetings", allClasses);
		var classNames = classes.stream().map(Class::getName).collect(Collectors.toList());
		assertThat(classNames).hasSize(2).contains("com.greetings.Main", "com.greetings.test.Tests");
	}
	{
		var classes = classpathScanner.scanForClassesInPackage("com.greetings.test", allClasses);
		var classNames = classes.stream().map(Class::getName).collect(Collectors.toList());
		assertThat(classNames).hasSize(1).contains("com.greetings.test.Tests");
	}
}

...in the last line due to an empty list of class names.

After adding the missing / at the end here:

// ClasspathScanner.java line 213

private static String packagePath(String packageName) {
	return packageName.replace(PACKAGE_SEPARATOR_CHAR, CLASSPATH_RESOURCE_PATH_SEPARATOR)
			+ CLASSPATH_RESOURCE_PATH_SEPARATOR;
}

...the assertion is met, i.e. class com.greetings.test.Tests is found when scanning for classes in package com.greetings.test.

@marcphilipp @sbrannen - before I continue with this fix, I want make sure that it is the intended behaviour of classpathScanner.scanForClassesInPackage(X) to scan all packages that start with X as their name.

@marcphilipp
Copy link
Member

before I continue with this fix, I want make sure that it is the intended behaviour of classpathScanner.scanForClassesInPackage(X) to scan all packages that start with X as their name.

As documented at the entry point in ReflectionSupport:

* <p>The classpath scanning algorithm searches recursively in subpackages
* beginning within the supplied base package.

Thus, it should find all classes in package X and "subpackages", i.e. packages that starts with X., but not "sibling packages" that start with X but not X..

sormuras added a commit that referenced this issue Jan 16, 2021
Prior to this commit no trailing `/` character was
appended to the computed package path.
Now, except for the default package `""`, a `/` is
appended to package path. This leads to corrected
and documented behavior even if two modules start
with the same name elements.

Fixes #2500
sormuras added a commit that referenced this issue Jan 21, 2021
Prior to this commit no trailing `/` character was
appended to the computed package path.
Now, except for the default package `""`, a `/` is
appended to package path. This leads to corrected
and documented behavior even if two modules start
with the same name elements.

Fixes #2500
sormuras added a commit that referenced this issue Jan 21, 2021
Prior to this commit no trailing `/` character was
appended to the computed package path.
Now, except for the default package `""`, a `/` is
appended to package path. This leads to corrected
and documented behavior even if two modules start
with the same name elements.

Fixes #2500
sormuras added a commit that referenced this issue Jan 21, 2021
Prior to this commit no trailing `/` character was
appended to the computed package path.
Now, except for the default package `""`, a `/` is
appended to package path. This leads to corrected
and documented behavior even if two modules start
with the same name elements.

Fixes #2500
sormuras added a commit that referenced this issue Jan 21, 2021
Prior to this commit no trailing `/` character was
appended to the computed package path.
Now, except for the default package `""`, a `/` is
appended to package path. This leads to corrected
and documented behavior even if two modules start
with the same name elements.

Fixes #2500
sormuras added a commit that referenced this issue Jan 26, 2021
Prior to this commit no trailing `/` character was
appended to the computed package path.

Now, except for the default package `""`, a `/` is
appended to package path. This leads to corrected
and documented behavior even if two modules start
with the same name elements.

Fixes #2500
marcphilipp pushed a commit that referenced this issue Jan 27, 2021
Prior to this commit no trailing `/` character was
appended to the computed package path.

Now, except for the default package `""`, a `/` is
appended to package path. This leads to corrected
and documented behavior even if two modules start
with the same name elements.

Fixes #2500
# Conflicts:
#	documentation/src/docs/asciidoc/release-notes/release-notes-5.8.0-M1.adoc
@sormuras
Copy link
Member

sormuras commented May 7, 2021

Reopen this issue due to unwanted side-effects produced by the fix. See #2600 for details.

@sormuras sormuras reopened this May 7, 2021
@sormuras sormuras modified the milestones: 5.7.1, 5.7.2 May 7, 2021
@sormuras
Copy link
Member

sormuras commented May 7, 2021

New Repro-Setup

F:/junit5-2500
│   Main.class
│   Main.java
│
├───classes
│   ├───foo
│   │   │   module-info.class
│   │   │
│   │   └───foo
│   │           Foo.class
│   │
│   └───foo.test
│       │   module-info.class
│       │
│       └───foo
│           └───test
│                   FooTest.class
...
import java.net.*;
import java.util.*;

class Main {
    public static void main(String[] args) {
        System.out.printf("Java %s%n", System.getProperty("java.version"));
        printRootUrisForPackage("");
        printRootUrisForPackage("/");
        printRootUrisForPackage("foo");
        printRootUrisForPackage("foo/");
        printRootUrisForPackage("foo/test");
        printRootUrisForPackage("foo/test/");
    }

    static void printRootUrisForPackage(String basePackageName) {
      try {
        System.out.printf("basePackageName: '%s'%n", basePackageName);
        Enumeration<URL> resources = Main.class.getClassLoader().getResources(basePackageName);
        while (resources.hasMoreElements()) {
            URL resource = resources.nextElement();
            System.out.printf("  %s%n", resource);
        }
      } catch(Exception exception) {
        System.out.println("  " + exception);
      }
    }
}

Java 8 (-classpath)

javac Main.java && java -classpath .;classes Main
Java 1.8.0_281
basePackageName: ''
  file:/F:/junit5-2500/
  file:/F:/junit5-2500/classes/
basePackageName: '/'
basePackageName: 'foo'
  file:/F:/junit5-2500/foo
  file:/F:/junit5-2500/classes/foo
basePackageName: 'foo/'
  file:/F:/junit5-2500/foo/
  file:/F:/junit5-2500/classes/foo/
basePackageName: 'foo/test'
basePackageName: 'foo/test/'

Java 16 (--class-path)

java --class-path classes Main.java
Java 16.0.1
basePackageName: ''
  file:/F:/junit5-2500/classes/
basePackageName: '/'
basePackageName: 'foo'
  file:/F:/junit5-2500/classes/foo
basePackageName: 'foo/'
  file:/F:/junit5-2500/classes/foo/
basePackageName: 'foo/test'
basePackageName: 'foo/test/'

Java 16 (--module-path)

java --module-path classes --add-modules foo,foo.test Main.java
Java 16.0.1
basePackageName: ''
  file:/F:/junit5-2500/
basePackageName: '/'
basePackageName: 'foo'
  file:/F:/junit5-2500/classes/foo/foo/
  file:/F:/junit5-2500/classes/foo.test/foo/
  file:/F:/junit5-2500/foo
basePackageName: 'foo/'
  file:/F:/junit5-2500/classes/foo/foo/
  file:/F:/junit5-2500/classes/foo.test/foo/
  file:/F:/junit5-2500/foo/
basePackageName: 'foo/test'
basePackageName: 'foo/test/'
  file:/F:/junit5-2500/classes/foo.test/foo/test/

sormuras added a commit that referenced this issue May 11, 2021
Fix `getRootUrisForPackage()` in class `ClasspathScanner` by looking for two
"wordings" of a package name.

For example, package `foo.bar` is expanded to `foo/bar` and `foo/bar/`.
The latter allows find package `foo.bar` in a module called `foo.bar`,
and not `foo`.

Fixes #2500
Fixes #2600
Fixes #2612
sormuras added a commit that referenced this issue May 12, 2021
Fix `getRootUrisForPackage()` in class `ClasspathScanner` by looking for two
"wordings" of a package name.

For example, package `foo.bar` is expanded to `foo/bar` and `foo/bar/`.
The latter allows find package `foo.bar` in a module called `foo.bar`,
and not `foo`.
This commit also ensures, that there's no regression (compared to #2531)
by launching test runs using the standalone artifact with `--select-package`
and `--scan-class-path`.

Fixes #2500
Fixes #2600
Fixes #2612
marcphilipp pushed a commit that referenced this issue May 13, 2021
Fix `getRootUrisForPackage()` in class `ClasspathScanner` by looking for two
"wordings" of a package name.

For example, package `foo.bar` is expanded to `foo/bar` and `foo/bar/`.
The latter allows find package `foo.bar` in a module called `foo.bar`,
and not `foo`.
This commit also ensures, that there's no regression (compared to #2531)
by launching test runs using the standalone artifact with `--select-package`
and `--scan-class-path`.

Fixes #2500
Fixes #2600
Fixes #2612
runningcode pushed a commit to runningcode/junit5 that referenced this issue Feb 15, 2023
runningcode pushed a commit to runningcode/junit5 that referenced this issue Feb 15, 2023
Prior to this commit no trailing `/` character was
appended to the computed package path.

Now, except for the default package `""`, a `/` is
appended to package path. This leads to corrected
and documented behavior even if two modules start
with the same name elements.

Fixes junit-team#2500
runningcode pushed a commit to runningcode/junit5 that referenced this issue Feb 15, 2023
Fix `getRootUrisForPackage()` in class `ClasspathScanner` by looking for two
"wordings" of a package name.

For example, package `foo.bar` is expanded to `foo/bar` and `foo/bar/`.
The latter allows find package `foo.bar` in a module called `foo.bar`,
and not `foo`.
This commit also ensures, that there's no regression (compared to junit-team#2531)
by launching test runs using the standalone artifact with `--select-package`
and `--scan-class-path`.

Fixes junit-team#2500
Fixes junit-team#2600
Fixes junit-team#2612
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment