bndtools / bnd

Bnd/Bndtools. Tooling to build OSGi bundles including Eclipse, Maven, and Gradle plugins.
https://bndtools.org
Other
530 stars 305 forks source link

JUnit 5 Support? #2168

Closed jcommand closed 4 years ago

jcommand commented 6 years ago

JUnit 5 Support

What is the planning for JUnit 5 support? In the TODO list of the bnd 4.0 project I couldn't find anything about it. Can't tell if there is a need for most developers. I'm in the middle of something.

  1. the tests are easier to implement
  2. i'm implementing many tests right now
  3. the old 3 and 4 JUnit tests would still work.
  4. the limitation of @RunWith/TestRunner hinders my work
  5. extensions or even a test engine would suit me very well.

I can only assume that some other developers have similar topics.

Is it enough to adapt to the bnd Launcher or is it more difficult and are there many old JUnit specific implementations?

According to the documentation, it's "-tester my. tester. bundle" possible to register an implementation of aQute.bnd.build.ProjectTester. The abstract method public abstract int test () throws Exception; is it intended for test execution?

It doesn't seem to me that the JUnit 5 Platform Launcher and Platform Engine API can be handled in the aQute. bnd. build. ProjectTester#test () method.

The JUnit 5 platform is divided into descover of the test and separate execution of the test. Of course it is possible to do both in the test method. However, you will lose the IDE integration. I'd hate to see that happen.

Best regards. Frank

bjhargrave commented 6 years ago

I was thinking about adding support for Junit 5 and also using it in the Bnd tests but had not got there yet.

bjhargrave commented 6 years ago

Any one want to make a PR for this?

kriegfrj commented 5 years ago

Ok, I've had a fair bit of a look at this (in conjunction with #2958), and this is what I have come up with:

