Use bundle versions when adding JUnit to launch #2013
Conversation
Test Results 693 files - 78 693 suites - 78 42m 2s ⏱️ - 49m 17s Results for commit 6bf5bbb. ± Comparison against base commit 3201782. This pull request removes 306 tests.♻️ This comment has been updated with latest results. |
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Show resolved
Hide resolved
|
@HannesWell could you take a look here as well? |
I'll have a look at it tomorrow. Thanks for working on this! |
There was a problem hiding this comment.
Thanks for this work.
I tried to answer at least some of your questions, but I think it needs a bit more general rework to handle versions properly.
Furthermore the class org.eclipse.pde.unittest.junit.launcher.JUnitPluginLaunchConfigurationDelegate, which is more or less a copy needs the similar adjustments too.
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
trancexpress
force-pushed
the
gh2006
branch
2 times, most recently
from
f1b72a9 to
1515b1f
Compare
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
Unfortunately I don't know enough about the code and available APIs to provide a rework.
I've made change there too now. |
|
@HannesWell can you continue reviewing this? It would be nice to continue working on JUnit 6, which is blocked by the issue here. |
|
One oddity with the current code is, I do need to specify 
And so just launching a JUnit plug-in continues to not work when JUnit 5 and 6 are in the platform. The JUnit 5 plug-in test launch requires a |
trancexpress
force-pushed
the
gh2006
branch
2 times, most recently
from
315b26e to
8a1fa96
Compare
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/PluginBlock.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@HannesWell can you continue reviewing this? It would be nice to continue working on JUnit 6, which is blocked by the issue here.
Yes, I'll continue tomorrow with an in-depth review. Sorry for the delay.
For now I just have a few style remarks.
Furthermore it looks like you added more changes to the version-bump commit pushed by the bot. I suggest you squash your changes into one commit and remove the version bump so that the bot can do it's thing again after you (force) pushed the update.
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/launcher/PluginBlock.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/launching/JUnitLaunchConfigurationDelegate.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
trancexpress
force-pushed
the
gh2006
branch
2 times, most recently
from
b6c7fa7 to
b1232ad
Compare
|
Lets wait for test results, if everything is fine we should be able to merge. |
|
OK, I can now reproduce the problem locally too, I had to do some extra change (and use the I also had to take the version bumps by the bot here. Then I see the validation dialog popped up and the tests seem to be waiting forever on it: 
That seems to be part of But its not loaded in the process: I'm not sure what is missing here... The Maybe we have somehow messed up other requirements... I don't see anything unusual there, with standard out prints: I guess I'll have to debug this, I don't know why there is a missing bundle. The bundle description for Both have ID 88. From then on I.e.: |
|
We definitely have more problems here. Trying to run So looks like the replacement code doesn't work... In I do wonder how we have both though?... |
@HannesWell could it be that by finding bundles in the target or in the host, we can get ID collisions like this? See IDs for |
|
Maybe this table helps? https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation/table.html The underlined things (5.14.0 and 1.14.0) would be part of a junit 5 launch, while all the 6.0.0 things would be part of a junit 6 launch: 
Best the two sets not get mixed together in a launch. |
Sorry, this is a lot of input at the moment. I'm currently working through the change as it is to propose refinements and (besides others) to also consider version ranges when looking up dependencies in the host. Maybe this already helps. |
Absolutely! |
What the change computes in terms of versions is fine. Something else breaks, which makes plug-in validation miss one of the bundles. See my previous comments. |
|
I just added a commit with the suggestions from my review, but I didn't had the times to dive into the problem you described. Will do that tomorrow. |
This change adjusts JUnitLaunchConfigurationDelegate, to use bundle versions when adding required JUnit bundles. E.g. when adding JUnit bundles for a JUnit 5 test, JUnit 5 versions are added. This avoids conflicts when both JUnit 5 and JUnit 6 bundles are in the platform, since they share symbolic names. Fixes: eclipse-pde#2006
- Move code a bit around, trying to simplify it a bit - Implement version handling when looking up dependencies in host - Avoid unnecessary code - Add TODOs
With that commit on top of my changes, running the tests locally no longer results in a hang. I've pushed small changes I had to my commit that lead to merge conflicts with your changes, ideally please pull before you continue with your changes @HannesWell . Note that the local build is failing if I don't do the version bumps from one of the commits I've pushed. From my POV better to have that change too, so that local builds don't fail. |
|
Fails on Linux/Windows: E.g.: |
- The JUNIT5_VERSIONS VersionRange should be [1, 6) because [1, 5) excludes Junit 5. - The case for TestKindRegistry.JUNIT5_TEST_KIND_ID should add JUNIT5_JDT_RUNTIME_PLUGIN not JUNIT4_JDT_RUNTIME_PLUGIN. - The search range `new VersionRange(VersionRange.LEFT_OPEN, version, version, VersionRange.RIGHT_OPEN))` is always empty so should use `new VersionRange(VersionRange.LEFT_CLOSED, version, version, VersionRange.RIGHT_CLOSED))`.
I believe I fetched before I pushed to your branch (and also didn't force push). I'm sorry if it caused trouble, I didn't meant to do that.
Personally I often don't build locally but just let the CI do it, and as the bot just recreates the version increment quickly and therefore don't keep them. But I'll keep them here.
Sorry, it was a late for me after very long day and I oversaw some things. Will incorporate your changes, they are all correct. |
There was a problem hiding this comment.
I have now in cooperated the fixes suggested by Ed in
#2029
Furthermore I have removed parts that are actually unnecessary, see below.
For the moment I added these changes as separate commit, but eventually it should be squashed.
...rg.eclipse.pde.launching/src/org/eclipse/pde/internal/launching/JUnitLaunchRequirements.java
Outdated
Show resolved
Hide resolved
| if (replace) { | ||
| String startLevel = null; | ||
| for (IPluginModelBase m : models) { | ||
| startLevel = allModels.remove(m); | ||
| } | ||
| models.clear(); | ||
| allModels.put(model, startLevel); //TODO: isn't this overwritten? Should it be retained or just ignored? | ||
| } |
There was a problem hiding this comment.
Given that we now only add requirements of the o.e.jdt.junitX.runtime here and in general is possible that the same one bundle is present in a runtime in multiple versions, but it's generally recommended to use only one JUnit version at the time and not mix them, I wonder if we really should have this replacement here?
I think it might be better to just add each requirement here, if there isn't an exactly matching version present already.
@trancexpress what do you think?
One also has to consider that we're just considering here what's added to the runtime. During Start-up the OSGi Framework will create all the wiring and among all bundles available in the runtime.
And while generally one should be precise when assembling the runtime and should ideally neither add too many nor too few bundles, just replacing what was deemed required before can also be destructive.
Therefore in this place I would say: Better add a bit more than remove bundles.
The conflicts could not only arise among the JUnit bundles, but also among the other requirements of the o.e.jdt.junitX.runtime bundle.
There was a problem hiding this comment.
I didn't understand this at all. While debugging the startLevel was assignment multiple times; most just null.
There was a problem hiding this comment.
I can't say, I'll need to re-test what I describe here: #2013 (comment)
I'm all for less code, if I can still run tests as usual.
There was a problem hiding this comment.
Note that its best to work on this with both JUnit 5 and 6 in the platform.
There was a problem hiding this comment.
Indeed, when I commented out the remove with the following in the target platform I did have problems launching.
Caused by: java.util.ServiceConfigurationError: org.junit.platform.engine.TestEngine: org.junit.jupiter.engine.JupiterTestEngine not a subtype
at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:593)
I don't understand the startLevel thing though.
There was a problem hiding this comment.
The first non-null would seem more sensible than overwriting the variable without checking the value.
There was a problem hiding this comment.
I guess we can just expect users to change their manifests to restrict JUnit to "[5,6)" for a JUnit 5 launch, leaving the code as is.
Once we have JUnit 6 launches and its default, any new launch configuration will just work, assuming JRE 17+.
Old launched will need either JUnit 6 type change, or the respective plug-in will need version restrictions.
I guess there can be subtile problems without the version restriction anyway, e.g. at compile time. We would have a disruptive change though.
There was a problem hiding this comment.
org.eclipse.swt.SWTException: Failed to execute runnable (java.util.ServiceConfigurationError: org.junit.platform.engine.TestEngine: org.junit.jupiter.engine.JupiterTestEngine not a subtype)
I can reproduce that error when launching a secondary Eclipse just from my 'PDE' workspace, where I only have the PDE repo checked out and get everything else from the latest I-build repo. In the that secondary Eclipse I use the following target definition to have JUnit-5 and 6 to the TP:
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<?pde version="3.8"?>
<target includeMode="feature" name="featureBased">
<locations>
<location includeAllPlatforms = "false" includeConfigurePhase = "true" includeMode="planner" includeSource="true" type="InstallableUnit">
<repository location="https://download.eclipse.org/eclipse/updates/4.38-I-builds/"/>
<repository location="https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation/milestone/latest"/>
<unit id="org.eclipse.sdk.feature.group" />
<unit id="junit-jupiter-api" version="[5,6)"/>
<unit id="junit-jupiter-engine" version="[5,6)"/>
<unit id="junit-jupiter-api" />
<unit id="junit-jupiter-engine" />
</location>
</locations>
</target>
I think the only difference to your state is the absence of the new org.eclipse.jdt.junit6.runtime bundle. But as that's not yet considered by PDE (I assume you plan a follow-up once it's ready?) and therefore I assume it doesn't make a difference?
In the secondary Eclipse I have created a test-plugin with the requirement
Import-Package: org.junit.jupiter.api;version="5.0.0",
which is currently equivalent to
Import-Package: org.junit.jupiter.api;version="[5.0.0,7.0.0)"
(but I think the former is what's encountered more often in practice).
Launching then fails with the described error:
Caused by: java.util.ServiceConfigurationError: org.junit.platform.engine.TestEngine: org.junit.jupiter.engine.JupiterTestEngine not a subtype
The reason is as, as indicated in #2013 (comment), that we now have two versions of the org.junit.jupiter.engine.JupiterTestEngine class in the runtime. One from junit-jupiter-engine version 5 and one from version 6. As the ServiceLoader discovers the classes by name, both are found and version 6 even seems to be preferred (I guess OSGi somehow prefers the greater version although I'm surprised the ServiceLoader works for JUnit without SPI. Maybe they have some more specialties).
The make a long story short: Having especially two versions of junit-jupiter-engine in one test-runtime is indeed a problem.
So that should be prevented. But at the same time we cannot generally prevent duplicated versions.
I guess we can just expect users to change their manifests to restrict JUnit to "[5,6)" for a JUnit 5 launch, leaving the code as is.
Once we have JUnit 6 launches and its default, any new launch configuration will just work, assuming JRE 17+.
Old launched will need either JUnit 6 type change, or the respective plug-in will need version restrictions.
I guess there can be subtile problems without the version restriction anyway, e.g. at compile time. We would have a disruptive change though.
In the context of what I've written above I second each of your statements. Having a JUnit-5 launch with JUnit-6 bundles simple doesn't work out well.
And instead of trying to be smart when launching, my suggestion is to make it clear to a user and help them to get a clean state. Therefore my suggestion is to only add a detection if the runtime contains two different versions of junit-jupiter-engine (or its old name) and if that's the case, to open a dialog explaining the problem to the user and asking to restrict version ranges or to update test launch type.
But we can also do this in a follow-up when JUnit-6 is supported by JDT.
The only thing that has to be investigated is the starvation of the org.eclipse.pde.junit.runtime.tests. I assume they don't have proper version ranges too.
I'll investigate.
There was a problem hiding this comment.
And instead of trying to be smart when launching, my suggestion is to make it clear to a user and help them to get a clean state. Therefore my suggestion is to only add a detection if the runtime contains two different versions of junit-jupiter-engine (or its old name) and if that's the case, to open a dialog explaining the problem to the user and asking to restrict version ranges or to update test launch type.
But we can also do this in a follow-up when JUnit-6 is supported by JDT.
Last but not least PDE should encourage users from the beginning to use suitable version ranges. While this is already the case for page imports, it's not yet done for required bundles. But the latter should be easy to do.
There was a problem hiding this comment.
I think the only difference to your state is the absence of the new org.eclipse.jdt.junit6.runtime bundle. But as that's not yet considered by PDE (I assume you plan a follow-up once it's ready?)
Yes, once this problem is out of the way.
|
I did manual tests with the changes here, I didn't find issues. Would be good to squash the main changes into one commit and leave the version bump commits as is. |
|
It's odd that the following PR's jenkins job ran but this PR's jenkins job doesn't run ever! https://ci.eclipse.org/pde/job/eclipse.pde/view/change-requests/job/PR-2029/ |
Awesome. I just pushed another commit to further simplify/unify the search in the host and to remove the 'replacement' as suggested above. I have to leave now, but I try to continue this evening or tomorrow morning. Regarding your response from: #2013 (comment)
Maybe this was because
Indeed. But I see it in other PRs too. Maybe it only works for Committers atm? |
I just wanted to note that due to the current way we execute the JUnit runtime it does not really follows OSGi semantic when it comes to SPI loading. Because of this I prototype a new approach in Tycho that loads the JUnit Engine through the classloader of the testprobe and then installs a special SPI classloader as the thread-context loader. The important part here is to check if two SPI impls are classloader compatible. This al works quite neat, but also shows for the matter here, that adding to much will result in problems even though OSGi itself can sort it out for its own runtime! |
|
I see a hang when I run tests locally, with the latest batch of changes. I'll check if we can abort the launch on JUnit 5 launch but JUnit 6 used, that might at least avoid hangs... if there is no dialog for the abort. Doesn't help, the problem we have is the same as what I described here: There is an ID collision between |
6 participants
This change adjusts JUnitLaunchConfigurationDelegate, to use bundle versions when adding required JUnit bundles.
E.g. when adding JUnit bundles for a JUnit 5 test, JUnit 5 versions are added.
This avoids conflicts when both JUnit 5 and JUnit 6 bundles are in the platform, since they share symbolic names.
Fixes: #2006