-
Notifications
You must be signed in to change notification settings - Fork 90
Added JUnit 6 to platform #3399
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
Conversation
|
/request-license-review |
Workflow run (with attached summary files): |
|
Build fails with
|
merks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we should avoid duplicates and unnecessary whitespace changes.
6969627 to
dd17eef
Compare
Let me get the build to work locally, then we can do this. Likely the set of libraries will change. |
|
I had similar problems locally when updating Orbit. Just at a glance I noticed you added 11 things but in Orbit I only added 10 things: Maybe I missed something? Or maybe one of the things you added was bogus (which happened to me too). Maybe this summary helps:
https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation/table.html |
One of the libraries is removed ( |
With the latest changes. Should be coming from here: https://mvnrepository.com/artifact/org.junit.platform/junit-platform-commons/6.0.0
I guess we can replace the call with just the contents of the method... Its nothing special. |
Yes, please. It's even under same EPL. |
I'm not sure what is the suggestion here? We should really not "reimplement" what JUnit already offers, if there is any issue in the usage of the JUnit in client code then it should be reported to JUnit team. If it is in the code we use to run the JUnit platform then it exactly falls under
and we should just include whats required here. |
This is not how I understand "internal" code, but what is your proposal? Do we have a way to avoid the error from my comment with some Or from your POV we ask the JUnit contributors to relax the restriction on the package? |
|
If the code at eclipse call that function its of course possible to replace that call (and remove the import) |
|
It is not transitive, and just used internally in the test bundle. |
|
As I understand it, we use just one method in one place . We do similar things elsewhere already, e.g., org.eclipse.core.internal.utils.Policy.debug(Throwable), so let's not lose sleep over "copying" some trivial method where any idiot programmer would come up with the same method implementation from a "clean room" design. Let's copy the one from org.eclipse.core.internal.utils.Policy.debug(Throwable), adapt it to our needs, and call it done. |
|
OK, the platform build is successful with the current changes. I'm building with: I'll check if anything is broken w.r.t. using JUnit tests. Seems like more is needed to actually add the bundles to the SDK, I only find the JUnit 5 in the build: I don't see what though: https://bugs.eclipse.org/bugs/show_bug.cgi?id=521750 Maybe we are missing dependencies to the new bundles... For JUnit 5 they were added (I think) with: eclipse-jdt/eclipse.jdt@ddf4de8 |
|
/request-license-review |
✔️ All licenses already successfully vetted. Workflow run (with attached summary files): |
|
@merks maybe it will be faster to just ask you, where do we include JUnit now? So that its actually added in the SDK. I see its no longer in the JDT feature: eclipse-jdt/eclipse.jdt@bae402e |
merks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks okay. The whitespace change in the *.target is a nitty annoyance, but one we could overlook.
But if I build from the aggregator repository, the resulting SDK doesn't have JUnit 6. What next changes do we need so that JUnit 6 actually lands in the built product? I want to test what breaks, if anything. |
The repository is configured by the SDK product which directs it to include all dependencies and all sources:
So it could be problematic which bundles actually would end up in the result. I'd be a bit concerned about that. If we plan for JDT to support 5.x and 6.x the dependencies of the bundles themselves should be such that both versions are dragged into the final result (like for junit 4). |
You mean JUnit 5? No. Otherwise we will lose the ability to run tests on Java < 17 |
So we need some plug-in to have a
Which version of JUnit? If you mean JUnit 5, no, we will have it in parallel to JUnit 6 by the looks of it: eclipse-jdt/eclipse.jdt.ui#2530 Since JUnit 6 can't run on Java 8 and Java 11. |
Yes if some things require < 6 and nothing requires >=6 then the transitive closure does need 6.x so won’t include it. So I think this PR is a step in the right direction for making 6.x available for upcoming jdt changes. Note that 6.x does not need to be in the I-build result. The root pom will be published and subsequent builds will have the target platform changes of this commit available and that is sufficient. |
Should I look for the Seems like something we should fix before merging, ensuring |
|
OK, let me check then, maybe its simple. Matches I find for Of these, the So concerning, at least in the aggregator repository: I'll try adding version The other matches seem to be outdated, I'm not sure how their versions are satisfied. We should have |
No difference, If I build on |
|
Personally I would merge and deal with the fallout afterwards. Do you need someone else to push the green button? |
Well the SDK itself doesn't have
@iloveeclipse are we merging this now? From my POV we are not in a rush here, I still need to add dependencies to JUnit 6 and check what happens with the built SDK - if there are bugs due to having 2 bundles with the same symbolic name of most JUnit 5/6 jars. So I'm not sure merging this now will help much with adding JUnit 6 support. But we will find problems with this particular change sooner... |
|
To my thinking finding problems sooner rather than later is better. |
Agree. The change here shouldn't be breaking, and if it is, better to be informed ASAP. Let's merge ans see what happens with tests tomorrow. |
SDK tests seem to be not affected, however looking on these strange failures I fear they could be related: eclipse-platform/eclipse.platform.ui#3382 (comment) |
The version of the org.eclipse.sdk.tests feature was increased twice in this release cycle and therefore isn't properly updated in the release preparation anymore. To unbreak the release preparation it's misaligned version is temporarily ignored/accepted. Caused by - eclipse-platform#3399
| id="org.eclipse.sdk.tests" | ||
| label="%featureName" | ||
| version="4.38.0.qualifier" | ||
| version="4.38.100.qualifier" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version increment should not be necessary (a enforced qualifier-update, if necessary at all, could probably have been sufficient).
In general this version should be aligned with the eclipse release version and the current state breaks the release preparation workflow (respectively the workflow detects the missing version increment and fails).
This is work-around with
The version of the org.eclipse.sdk.tests feature was increased twice in this release cycle and therefore isn't properly updated in the release preparation anymore. To unbreak the release preparation it's misaligned version is temporarily ignored/accepted. Caused by - #3399





No description provided.