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

[tester] Resolver to optionally resolve tester dependencies #3795

Closed
kriegfrj opened this issue Feb 20, 2020 · 12 comments
Closed

[tester] Resolver to optionally resolve tester dependencies #3795

kriegfrj opened this issue Feb 20, 2020 · 12 comments
Assignees
Labels
abeyance need of contributor [requires is:closed] stale Stale issue or pull request

Comments

@kriegfrj
Copy link
Contributor

When you run a test using -tester, it dynamically adds the specified tester bundle to the end of -runbundles. However, it doesn't add the tester's dependencies.

For biz.aQute.tester, it only has one dependency and normally that dependency will also be required by the test bundle itself, so the resolver will already have included it.

However, with biz.aQute.tester.junit-platform, it depends on the junit-platform bundles which are ordinarily not dependencies of individual test bundles. So you need to manually add all of these bundles to -runrequires.

It would be nice if there was a "tester-resolve" mode/command, that added -tester to -runrequires prior to running the resolution, so that the resolver will also find and add the tester's dependencies.

@bjhargrave
Copy link
Member

It seems like we should just have biz.aQute.tester.junit-platform declare the necessary requirements for the resolver to bring in the necessary dependencies. I don't think we want to define some option on -tester to define additional dependencies when we can define them in the biz.aQute.tester.junit-platform bundle's manifest.

@kriegfrj
Copy link
Contributor Author

Sorry, I clearly didn't explain myself properly.

The dependencies are already properly declared in biz.aQute.tester.junit-platform's manifest.

The problem is that the resolver doesn't look at -tester's manifest when calculating the set of runbundles. You have to explicitly add the tester to runrequires, which means that the tester also ends up explicitly in runbundles.

Perhaps this is an acceptable solution. However, I recall it was suggested elsewhere (by yourself and @rotty3000 ) that ideally this ought not to be necessary. The only real alternative is to have a flag to automatically and implicitly add the tester bundle to runrequires at resolution time so that the resolver uses biz.aQute.tester.junit-platform's manifest too.

@rotty3000
Copy link
Contributor

The fact that -tester is a special case is what is really weird. The resolver really should simply add that to the initial requirements. That would solve the issue I believe. Right now it's unceremoneously jammed into the resulting bundles fully anticipating that everything will be hunky-dory but that is only true if it has zero dependencies beyond the framework.

@bjhargrave
Copy link
Member

We would need some code for this in the Bndrun resolve logic. We would need to add -tester to the runrequire list and then remove it from the generated runbundles since the launcher will already add it to the runbundles and we don't want it in their twice.

