POC of junit5/jupiter test migration #1195

jbtrystram · 2019-04-24T15:08:58Z

This is a POC on the tests to experiment the junit 5 tests, as discussed in a recent community call.
As expected, it's needed to rewrite a bunch of things, but nothing really complicated.

Also, there is currently no support for class-wide @Rule timeout, as we currently use in hono. Each test must have a Timeout specified. However, this feature should be introduced back in the next jupiter release, as this is a popular demand ( junit-team/junit5#80).

On the plus side there are some nice features such as nested tests that would allow to improve the maintainability of the tests ( see FileBasedCredentialsServiceTest.testLoadCredentialsCanReadOutputOfSaveToFile where I needed countdownLatch to replace the vertx Async. This could be factored the @BeforeEach method of a nested test class)
I also removed all the vertx assertions to use only the assertions provided by Junit.
It is also nice that both Junit4 and jupiter can coexist without disrupting anything, so the migration can happen over time.

I also completely removed the junit4 dependency : all the tests can run through the vintage engine. I had to add hamcrest as i think it was pulled through junit4. Keeping both dependencies made the codecov bot miss out on the test coverage.

For more details on VertxTestContext see https://vertx.io/docs/vertx-junit5/java/

Signed-off-by: Trystram Jean-Baptiste [email protected]

codecov · 2019-04-25T07:20:15Z

Codecov Report

Merging #1195 into master will decrease coverage by 0.16%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1195      +/-   ##
============================================
- Coverage     68.47%   68.31%   -0.17%     
  Complexity      383      383              
============================================
  Files           276      276              
  Lines         12427    12427              
  Branches       1057     1057              
============================================
- Hits           8510     8489      -21     
- Misses         3151     3170      +19     
- Partials        766      768       +2

Impacted Files	Coverage Δ	Complexity Δ
...se/hono/connection/impl/ConnectionFactoryImpl.java	`64.59% <0%> (-4.97%)`	`0% <0%> (ø)`
...ipse/hono/service/VertxBasedHealthCheckServer.java	`67.39% <0%> (-4.35%)`	`0% <0%> (ø)`
.../org/eclipse/hono/service/AbstractApplication.java	`55.78% <0%> (-2.11%)`	`0% <0%> (ø)`
...a/org/eclipse/hono/service/metric/MetricsTags.java	`88.46% <0%> (-1.29%)`	`0% <0%> (ø)`
.../eclipse/hono/client/impl/TelemetrySenderImpl.java	`70.29% <0%> (-1%)`	`0% <0%> (ø)`
...er/mqtt/AbstractVertxBasedMqttProtocolAdapter.java	`73.81% <0%> (-0.66%)`	`0% <0%> (ø)`
...ipse/hono/service/AbstractProtocolAdapterBase.java	`77.17% <0%> (-0.65%)`	`0% <0%> (ø)`
...er/http/AbstractVertxBasedHttpProtocolAdapter.java	`78.1% <0%> (-0.25%)`	`0% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce8c4c7...d5f98a0. Read the comment docs.

dejanb · 2019-04-25T10:59:44Z

bom/pom.xml

@@ -42,7 +43,7 @@
    <javax.annotation.api.version>1.3.2</javax.annotation.api.version>
    <jjwt.version>0.7.0</jjwt.version>
    <jmeter.version>3.3</jmeter.version>
-    <junit.version>4.12</junit.version>
+    <junit.jupiter.version>5.4.2</junit.jupiter.version>


I looked up CQs related to this and I found 5.4.0 is already approved (https://dev.eclipse.org/ipzilla/show_bug.cgi?id=18942) and 5.4.1 (https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19534) is currently being processed (although there's some issues)

Maybe we can use some of these version, so we can easily piggyback on this CQ?

Why not. We'll have to use 5.5 if we want to get back the Timer rule anyway..

@jbtrystram So timer rule is just ignored at the moment? Is there a workaround for that before 5.5? Or is it not worth bothering?

Also, we might want to try 5.5.0-M1 if we can wait for the official release before merging it?

in tests using jupiter the timer rule is ignored yes. It's explained better here : junit-team/junit5#80
trying 5.5.0-M1 make sense yes. As i'd like to migrate the complete module before merging anyway

dejanb · 2019-04-25T11:10:07Z

...c/test/java/org/eclipse/hono/service/credentials/AbstractCompleteCredentialsServiceTest.java

-        getCompleteCredentialsService().remove("tenant", "myType", "myId", ctx.asyncAssertSuccess(s -> {
-            assertThat(s.getStatus(), is(HttpURLConnection.HTTP_NO_CONTENT));
+        getCompleteCredentialsService().remove("tenant", "myType", "myId", ctx.succeeding(s -> ctx.verify(() -> {
+            assertEquals(HttpURLConnection.HTTP_NO_CONTENT, s.getStatus());
            assertNotRegistered(getCompleteCredentialsService(), "tenant", "myId", "myType", ctx);


Does this helps with "nested" calls issues we experienced with Infinispan client?

did not tried yet ! I would need to finish porting this module first, then port the infinispan test :)
why do you think it would help ?

No, I don't think it would. Just thinking maybe we can tackle this at the same time. You can always use your local builds (SNAPSHOTS) to test against Infinispan. But we can do it, one step at the time.

I'll definitely give it a try :)

.travis.yml

sophokles73 · 2019-04-25T11:27:18Z

...c/test/java/org/eclipse/hono/service/credentials/AbstractCompleteCredentialsServiceTest.java

-        }));
-        add.await();
+        getCompleteCredentialsService().add("tenant", payload2, ctx.succeeding(s -> ctx.verify(() -> {
+            assertEquals(HttpURLConnection.HTTP_CONFLICT, s.getStatus());


what's wrong with using assertThat? FMPOV it is easier to read and produces better error messages...

assertThat is removed from junit5. IMHO, for comparing HTTP status codes assertEquals is more intuitive.

that said it's totally possible to keep using the hamcrest Assertion Library : https://junit.org/junit5/docs/5.0.1/user-guide/#writing-tests-assertions-third-party

just wanted to make sure that we do not strictly replace all occurrences of assertThat with assertEquals because we think that we have to.

We absolutely don't have to. As long as hamcrest-core stays in the dependencies that will work fine. I simply wanted to try it because using the built-in assertions would remove a dependency. And also for the sake of trying something new :)

sophokles73 · 2019-04-25T11:32:06Z

...c/test/java/org/eclipse/hono/service/credentials/AbstractCompleteCredentialsServiceTest.java

-        add.await();
+        getCompleteCredentialsService().add("tenant", payload2, ctx.succeeding(s -> ctx.verify(() -> {
+            assertEquals(HttpURLConnection.HTTP_CONFLICT, s.getStatus());
+            ctx.completeNow();


it looks strange to me that you would need to explicitly invoke this method. Have you checked if it works without?

the ctx.completeNow(); method ? Yes, I agree with you that it's a bit odd.
I went to re-read the vertx doc and send you a link but it look like when the test expect a success I could use ctx.completing() which complete the test context on a success. I am going to try it !

edit : false hope. As we need to do assertions, we need to get the handler result so completing doesn't work. https://vertx.io/docs/vertx-junit5/java/#_use_any_assertion_library
so if we remove the completeNow() method the test timeout because the VertxTestContext is not notified that the test is complete.

So as @jbtrystram explained, with vertx it is necessary to let the async test cases know that the test is actually completed. As the idea is that the unit tests may still be running when it exited the method annotated with @Test.

One thing to note here, the call to completeNow() actually completes the test context. If the test has multiple steps, you need to use Checkpoint as done in the other tests.

sophokles73 · 2019-04-25T12:36:12Z

@jbtrystram would it also be possible to keep all tests as-is for the time being and simply replace junit 4.12 with junit5 using the legacy engine?

jbtrystram · 2019-04-25T12:40:48Z

@jbtrystram would it also be possible to keep all tests as-is for the time being and simply replace junit 4.12 with junit5 using the legacy engine?

@sophokles73 it's exactly what i've done here :) See b69291e

sophokles73 · 2019-04-26T09:02:40Z

@jbtrystram FMPOV it would be nice to create a separate PR for migrating to JUnit5 first, i.e. removing the dependency on JUnit4 altogether and instead run the existing tests using the legacy engine.
Then we could use this PR to actually start migrating the first tests to JUnit5.
WDYT? @dejanb @jbtrystram

dejanb · 2019-04-26T09:52:59Z

@sophokles73 sounds reasonable to me. Did we decide which exact version are we going to use? 5.5.0-M1 for the timeout support?

sophokles73 · 2019-04-26T11:09:34Z

@dejanb @jbtrystram not sure if I fully understood the consequences of not having the timeout support. Would that mean that the Timeout Rule that we use in the existing tests wouldn't work any more or would it just mean that in newly created JUnit5 based tests we would need to specify the timeout on each method individually? The latter wouldn't be a problem FMPOV since in many cases we would like to do that in any case ...
So, my preference would be not to use a milestone but a released version, if possible.

jbtrystram · 2019-04-26T11:38:00Z

it just mean that in newly created JUnit5 based tests we would need to specify the timeout on each method individually

@dejanb @sophokles73 This is correct I think.

sophokles73 · 2019-04-26T12:58:21Z

@jbtrystram @dejanb can we then agree on using the most recent released version of JUnit5?

dejanb · 2019-04-26T12:59:52Z

@sophokles73 yes. I'll get the CQs going.

sophokles73 · 2019-04-26T13:06:56Z

@dejanb we do not need CQs for that, we already have a works-with CQ for JUnit which covers version 4 and higher ...

dejanb · 2019-04-26T13:11:50Z

@sophokles73 ok, great

sophokles73 · 2019-04-29T13:31:02Z

@jbtrystram can you rebase this PR now that we have migrated to JUnit5?

All the base tests needed for testing device registries implementations are now using junit5. The file based registry unit tests use Junit5 Signed-off-by: Trystram Jean-Baptiste <[email protected]>

ctron · 2019-05-02T07:50:47Z

@sophokles73 I am trying to sum up what is missing in order to merge this PR. It is rebased, we don't need new CQs. So the only thing missing is to move the @Timeout annotation to individual tests?!

sophokles73 · 2019-05-02T07:59:12Z

@ctron you probably mean: we do not need new PRs, right? FMPOV there is nothing missing apart from @dejanb and your approval ...

jbtrystram · 2019-05-02T08:11:54Z

@ctron the default 30sec timeout is still working!
I migrated the file based tests as well (as they extend the base tests), FMPOV it's good to go, and thoses tests can be looked by other guys that may want to write tests in the future :)

ctron · 2019-05-02T08:28:35Z

@ctron you probably mean: we do not need new PRs, right? FMPOV there is nothing missing apart from @dejanb and your approval ...

No, I did mean that we do not need additional CQs for merging this.

jbtrystram requested review from ctron, dejanb and sophokles73 as code owners April 24, 2019 15:08

jbtrystram force-pushed the junit5-dr-tests branch from 16f2aaf to 99c77a3 Compare April 24, 2019 15:09

dejanb reviewed Apr 25, 2019

View reviewed changes

sophokles73 reviewed Apr 25, 2019

View reviewed changes

jbtrystram mentioned this pull request Apr 26, 2019

remove junit4 and use Junit5 vintage engine for all the tests #1207

Merged

jbtrystram force-pushed the junit5-dr-tests branch from d278c7a to 733c107 Compare April 29, 2019 14:00

jbtrystram mentioned this pull request Apr 30, 2019

Iot device registry backed with infinispan EnMasseProject/enmasse#2498

Merged

jbtrystram force-pushed the junit5-dr-tests branch from bb3c2a7 to 0d6e7f7 Compare April 30, 2019 15:47

junit5/jupiter test migration for some tests in the service-base module.

d5f98a0

All the base tests needed for testing device registries implementations are now using junit5. The file based registry unit tests use Junit5 Signed-off-by: Trystram Jean-Baptiste <[email protected]>

jbtrystram force-pushed the junit5-dr-tests branch from 0d6e7f7 to d5f98a0 Compare May 1, 2019 07:22

ctron approved these changes May 2, 2019

View reviewed changes

sophokles73 added this to the 1.0.0 milestone May 2, 2019

sophokles73 merged commit 014d7c7 into eclipse-hono:master May 2, 2019

jbtrystram deleted the junit5-dr-tests branch May 3, 2019 06:37

jbtrystram mentioned this pull request May 6, 2019

removed ununsed checkpoint in tests #1220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POC of junit5/jupiter test migration #1195

POC of junit5/jupiter test migration #1195

jbtrystram commented Apr 24, 2019 •

edited

Loading

codecov bot commented Apr 25, 2019 •

edited

Loading

dejanb Apr 25, 2019

jbtrystram Apr 25, 2019

dejanb Apr 25, 2019

dejanb Apr 25, 2019

jbtrystram Apr 25, 2019 •

edited

Loading

dejanb Apr 25, 2019

jbtrystram Apr 25, 2019 •

edited

Loading

dejanb Apr 25, 2019

jbtrystram Apr 25, 2019

sophokles73 Apr 25, 2019

jbtrystram Apr 25, 2019 •

edited

Loading

sophokles73 Apr 25, 2019 •

edited

Loading

jbtrystram Apr 25, 2019

sophokles73 Apr 25, 2019

jbtrystram Apr 25, 2019 •

edited

Loading

ctron Apr 25, 2019

sophokles73 commented Apr 25, 2019

jbtrystram commented Apr 25, 2019 •

edited

Loading

sophokles73 commented Apr 26, 2019

dejanb commented Apr 26, 2019

sophokles73 commented Apr 26, 2019

jbtrystram commented Apr 26, 2019

sophokles73 commented Apr 26, 2019

dejanb commented Apr 26, 2019

sophokles73 commented Apr 26, 2019

dejanb commented Apr 26, 2019

sophokles73 commented Apr 29, 2019

ctron commented May 2, 2019

sophokles73 commented May 2, 2019

jbtrystram commented May 2, 2019

ctron commented May 2, 2019

POC of junit5/jupiter test migration #1195

POC of junit5/jupiter test migration #1195

Conversation

jbtrystram commented Apr 24, 2019 • edited Loading

codecov bot commented Apr 25, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbtrystram Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbtrystram Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbtrystram Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

sophokles73 Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbtrystram Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophokles73 commented Apr 25, 2019

jbtrystram commented Apr 25, 2019 • edited Loading

sophokles73 commented Apr 26, 2019

dejanb commented Apr 26, 2019

sophokles73 commented Apr 26, 2019

jbtrystram commented Apr 26, 2019

sophokles73 commented Apr 26, 2019

dejanb commented Apr 26, 2019

sophokles73 commented Apr 26, 2019

dejanb commented Apr 26, 2019

sophokles73 commented Apr 29, 2019

ctron commented May 2, 2019

sophokles73 commented May 2, 2019

jbtrystram commented May 2, 2019

ctron commented May 2, 2019

jbtrystram commented Apr 24, 2019 •

edited

Loading

codecov bot commented Apr 25, 2019 •

edited

Loading

jbtrystram Apr 25, 2019 •

edited

Loading

jbtrystram Apr 25, 2019 •

edited

Loading

jbtrystram Apr 25, 2019 •

edited

Loading

sophokles73 Apr 25, 2019 •

edited

Loading

jbtrystram Apr 25, 2019 •

edited

Loading

jbtrystram commented Apr 25, 2019 •

edited

Loading