-
-
Notifications
You must be signed in to change notification settings - Fork 303
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
bnd-testing-maven-plugin:7.0.0-SNAPSHOT:testing goal fails for certain tests #5568
Comments
Could you diagnose the exact reason why the resource is not found? Is it not in the bundle? |
@wborn any news about what the problem is? |
If I look at the code it uses the I see it is using a I also tried to get some more Equinox debug output by adding a VM arg
However it doesn't print any addtional debug output. I also tried setting a breakpoint in Eclipse and inspect the classloader that way, however I wasn't able to debug that way due to an Exception when launching the test in Eclipse 2022-03: Maybe you know a way to troubleshoot this? |
Hmm. It depends very much on the setup? The error you get is that the biz.aQute.tester jar is not found on the |
Ping? |
Sorry I currently don't have a lot of time to further debug this. You probably know how that goes as fellow OSS maintainer. 😉 |
Any idea when we can do something? |
When you want to work on this, please reopen. Notice that we will release in the coming month |
I see there is even a commit in that PR to change class loaders: 859636b |
Perhaps the class loader change is no longer necessary for JUnit because there were also some fixes regarding this in JUnit, see: WDYT @kriegfrj? |
Sorry @wborn, I'm late to the party (again) and I've only just seen your message. I can't comment with certainty as I have not had (and likely will not have) time for a deep-dive on this. However, I suspect that the problem that @bjhargrave was trying to solve with 859636b related to the loading of custom plugins - JUnit 5 (and in particular the Jupiter engine) uses the serviceloader as one mechanism to load custom plugins. This is different to the issue mentioned in https://bnd.discourse.group/t/classnotfoundexception-when-using-junit-5-parametrizedtest-with-methodsource-on-java-11-with-bnd/314, which is not about loading plugins but about loading test classes and their associated annotations. I don't think that junit-team/junit5#3297 will solve this particular problem. I'm also not sure how the TCCL classloader trick here will solve any serviceloader issue with JUnit. The way it is written, it will mean that Also with regard to the screenshot that you have of the |
I don't recall the specific reason for the change to set the TCCL but I will assume it was necessary for update of the JUnit 5 version when updating to Eclipse 4.25. I think the issue was that the updated JUnit 5 version did not find the ServiceLoader resource for the bnd test engine so that it would be loaded. It is possible the fix will need to set the TCCL to a "composite" class loader which will look in both the bundle's class loader (to find bnd test engine) and the original TCCL (to handle the case identified here). |
@bjhargrave since you reopened it, are you planning to work on it? |
We use CompositeClassLoader to composite the tester bundle's class loader with the original TCCL when loading and executing tests. This will provide visibility classes and resource from the original TCCL. Fixes bndtools#5568 Signed-off-by: BJ Hargrave <[email protected]>
We use CompositeClassLoader to composite the tester bundle's class loader with the original TCCL when loading and executing tests. This will provide visibility classes and resource from the original TCCL. Fixes bndtools#5568 Signed-off-by: BJ Hargrave <[email protected]>
Hi team, Sorry I'm late to the party again, but I've had a deeper dive on this. I went to the trouble of going back and reverting the TCCL hack to try and understand why we introduced it in the first place. It seems that the reason the classloader switch was brought in was to make the tests pass once we upped the minimum dependency to Eclipse 4.25. What happened is that the later version of Eclipse brought a later version of JUnit-Platform (1.9.0), which was using the I think that this is primarily a problem created by the use of Launchpad and the presence of two different junit-platform-launcher bundles. In my own local build, I reverted 859636b and although the Launchpad tests fail, when I deploy it into the test instance of Eclipse 4.25 and put it through its paces in there everything runs fine. So I think that the TCCL trick will be mostly not be needed (if at all) in the real world - the only case would be in a framework with multiple versions of JUnit Platform installed, but this seems an unlikely possibility. But if we are to keep it for those remote possibilities (as well as keeping the Launchpad tests happy), I propose the following changes:
I think that the second point in particular will solve the classloader-related issues that you had in your tests. @wborn, if you get a chance, can you try out #5744 and see if that fixes your problem too? |
How does that allow ServiceLoader to find the bnd engine?
OK, but I am not sure what this buys you. Is there a real reason to limit the scope? |
bnd/biz.aQute.tester.junit-platform/src/aQute/tester/junit/platform/Activator.java Lines 135 to 138 in 535466e
As JUnit itself is gradually becoming more classloader-sensitive, such gymnastics may not be needed at some point in the future. The current TCCL hack in #5744 may not be a complete solution either.
This was based on the observation that @wborn's tests work without the TCCL switch, and when I looked at his failing stack traces I noted 1. that they were clearly classloader-related issues, and 2. that they originated during the execution phase of the test lifecycle (not during launcher creation). Thus I reasoned that by ensuring that original classloader was restored during test execution it would make his tests start working again. But I wanted to retain the TCCL switch during the launcher creation phase so as not to undo your original fix in 859636b. Reducing the scope of the TCCL seemed like the easiest way to achieve both at the same time. |
Thank you so much for obliging with your feedback about whether our fixes work. It might seem like a small thing, but it's invaluable. I'm glad we were able to sort it out for you (eventually!) |
This is an alternative fix for #5568 which supersedes commit bef0458 in PR #5743. We minimise the scope of the TCCL change so that it does not interfere with the (otherwise working) default classloader mechanisms during test execution. Fixes #5568. Signed-off-by: Fr Jeremy Krieg <[email protected]>
I'm very happy to see it fixed as these class loader issues are always very tricky. I hope it will give others also a seamless migration to Bnd 7! 😀 Your write up on how it all works is also very helpful for future reference whenever this subject needs to be revisited. 👍 |
I gave the 7.0.0-SNAPSHOT builds a test on openhab-core but it seems some tests now fail which work fine with 6.4.0.
It looks like there are some classloader issues:
Logging of another test having issues:
It can be reproduced using my openhab-core bnd-7 branch as follows:
Tested using Java 17 (Temurin) on macOS 12, Ubuntu 22.04 and Windows 2022.
See also this GHA build: https://github.com/wborn/openhab-core/actions/runs/4270878268/jobs/7434962625
The text was updated successfully, but these errors were encountered: