Remove error dialog for JUnit 5 and 6 conflicts #2092
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
@HannesWell, @laeubi please review. From my POV we should check what we know causes problems, which we will do with this PR. No idea why |
Member
In general yes, but as the problem where two versions of a class from the
I think that should also be investigated as well to see if something is wrong somewhere. Maybe the error also doesn't surface anymore because of But for complete JUnit-6 support this probably has to be reworked and in general I think it's better to not mix the junit-jupiter versions in the runtime. So this check could still be valuable (but maybe the wording could be made less severe). |
Contributor
I explicitly developed and tested this with both JUnit 5 + 6 in the runtime, so yes it likely fixed that. The only problem now should arise is if the user has an unbounded JUnit import and it resolves to a higher version (or more formally not matching the selected runtime!), maybe this is better checked here and recorded to give a consistent error message see below.
I'd like to highlight that the test has only a dependency on JUnit 5, I have selected JUnit 5 runtime to execute and I have only selected the test bundle. So from that I cannot see how JUnit 6 should ever be included (whether or not it causes issues!) so I think this has to be investigated and fixed. If it is due to any dependency in the chain we should fix it and the message MUST clearly state what bundle is pulling in the JUnit 6, otherwise it is completely impossible for the user to act on this warning and it is better not shown at all. |
Contributor
Author
|
Requirements like these are not a problem for running a test: These are a problem: The (new) error is:
I'll try to get the bundle that depends on JUnit 6 here, maybe its simple.
I'm not sure the new error helps to understand what the problem is, the dialog at least explains its mixed JUnit versions. I guess if the error mentions mixed JUnit 5 and 6 as a problem, we can drop the dialog? Lets see if we can include the bundle that depends on the unexpected JUnit version, if we can't we can drop the dialog altogether and mention mixing JUnit versions in the new error message. |
Contributor
Author
|
It seems difficult to analyze the problem that @laeubi ran into. There is no bundle that requires For package imports, we'll have to know what packages are exported by the bundles we test for. |
Contributor
Author
|
Interestingly this Same for: Just this is logged: |
Contributor
I have already explained that case, the test is using JUnit 6 so JUnit 5 runtime will not see the annotation so no error and no tests executed... |
Contributor
Author
|
I've updated the PR to report the bundle that requires/imports Mixing My intention for #2045 was to warn the user if they require |
trancexpress
commented
Nov 4, 2025
| map.put("org.junit.jupiter.api", BuildPathSupport.JUNIT_JUPITER_API); //$NON-NLS-1$ | ||
| map.put("org.junit.jupiter.engine", BuildPathSupport.JUNIT_JUPITER_ENGINE); //$NON-NLS-1$ | ||
| map.put("org.junit.platofrm.engine", BuildPathSupport.JUNIT_PLATFORM_ENGINE); //$NON-NLS-1$ | ||
| map.put(BuildPathSupport.JUNIT_JUPITER_API, BuildPathSupport.JUNIT_JUPITER_API); |
Contributor
Author
There was a problem hiding this comment.
Since we want to report the bundle that depends on JUnit 6, we can no longer just look for junit-jupiter-engine. Since we find that in the dependencies of junit-jupiter-api, which we must skip for the report to make sense - we are not reporting the whole chain of dependencies.
The report then looks like this:
trancexpress
commented
Nov 4, 2025
| * The dependency from junit-jupiter-api 5.x to junit-jupiter-engine 5.x doesn't interfere here. | ||
| */ | ||
| if (junitJupiterEngine5 != null && junitPlatformEngine6 != null) { | ||
| String message = NLS.bind(PDEMessages.JUnitLaunchConfiguration_error_JUnitLaunchAndRuntimeMissmatch, 5, 6, junitPlatformEngine6.fromBundle, junitPlatformEngine6.toBundle); |
Contributor
Author
There was a problem hiding this comment.
I'm not sure its worth to report this. I'm sure there are other errors that we are not aware of, this must just be one of many.
Member
Good finding. The imports of ant are in fact optional.
I appreciate your effort to provide more information to the user and it's indeed difficult to find the cause for the inclusion. But for this dialog it seems quite heavy weight and I actually prefer to keep this dialog simple. However, as it's a general problem to find out why a particular bundle is part of the runtime, I'd prefer to provide a general solution to that problem. This could then be used to analyze problems like this. For today I don't have more time, but I'll try my best to follow-up on this tomorrow. |
Contributor
Author
|
OK, @laeubi wants to report which bundle requires a "bad" JUnit version, which I think is useful. @HannesWell wants this but not in the validation here. So should we remove the validation until its clear what is wanted in PDE and how to achieve it? In its current form, the validation its reporting a problem that doesn't exist. And the problem it does report, I guess we can mention in |
Contributor
Author
Sorry, I doubt I'll find time for that. I assume the problem can already be analyzed by |
trancexpress
force-pushed
the
gh2045
branch
2 times, most recently
from
abe1307 to
b83ba17
Compare
Contributor
|
@merks should we fix the ant bundle (I think its from orbit?) @trancexpress @HannesWell I would step back and maybe think about a different approach! When we launch a run, there is already a validation step that results in this "Missing requirements" dialog if something went wrong. When we do this we create a state and resolve it and then check for errors to show in this dialog. I think this would be much more suitable than what we can get from the current dialog as it already has ways to report errors and problems. So as a rough idea I would do the following:
That way it becomes a real validation warning, and the user can still choose to ignore that one. This will then also cover the case that the test-bundle itself has not proper imports. To maybe get some of the boilerplate code easier I fired up a session here, maybe it can act as an inspiration (will take some minutes to show up results): |
Copilot AI
added a commit
to laeubi/eclipse.pde
that referenced
this pull request
Nov 5, 2025
- Modified JUnitLaunchValidationOperation to call parent's run() first to resolve the state - Added checkForJUnit6InJUnit5Launch() to detect JUnit 6+ bundles in JUnit 5 launches - Added findBundleRequiringJUnit6() to identify which bundle is pulling in JUnit 6 - Updated error reporting to include the bundle causing the conflict - Added new message key JUnitLaunchConfiguration_error_JUnitLaunchAndRuntimeMissmatch_withRequiringBundle - Merged parent validation errors with JUnit-specific errors in getInput() and hasErrors() This implements the validation as described in PR eclipse-pde#2092 comment #3489590138 Co-authored-by: laeubi <[email protected]>
Contributor
|
I really don't know that it's true that ant cannot work with an arbitrary version of junit, so going down a path that effectively requires the entire ecosystem to add upper bounds (that may not even be appropriate or needed) is probably a long and winding road that will never reach an end. What appears to be the case is that a combination doesn't work and the logic is probably best implemented to avoid that. For ant those package imports are optional: 
I don't know if optional imports are even needed; I see no uses for the in the BND generated bundle. In any case, why an optional bundle import should force a "too high" version of some bundle to be resolved seems mysterious to me. |
Contributor
A user can choose in the run config to include optional requirements and often it is a good thing because many "optionals" are not that optional in reality (remember fragments are also "optional") as they are claimed. And whatever ant requires here I highly doubt it is JUnit 6 so restricting the optional import to version 5 at maximum would not harm nor seems something we should bug the user with. For me any unrestricted version range is simply an error. So including JUnit 6 is here correct and PDE has no way to know that this is not needed for this bundle. Nevertheless PDE can probably do better and already do in many places (e.g as you can see it actually works to launch here), the question is more how much effort should be put into helping people keeping bad metadata (e.g. missing version ranges on imports)... |
Contributor
|
The world is a bad place: 
MANIFEST.MFFor all we know:
If we need to sovle world hunger first, we'll need to be very very patient... |
Contributor
|
For all we know:
Why do we then include ant-junitXXXX bundles in the bundle classpath?
Who has ever tested this?
of what? I assume the root cause here is that the ant jars have no osgi manifest so bndlib can not knwo whats the version, but of course we can list packages + version ranges for the dependencies that ant uses in its junit parts, that the responsibility of those who wrap a jar and why I always argue to not wrap jars as it puts the burden of checking whats needed to you and often its not easy to guess. |
trancexpress
changed the title
Contributor
Author
|
Anyway, lets remove the new dialog. We can then take some time to find the right approach. |
Contributor
Author
|
Did something break for the build jobs? |
Member
…e in RemotePluginTestRunner Fixes: eclipse-pde#2045
Contributor
Contributor
Author
|
Lets merge @iloveeclipse . |
iloveeclipse
approved these changes
Nov 5, 2025
This was referenced Nov 5, 2025
Member
From my experience I can only strongly recommend to not include optional dependencies by default, because of cases like this. Therefore I think optional dependencies should be ignored by default for new JUnit tests by default.
I haven't looked into the details, but I assume it's for the junitlauncher ANT task and friends. And since we probably don't use this in cases like this, I assume the JUnit version actually doesn't matter as that part is not used here.
Too bad for the work to create it, but with the current situation I think that's indeed the best way forward. Thanks for the update. |
Contributor
For this particular case everything is optional and I really doubt ant is prepared for this. So it does not really "solve" anything and I strongly believe we should fix bad meta-data instead of assume anything is "fixed" by not including optional things because in this case something else is guaranteed to fail here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #2045