Relatedly, we only use -tester (and its dependencies if we are launching for testing, right? Couldn't one also launch as a normal run in which case tester is not added and it dependencies should probably not be added either. So maybe we need to treat -tester as a separate runrequire root which generates its own runbundles deps list?

@kriegfrj
Copy link
Contributor Author

We would need some code for this in the Bndrun resolve logic. We would need to add -tester to the runrequire list and then remove it from the generated runbundles since the launcher will already add it to the runbundles and we don't want it in their twice.

Yes, although having it in there twice is not really an issue since you implemented #3533.

In this case, instead of having a new bnd testresolve command, we simply have the existing bnd resolve command look for the presence of the -tester directive and implicitly add it to -runrequires before resolution starts.

Relatedly, we only use -tester (and its dependencies if we are launching for testing, right? Couldn't one also launch as a normal run in which case tester is not added and it dependencies should probably not be added either. So maybe we need to treat -tester as a separate runrequire root which generates its own runbundles deps list?

I've been thinking about this and in my opinion the answer is yes, but I think it would be cleaner to maintain sepearate runbundles deps lists by having separate bndrun files - one for regular launches, and one for testing. My reasoning follows:

In the most generic case, test launches may want to change a whole bunch of different -runXXX settings (not just -runbundles). They may also want to remove implementation bundles that are present in the regular launch and replace them with mock implementations at test time. Rather than come up with a full set of duplicate -testXXX instructions to parallel the -runXXX settings, it would be better to re-use the -runXXX instructions and put them in a separate file. Duplication between launch and test files can be minimised through the use of includes.

Thoughts?

@kriegfrj
Copy link
Contributor Author

The fact that -tester is a special case is what is really weird. The resolver really should simply add that to the initial requirements. That would solve the issue I believe. Right now it's unceremoneously jammed into the resulting bundles fully anticipating that everything will be hunky-dory but that is only true if it has zero dependencies beyond the framework.

Actually, what's saved us so far is not so much that the tester has zero dependencies beyond the framework, the fact that JUnit 3/4 itself was not modular - both the runner and the API are in the same bundle. Because the test bundle will always requires the API, JUnit is always pulled in by the resolver as a dependency if you have your test bundle in -runrequires (as you normally would).

Since JUnit 5 has been modularized, the API and runner are now in separate bundles. The test still depends on the API bundles, so the resolver brings those in, but it no longer brings in the runner bundles (ie, the dependencies of the tester bundle).

@bjhargrave
Copy link
Member

The test still depends on the API bundles, so the resolver brings those in, but it no longer brings in the runner bundles (ie, the dependencies of the tester bundle).

Perhaps an answer here is for an additional fix to the JUnit bundes to add osgi.implementation capabilities (to the runner bundles) and osgi.implementation requirements (to the API bundles). Then the resolver will work properly. Thoughts @rotty3000?

@kriegfrj
Copy link
Contributor Author

kriegfrj commented Mar 2, 2020

This is essentially what the suggestion is in junit-team/junit5#2100. I think that this is a worthy additional suggestion, but it is not a full solution for the problem I outlined in the OP.

There are two types dependency trees to worry about: one rooted in the tester bundle, and then one rooted at each of the test bundles.

The solution you are talking about targets the second type: ensuring that an appropriate TestEngine capability is available at runtime for each test bundle.

The dependency issue I was trying to target in this issue refers to the first type.

I guess that we could also define a capability namespace for the testers, and then the resolver will do its thing, but we'll also end up with the tester bundle in -runbundles.

@bjhargrave
Copy link
Member

My point was that biz.aQute.tester.junit-platform can have requirements to drag in the necessary junit bundles (once they are updated to provide capabilities).

@kriegfrj
Copy link
Contributor Author

kriegfrj commented Mar 5, 2020

Yes, but to clarify, those dependencies should not be expressed directly as dependencies of biz.aQute.tester.junit-platform, rather they should be expressed as dependencies of the test bundles as they are an implementation detail of the test bundle.

My intended scope for this issue was to ensure that the -tester's direct dependencies were considered during resolution. What you're proposing is (I think) more pertinent to junit-team/junit5#2100.

@bjhargrave bjhargrave added this to the Abeyance milestone Apr 3, 2020
@bjhargrave bjhargrave removed this from the Abeyance milestone Jun 9, 2020
@bjhargrave bjhargrave added the abeyance need of contributor [requires is:closed] label Jun 26, 2020
kriegfrj added a commit to kriegfrj/bnd that referenced this issue Feb 5, 2021
Looks for the -tester directive and if present makes sure that the
chosen tester's requirements are added to the list of initial
requirements.

Fixes bndtools#3795

Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. If you feel this is something you could contribute, please have a look at our Contributor Guide. Thank you for your contribution.

@kriegfrj
Copy link
Contributor Author

kriegfrj commented Jul 1, 2021

I think that this issue can be closed thanks to @rotty3000 's fix in junit-team/junit5#2654 (which will ensure that all the required JUnit dependencies are pulled in as dependencies of the test bundle).

I think that, in theory, this issue still has merit - if you know that (eg) biz.aQute.tester.junit-platform is potentially going to be dynamically added to the runbundles at runtime, then it makes sense for the resolver to also make sure that its dependencies are added too. However, with the above fix, in the normal use cases, all of the necessary dependencies will be included anyway, so there isn't a great need for this feature at this point in time (generally speaking, bundles in the embedded repo should be sparing in the dependencies that they declare anyway).

@kriegfrj kriegfrj closed this as completed Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abeyance need of contributor [requires is:closed] stale Stale issue or pull request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants