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

Fix package path computation in ClasspathScanner #2531

Merged
merged 4 commits into from
Jan 26, 2021

Conversation

sormuras
Copy link
Member

@sormuras sormuras commented Jan 16, 2021

Overview

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

I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

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
@codecov
Copy link

codecov bot commented Jan 16, 2021

Codecov Report

Merging #2531 (be7d186) into main (d64a4a5) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #2531      +/-   ##
============================================
+ Coverage     90.56%   90.57%   +0.01%     
- Complexity     4683     4684       +1     
============================================
  Files           407      407              
  Lines         11568    11571       +3     
  Branches        923      923              
============================================
+ Hits          10476    10480       +4     
  Misses          814      814              
+ Partials        278      277       -1     
Impacted Files Coverage Δ Complexity Δ
.../junit/platform/commons/util/ClasspathScanner.java 88.17% <100.00%> (+0.39%) 27.00 <2.00> (+1.00)
...jupiter/engine/execution/ExtensionValuesStore.java 93.18% <0.00%> (+1.13%) 25.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d64a4a5...6321af0. Read the comment docs.

}
{
var classes = classpathScanner.scanForClassesInPackage("com.greetings.test", allClasses);
var classNames = classes.stream().map(Class::getName).collect(Collectors.toList());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd use a static import for toList().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait until we switched to Java 16 -- and replace all those with Stream::toList

https://twitter.com/sormuras/status/1337495352962904066

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All? All 44 usages found in test files.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go for it! 👍

Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How was the platform-tests/src/test/resources/com.greetings.test@0-ea.jar file created; what does it contain; and can we reproduce/modify it?

@sormuras
Copy link
Member Author

How was the platform-tests/src/test/resources/com.greetings.test@0-ea.jar file created; what does it contain; and can we reproduce/modify it?

Good questions. I'll replace this JAR file and the related com.greetings@1-ea.jar one with a on-the-fly compilation solution.

Btw. do we know how to reproduce/modify gh-1436-invalid-nested-class-file.jar and jartest.jar?

@sormuras sormuras force-pushed the issues/2500-scan-for-classes-in-modules branch 2 times, most recently from 40b8377 to a7ec3a4 Compare January 21, 2021 08:03
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 sormuras force-pushed the issues/2500-scan-for-classes-in-modules branch from a7ec3a4 to 6cf0011 Compare January 21, 2021 08:13
Copy link
Member

@marcphilipp marcphilipp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@sbrannen
Copy link
Member

Btw. do we know how to reproduce/modify gh-1436-invalid-nested-class-file.jar and jartest.jar?

I don't know. Sounds like it may be problematic.

🤷‍♂️

@sbrannen
Copy link
Member

I'll replace this JAR file and the related com.greetings@1-ea.jar one with a on-the-fly compilation solution.

Thanks! 👍

@sormuras sormuras merged commit 33f7d6e into main Jan 26, 2021
@sormuras sormuras deleted the issues/2500-scan-for-classes-in-modules branch January 26, 2021 16:43
@sormuras sormuras added this to the 5.7.1 milestone May 7, 2021
sormuras added a commit that referenced this pull request 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 pull request 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 pull request 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClasspathScanner.getRootUrisForPackage returns empty list when package name conflicts with module name
3 participants