Skip to content

Support scanning for classpath resources #3705

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

Merged

Conversation

mpkorstanje
Copy link
Contributor

@mpkorstanje mpkorstanje commented Feb 25, 2024

Overview

Adds several new methods to ReflectionSupport to find or stream resource objects in the classpath root, package, and modules.

A resource is represented as:

public interface Resource {
	String getName();

	URI getUri();

	default InputStream getInputStream() throws IOException {
		return getUri().toURL().openStream();
	}
}

Fixes: #3630


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


Definition of Done

@mpkorstanje mpkorstanje changed the title Support scanning for class path resources Support scanning for classpath resources Mar 7, 2024
@mpkorstanje mpkorstanje marked this pull request as ready for review March 7, 2024 23:05
mpkorstanje added a commit to mpkorstanje/junit5 that referenced this pull request Mar 8, 2024
As a follow up for junit-team#3630 and junit-team#3705 this adds a
`addResourceContainerSelectorResolver()`
method to `EngineDiscoveryRequestResolver.Builder` analogous to
`addClassContainerSelectorResolver()`.

Points of note:

 * As classpath resources can be selected from packages, the package
   filter should also be applied. To make this possible the base path of
   a resource is rewritten to a package name prior to being filtered.

 * The `ClasspathResourceSelector` now has a `getClasspathResource`
   method. This method will lazily try to load the resource if not was
   not already provided when discovering resources in a container.

 * `selectClasspathResource(Resource)` was added to short circuit the
    need to resolve resources twice. And to make it possible to use
    this method as part of the public API,
    `ReflectionSupport.tryToLoadResource` was also added.
@sbrannen
Copy link
Member

sbrannen commented Mar 9, 2024

Thanks for the PR, @mpkorstanje! 👍

We'll try to find some time to properly review it in the coming weeks (months?).

@mpkorstanje
Copy link
Contributor Author

Cheers! No problem.

@marcphilipp
Copy link
Member

I'll have a look at the coverage later. I'm out of time for now.

That's my bad (again). Rebasing should fix it.

@mpkorstanje
Copy link
Contributor Author

Cheers! Might be good to document the switches for coverage and predictive test selection in the CONTRIBUTING.md.

@marcphilipp
Copy link
Member

Cheers! Might be good to document the switches for coverage and predictive test selection in the CONTRIBUTING.md.

Done in c8beae0. Do you think that's clear enough?

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.

I briefly discussed this PR with the team. We think restricting the resources to remove "duplicates" is too limiting. Instead, a filter that checks that Resource.getUri() equals ClassLoader.getResource could be used with a default implementation provided as a static method in ResourceFilter. WDYT?

@mpkorstanje
Copy link
Contributor Author

Done in c8beae0. Do you think that's clear enough?

Yes. Though I now seem to have some problems generating the coverage report at all now. I'll have to look into this later.

We think restricting the resources to remove "duplicates" is too limiting. Instead, a filter that checks that Resource.getUri() equals ClassLoader.getResource could be used with a default implementation provided as a static method in ResourceFilter. WDYT?

Seems reasonable. I'll have a look at the plumbing.

@marcphilipp
Copy link
Member

@mpkorstanje Do you have time to continue working on this? We'd like to include it in the 5.11 RC1 which is ~3 weeks from now.

@mpkorstanje
Copy link
Contributor Author

I've pushed the plumbing changes.

I'm mostly stuck on #3718. I think there are some subtleties that could maybe propagate backwards into this PR. But I suppose we can keep evolving the API as needed. So we can consider this done.

@marcphilipp
Copy link
Member

Agreed. Let's finish this one and then focus on #3718. I can also take a more active role there if you don't have the time.

@mpkorstanje
Copy link
Contributor Author

That would be appreciated!

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.

@mpkorstanje Do you think it would make sense to add a Predicate<Resource> as a constant that allows filtering out duplicates?

Predicate<Resource> DEFAULT_RESOURCE = resource -> {
	try {
		URL defaultResourceUrl = ClassLoaderUtils.getDefaultClassLoader().getResource(resource.getName());
		return defaultResourceUrl != null && resource.getUri().equals(defaultResourceUrl.toURI());
	}
	catch (URISyntaxException e) {
		throw new JUnitException("Failed to convert URL of resource to URI", e);
	}
};

@mpkorstanje
Copy link
Contributor Author

mpkorstanje commented Jul 12, 2024

@mpkorstanje Do you think it would make sense to add a Predicate as a constant that allows filtering out duplicates?

Not in this iteration I'd think. You're about to go down the same rabbit hole I spend a week in. 😉

A predicate would not work. It would not remove duplicates, but rather remove all shadowed resources. Instead you would need a function that transform a resources into a "canonical" version of the resource. Then you can de-duplicate the canonical resources. In #3718 I wrote a few functions to just that, a good starting point would be the ResourceContainerSelectorResolver from there look at ResourceUtils.getClasspathResource() and ReflectionSupport.tryToGetResource().

You could probably cherry pick those in.

@marcphilipp
Copy link
Member

Not in this iteration I'd think. You're about to go down the same rabbit hole I spend a week in. 😉

Thanks for writing that up! I'll reply on the other PR.

A predicate would not work. It would not remove duplicates, but rather remove all shadowed resources.

Isn't that what you were after? Wouldn't it only allow canonical resources to pass the predicate?

@mpkorstanje
Copy link
Contributor Author

mpkorstanje commented Jul 15, 2024

Isn't that what you were after? Wouldn't it only allow canonical resources to pass the predicate?

That depends. When discovering a class in classpath root a, we currently load a class from classpath root b, if b shadows a.

Do we want the same behaviour for resources? If so we need a function, if not, then a predicate will do.

@marcphilipp
Copy link
Member

I see what you mean now... 🤔

I'm pondering whether it makes sense to include this PR in 5.11 without #3718 and, if so, when we would ship the latter. WDYT?

@mpkorstanje
Copy link
Contributor Author

I think it would make sense to ship this PR as it would unlock #3628. That was the original motivation for splitting these PRs.

@marcphilipp marcphilipp merged commit 8e8268c into junit-team:main Jul 17, 2024
16 checks passed
@marcphilipp
Copy link
Member

@mpkorstanje Thanks for all your hard work! 👍

marcphilipp added a commit that referenced this pull request Oct 29, 2024
As a follow up for #3630 and #3705 this adds a
`addResourceContainerSelectorResolver()`
method to `EngineDiscoveryRequestResolver.Builder` analogous to
`addClassContainerSelectorResolver()`.

Points of note:

 * As classpath resources can be selected from packages, the package
   filter should also be applied. To make this possible the base path of
   a resource is rewritten to a package name prior to being filtered.

 * The `ClasspathResourceSelector` now has a `getClasspathResources`
   method. This method will lazily try to load the resources if not already
   provided when discovering resources in a container.

 * `selectClasspathResource(Resource)` was added to short circuit the
    need to resolve resources twice. And to make it possible to use
    this method as part of the public API,
    `ReflectionSupport.tryToLoadResource` was also added.

---------

Co-authored-by: Marc Philipp <[email protected]>
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.

Support scanning for classpath resources
3 participants