There are three possible solutions:

  1. Use JUnit 5 runner for all tests. Courtesy of the JUnit vintage engine, it should be possible to do it this way and still support tests of all flavours.
  2. Configure which runner to use (like what Eclipse's JUnit launcher does).
    1. Custom error reporter/adapters for each runner framework, embedded in the bundle itself.
    2. Custom error reporter/adapter for each runner framework, each implemented as a separate bundle or fragment.
    3. Use the high-level Eclipse JUnit API (which already has separate custom bundles for each test runner framework).

The advantage of the first is simplicity of implementation - it should be fairly simple to implement. The disadvantage is that it is not the most backward-compatible option, and it forces users of bnd.aQute.junit to use the JUnit 5 runner.

The advantage of the second is maximum backward compatibility and flexibility. The disadvantage generally is additional complexity. The three ways to implement it that I can see that each have the following pros/cons: i. Pros: no separate bundles; Cons: biz.aQute.junit bundle is unnecessarily large (may be an issue on embedded systems). ii. Pros: minimises memory footprint; Cons: requires you to manage at least two bundles instead of one. iii. Pros: biz.aQute.junit doesn't have to re-invent the wheel; Cons: pulls in a few Eclipse dependencies (may be an issue on embedded, depending on how many come in).

I know that @pkriens is sensitive to making these bundles run on embedded systems (and hence to their size/complexity and need to minimise third-party dependencies). Hence, I'm assuming that option 1 will be the best - I personally consider the lack of backward compatibility to be a low risk factor. But I wanted to check with the "brains trusts" for some feedback before investing too much time in it.

pkriens commented 5 years ago

First, the primary test bundle nowadays is biz.aQute.tester, not the biz.aQute.junit. I changed to tester because it then can load the JUnit library as a bundle. I think biz.aQute.junit is more or less history? biz.aQute.tester is based on biz.aQute.junit so I could see we need to support new features but the idea to include JUnit was brain damaged with hindsight. When I use biz.aQute.tester and include JUnit 5, what is missing?

kriegfrj commented 5 years ago

First, the primary test bundle nowadays is biz.aQute.tester, not the biz.aQute.junit. I changed to tester because it then can load the JUnit library as a bundle. I think biz.aQute.junit is more or less history? biz.aQute.tester is based on biz.aQute.junit so I could see we need to support new features but the idea to include JUnit was brain damaged with hindsight.

Yes, I understood that. But as biz.aQute.tester directly embeds the aQute packages from biz.aQute.junit, I either have to make these changes in biz.aQute.junit, or else fork the source of biz.aQute.junit into biz.aQute.tester and work on it there. Your call on which of these I do.

When I use biz.aQute.tester and include JUnit 5, what is missing?

Two, possibly three main parts (all of the relevant code is in aQute.junit.Activator):

  1. The code currently instantiates and runs the JUnit 3 run framework. It handles JUnit 4 test cases by wrapping them in a JUnit4Adapter instance. This needs to be updated to be able to instantiate & run the JUnit 5 framework instead.
  2. The code registers itself as two listeners (three if Eclipse IDE support is needed) on the JUnit runner framework so that the errors can be reported (to console, XML, and Eclipse if registered). In the current implementation, the listener implements junit.framework.TestListener (JUnit 3). This would need to be updated to use a listener compatible with the framework being run (eg, in JUnit 5, it would be org.junit.platform.launcher.TestExecutionListener).
  3. Possibly, the Eclipse listener (which directly implements Eclipse's JUnit over-the-wire socket protocol) would need to be updated to handle the updated syntax introduced to support later versions of JUnit.

Your thoughts on whether to natively support all three JUnit runners, or simply to support only JUnit 5 runner (+JUnit 5 vintage runner for backward compatibility)? I'd be very happy to only go with the latter (the former requires implementing the above three steps for all three types of JUnit runner), but be aware that there is a very small chance some older tests might not work if we take this path.

pkriens commented 5 years ago

So the key point is that we break existing biz.aQute.tester installations that have JUnit 4 on their classpath if we move to JUnit 5 in the Activator. If we could gracefully handle that case then I think we can upgrade biz.aQute.junit to JUnit 5 as long as it can fallback to the current mode of operation. This is likely awkward in biz.aQute.junit since it carries JUnit 5 then. So we could move the older code for running on JUnit 4 to biz.aQute.tester? I.e. an Activator in biz.aQute.tester can detect if it is linked to JUnit 4 or 5 and call the old Activator for 4 and the new Activator from biz.aQute.junit when JUnit 5 is linked. I would not optimize or refactor the code too much. Just copy the existing Activator and any classes that require unique JUnit 5 classes. The JUnit 4 code is just there to die one day. Whenever there is a bug, and we had extremely few so far, we can tell people to move to JUnit 5. So although we duplicate the source, I do not see a maintenance issue.

Would that work you think?

kriegfrj commented 5 years ago

Actually, JUnit 4 and 5 can coexist happily on the classpath. The backward compatibility I was concerned about is that the JUnit 3 and 4 test classes would be running under the JUnit 5 runner (aka the "vintage" engine). I'm sure the vintage engine did a pretty good job of emulating junit 3/4, but there's always a risk some tests will break if it's not using the same runner.

Re: your proposed solution: perhaps we could do it the other way around. Given that biz.aQute.junit is pretty much deprecated already and only kept for backwards compatibility, leave it as-is. We can then add JUnit 5 support to biz.aQute.tester and have it fallback to biz.aQute.junit implementation for legacy support if junit 5 is not available. Or perhaps simply leave both alone, and have a dedicated biz.aQute.tester.junit5 bundle, since we are already going down the path of duplicating the code anyway. Then the resolver can handle the dependency resolution instead of having to use dynamic closing tricks to optionally run different code depending on the availability of other junit bundles. Thoughts?

bjhargrave commented 5 years ago

Given that biz.aQute.junit is pretty much deprecated already and only kept for backwards compatibility

Can we please, please stop saying this! It is not true.

It is preferred that you use -tester: biz.aQute.tester (which is the default) instead of using -tester: biz.aQute.junit (for OSGi Junit launching). But there are valid uses cases for -tester: biz.aQute.junit. We use it in certain OSGi compliance test cases where we are testing OSGi framework launching and thus there is no framework for the test cases since the test cases launch the framework as part of their testing.

Also, completely aside from launching OSGi Junit, biz.aQute.junit is fine bundle which exports Junit 3 and 4 (and soon, hopefully, 5) and AssertJ packages for test cases to compile against and for test cases in bundles to import at runtime.

So biz.aQute.junit is not deprecated, not going anywhere, and must continue to be supported. We will of course prefer the use of biz.aQute.tester as the launcher for OSGi Junit tests (aka. -tester: biz.aQute.tester).

pkriens commented 5 years ago

Ehhm, I am missing something. Why can't you use tester and then the preferred JUnit library in your tests? Even if there is no framework you should be able to pick a JUnit library and add it to the runpath? The combination of the tester and the JUnit code was just a bad idea in my mind and therefore biz.aQute.junit is only there for backward compatibility?

bjhargrave commented 5 years ago

biz.aQute.junit is a Junit library with nice OSGi metadata managed by an active project which is OSGi saavy. I choose it. :-)

pkriens commented 5 years ago

That was the reason for osgi.enroute.junit.wrapper and I think there is now also one in servicemix ... It is just not sound and not cohesive to couple the bnd code directly to the junit code. So we should promote the tester model.

kriegfrj commented 5 years ago

For what little it's worth, I tend to agree with @pkriens on this one. If the only reason for biz.aQute.unit to exist is to repackage JUnit, that doesn't seem to be a good enough reason to maintain it. There are already other bundles out there that do that, and as far as I know they work fine. If there are other reasons to maintain biz.aQute.junit I'm happy to hear them, but in my (admittedly inexperienced) opinion, this alone would not be a good enough reason.

bjhargrave commented 5 years ago

Peter, you are always a big proponent of not breaking backwards compatibility. So it seems wrong to break it here.

I am for promoting using tester as the launcher. But that does not mean you break function. If you need to duplicate the launching function and evolve it only in tester, that may make sense if it is the only way forward. But you should not remove the existing launching function from junit.

Also, osgi.enroute.junit.wrapper is a dead end. It is unsupported and not maintained by the OSGi Alliance since enRoute updated to v7.

pkriens commented 5 years ago

I never wanted to break backward compatibility! We need to move forward but the reason I did not do the tester functions in biz.aQute.junit was to not break anybody! So we fully agree.

However, I created tester to move forwards since the coupling with the JUnit version was painful. So if Fr. Krieg wants to do work we should make sure that moving forwards is done in the proper project. Existing projects should never break but I think it is fair to ask them to move to tester when they want to use JUnit 5?

The enroute wrappers were recently updated by Qivicon. They are still the best wrapped version of JUnit and hamcrest around as far as I know.

bjhargrave commented 5 years ago

Existing projects should never break but I think it is fair to ask them to move to tester when they want to use JUnit 5?

Yes, this is what I am saying. But this means you cannot remove the existing launching function from biz.aQute.junit.

pkriens commented 5 years ago

I never argued for that ... and that would be extremely unlikely for me to argue such breakage?

bjhargrave commented 5 years ago

I am just trying to make clear that we cannot break biz.aQute.junit.

pkriens commented 5 years ago

Which seems to be preaching to choir, hich much sounds like gospel to Fr. Krieg :-)

kriegfrj commented 5 years ago

Thanks guys for thrashing this out.

Ok, so we're all on the same page then: Leave biz.aQute.junit as-is, and make sure its functionality doesn't break. However, JUnit 5 support won't be added to biz.aQute.junit: if you want it, you'll have to migrate to biz.aQute.tester.

I'll start working on a solution. I'm sure that one I've submitted the first prototype there'll be more discussion about some of these issues then if necessary.