Limit access to junit platform engines#2071
Conversation
|
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. |
|
This is meant to be addition to #2070, not replacement? |
No this should replace the former ... but I have some trouble debugging the code, even though remote debugger port is opened Eclipse does not hold on the breakpoints :-\
With the same message or a different one? |
Looks like same:
This happens if breakpoint info is misaligned with the code in question (switching from one branch to other changes breakpoint positions that do not match the methods/lines). |
|
I got it working and see why it has not the desired effect... this needs a bit more sophisticated approach like it did on Tycho... |
laeubi
force-pushed
the
limit_access_to_services
branch
from
6ffdf11 to
83f7494
Compare
|
Last commit works now, however seem to break some JUnit 4 tests |
|
Yes I need to dig into a bit more... but I think it goes into right direction. |
| Bundle-ActivationPolicy: lazy | ||
| Import-Package: org.eclipse.ui.testing;resolution:=optional | ||
| DynamicImport-Package: org.junit.platform.engine | ||
| DynamicImport-Package: org.junit.platform.engine;version="[1.14.0,2.0.0)" |
There was a problem hiding this comment.
I have to admit that I didn't knew that dynamically imported packages can have a version(range) too.
In general I wonder if we should better convert this to an optional package-import?
IIRC dynamic imports still create wires at runtime, but only on demand/dynamically. But as a test-runtime usually doesn't change I'm think if an ordinary (optional) import would be fine.
Rereading my comment from #1047 (comment), I think there was no greater meaning in it.
And we already have an optional requirement on org.eclipse.ui.
There was a problem hiding this comment.
The difference between an optional and a dynamic import is that the optional one is resolved at the time the bundle is resolved, and the dynamic one is resolved at the time you try to access that package (+dynamic supports wildcards)
| * Default implementation used with Java 1.8 | ||
| * TODO provide MR variant using stack walker, currently blocked by JDT bug ... |
There was a problem hiding this comment.
I was about to suggest to use StackWalker. Until I was reminded that this bundle is still at Java-8 and until I read this comment.
Looking forward to see a MR bundle in action. I think this would be a good fit. :)
There was a problem hiding this comment.
Yep, sadly I was hit by this bug now but once this is merged it should work, and would be good if this feature is actually used in platform somewhere, but this is here really not high throughput method I just wanted to make it work first:
|
|
||
| import org.osgi.framework.Bundle; | ||
|
|
||
| final class SPIMapping { |
There was a problem hiding this comment.
I'm so happy that we are way past beyond Java-8 in most bundles nowadays. I was about to suggest to make this a record and again had to remind myself that this bundle is still at Java-8...
Should JuNit 4 support code use "old" branch? We need to fix "known" Junit 5 test failures ASAP, they block us from further work. |
|
I'll look into it now, my current asumption is that actually JUnit5 is somehow always included, now I filter the engines correctly JUnit5 is not happy... one might ask if we not simply can drop junit4 provider at all and execute with JUni5 Vintage anyways... |
|
@akurtakov we do already similar here, the problem is more that if junit4 runtime is selected it complains to not find some JUnit 5 engines... what seems intended ;-) |
|
At laest when I create a new plain Junit 4 test it works... will check now what the test exactly does here.. |
|
It seems only runType test is affected and all other similar tests succeed here... I compared the test output and didn't found a direct problem... need to investigate further. |
|
One option of course would be to accept the test failure for now and merge then further investigate on the remaining problems later on. I could until now not reproduce the problem in a "real" launch, need to check what these tests are doing special |
Give me a moment to check. |
|
For the record, these two tests are failing: |
|
I got it to reproduce the problem, one needs to
Running it as a package, project or single method just works! So it is important to really select the class! |
|
I was able to reproduce the test failure just by executing |
|
I now found the issue and will investigate how to solve this properly... |
Currently the testruntime is exposed to multiple engines and the check for compatibility does not work well in the case of JUnit 5/6 on the classpath. This now - limit the dynamic import to a range of the currently only supported junit 5 - remove the falsely check for compatible engines - Use SPIClassloader to ensure compatible SPI are visible only - For JUnit 4 find the platform launcher as the SPI check
laeubi
force-pushed
the
limit_access_to_services
branch
from
4a07c3c to
d33a411
Compare
|
It should now work for the specific JUnit4 case as well. Once things has stabilized and is merged we can even use MR with a simpler approach for Java 9+ ... I also noticed that |
|
There are two other new failures: https://ci.eclipse.org/pde/job/eclipse.pde/job/PR-2071/6/testReport/ Test Result (2 failures / ±0) |
They look like "smaller" problems as broken JUnit4 support. |
|
I would be fine with that.
|
|
OK, let's merge. |
5 participants
Currently the testruntime is exposed to multiple engines and the check for compatibility does not work well in the case of JUnit 5/6 on the classpath.
This now
For JUnit 6 we need to revise the whole approach here, this is currently only for getting things back in action.
FYI @HannesWell @iloveeclipse @trancexpress