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

Add support for specify test requirements of a bundle #914

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Nov 12, 2023

Currently there is only limited support for "test only" dependencies of a bundle, the usual way is to add an (optional) import/require bundle to the test but this is not very suitable when one wants to have tests in the bundle itself, also some requirements can't be expressed as a package/bundle without binding to a particular implementation and even if, these are still added as regular dependencies and not with test scope.

This ads a way to specify a list of 'test.requirements' in the build.properties to account for this.

Will require

Copy link

github-actions bot commented Nov 12, 2023

Test Results

     270 files  ±0       270 suites  ±0   54m 43s ⏱️ + 7m 54s
  3 327 tests ±0    3 297 ✔️ ±0  30 💤 ±0  0 ±0 
10 278 runs  ±0  10 188 ✔️ ±0  90 💤 ±0  0 ±0 

Results for commit aee286a. ± Comparison against base commit a31752b.

♻️ This comment has been updated with latest results.

@laeubi laeubi force-pushed the support_test_dependencies branch 4 times, most recently from 43df917 to a77bb15 Compare November 13, 2023 12:36
@laeubi laeubi marked this pull request as ready for review November 14, 2023 05:57
@HannesWell
Copy link
Member

Thanks for this. I'll review it tomorrow.

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this forward, this will definitively help to reduce the need for Test-Fragments.

But what I think is totally missing is support for launching the Plugin projects with embedded tests as JUnit-Plugin test, isn't it?
In that case we need to adjust the manifest and add the test-requirements as well.
The launched Equinox runtime would then need to be pointed to the modified manifest, not sure if a dev-classpath entry is sufficient?

Content-Assist would also be nice to have.

And if I read the code correct, one can only add bundles?
It would be convenient if features could be added as well. I have seen a few test-projects in the Eclipse-SDK that require entire features.
The IU syntax could be used for that (i.e. if a requirement ends with .feature.group it could at least be attempted to get the corresponding feature).
Nevertheless this could also be done in a subsequent PR.

Comment on lines +1 to +11
package org.eclipse.pde.internal.core.bnd;

import aQute.bnd.osgi.PluginsContainer;

public class PDEPluginsContainer extends PluginsContainer {

public PDEPluginsContainer() {
add(TargetRepository.getTargetRepository());
}

}
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?
I cannot see any reference to it, neither in code nor in metadata?

Comment on lines +429 to +430
// Add any library entries contributed via classpath contributor
// extension (Bug 363733)
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this and the other unrelated white-space changes.

Repository repository = TargetRepository.getTargetRepository();
Map<Requirement, Collection<Capability>> providers = repository.findProviders(requirements);
List<BundleCap> testBundles = providers.values().stream().flatMap(Collection::stream)
.map(cap -> cap.getResource()).distinct().map(res -> ResourceUtils.getBundleCapability(res))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.map(cap -> cap.getResource()).distinct().map(res -> ResourceUtils.getBundleCapability(res))
.map(Capability::getResource).distinct().map(ResourceUtils::getBundleCapability)

Currently there is only limited support for "test only" dependencies of
a bundle, the usual way is to add an (optional) import/require bundle to
the test but this is not very suitable when one wants to have tests in
the bundle itself, also some requirements can't be expressed as a
package/bundle without binding to a particular implementation and even
if, these are still added as regular dependencies and not with test
scope.

This ads a way to specify a list of 'test.requirements' in the
build.properties to account for this.
@laeubi
Copy link
Contributor Author

laeubi commented Dec 1, 2023

This only covers the definition of requirements everything else has to be supplied on top of it.

@laeubi
Copy link
Contributor Author

laeubi commented Dec 2, 2023

It would be convenient if features could be added as well. I have seen a few test-projects in the Eclipse-SDK that require entire features.

Features would already work (if we can have them defined e.g. with something like bndtools/bnd#5805) because a feature is nothing we want/need to fetch here, a feature just require other things and then the bundle set will just grow with the additional requirements so in the end it all is bundles only, but this is definitely something we need more support in PDE with additional PRs.

@HannesWell
Copy link
Member

It would be convenient if features could be added as well. I have seen a few test-projects in the Eclipse-SDK that require entire features.

Features would already work (if we can have them defined e.g. with something like bndtools/bnd#5805) because a feature is nothing we want/need to fetch here, a feature just require other things and then the bundle set will just grow with the additional requirements so in the end it all is bundles only, but this is definitely something we need more support in PDE with additional PRs.

Yes of course, in the end a feature is only a group of bundles and one could simply replace it with all (transitively) contained bundles.
I don't think we have to wait for BND to have that. For launches the bundle collecting algorithm is already implemented in BundleLauncherHelper. If possible we could either use that directly or create some more abstract shared code for that.

@laeubi
Copy link
Contributor Author

laeubi commented Dec 3, 2023

The problem is not collecting features, this already would work, but features are currently not represented as resources and therefore can't be required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants