Junit 5 #422

tomblench · 2018-02-28T16:11:44Z

What

Upgrade tests to JUnit 5 (JUnit Jupiter)

How

Refactor tests to use JUnit 5 (org.junit.jupiter.api.*) APIs
Convert existing Resources/Rules etc to JUnit 5 Extensions
Add base classes in com.cloudant.tests.base package for common test
patterns eg remote-data-base-per-class, remote-database-per-method
Convert parameterised tests to TestTemplates which use helper
classes to provide parameters
Convert JUnit 4 Categories to JUnit 5 Tags
Upgrade gradle to 4.6
Add custom JavaExec task to build.gradle to invoke JUnit Platform
Console Launcher, due to lack of flexibility in current gradle
support for JUnit Platform.

Testing

All tests pass, with current exceptions:

DesignDocumentTest - number of tests run is exactly half on this branch. It appears that the existing code ran every test twice, maybe due to the use of @RunWith(Enclosed.class)
HttpProxyTest - two test combinations currently disabled due to failure. Previously it was determined that all test combinations would pass if run in some orderings, but now these two parameter combinations always fail. Needs further investigation.

TODO

~~Remove if (env.BRANCH_NAME == "master" || env.BRANCH_NAME == "junit5") from Jenkinsfile before merging.~~
~~Re-visit java version requirements. 1.8 is required to build and run tests now, although production code should still only require 1.6~~ Fixed in 62e82ad
~~Update copyrights~~
~~Format source files which have been automatically updated to switch order of assert arguments, which now have long lines~~

* Refactor tests to use JUnit 5 (org.junit.jupiter.api.*) APIs * Convert existing Resources/Rules etc to JUnit 5 Extensions * Add base classes in com.cloudant.tests.base package for common test patterns eg remote-data-base-per-class, remote-dat abase-per-method * Convert parameterised tests to TestTemplates which use helper classes to provide parameters * Convert JUnit 4 Categories to JUnit 5 Tags * Upgrade gradle to 4.6 * Add custom JavaExec task to build.gradle to invoke JUnit Platform Console Launcher, due to lack of flexibility in current gradle support for JUnit Platform.

ricellis · 2018-03-09T16:15:13Z

Regarding DesignDocumentTest running locally produces one instance each, but I did find this about running at the package level running things twice with Enclosed https://bugs.eclipse.org/bugs/show_bug.cgi?id=474636
Entirely possible this is the same weirdness.

ricellis

Submitting this first part for now, I'm only about 20% of the way through though, so expect more to follow.

ricellis · 2018-03-09T16:21:37Z

build.gradle

@@ -28,8 +28,8 @@ subprojects {
    // If the version says "snapshot" anywhere assume it is not a release
    ext.isReleaseVersion = !version.toUpperCase(Locale.ENGLISH).contains("SNAPSHOT")

-    sourceCompatibility = 1.6
-    targetCompatibility = 1.6
+    sourceCompatibility = 1.8


is it possible to set this property only for the java test compile?

@ricellis good idea, see 62e82ad

ricellis · 2018-03-09T16:28:56Z

cloudant-client/src/test/java/com/cloudant/api/query/IndexLifecycleTest.java


 import java.util.Collections;
 import java.util.List;


 // This really requires Couch2.0 + text index support, but we don't have a way of expressing that
-@Category(RequiresCloudant.class)
+@RequiresCloudant


I thought one of the objectives of using JUnit 5 tags was being able to separate out different API level tests.
Then we can choose appropriate sets in the matrix for Couch 1.6 and Couch 2 as well as Cloudant.
For example this could be:

@Couch2 @Cloudant

since it applies to any Cloudant or Couch2

Is that intended for a later PR?

Probably best done in a follow-on PR. I wanted to get the tags exactly the same so that I could check each of the matrix builds executes the correct number of tests, before thinking about changing the tags.

ricellis · 2018-03-09T16:32:08Z

cloudant-client/src/test/java/com/cloudant/api/query/IndexLifecycleTest.java


-    private Database db;
+    private static Database db;


Why is this static if it is a @BeforeEach?

@tomblench think you missed this one

Fixed in fe2ecb7 by extending TestWithDbPerTest

ricellis · 2018-03-09T16:33:30Z

cloudant-client/src/test/java/com/cloudant/test/main/RequiresCloudant.java

+@Tag("RequiresCloudant")
+@Tag("RequiresDB")
+public @interface RequiresCloudant {
+}


ricellis · 2018-03-09T16:45:28Z

cloudant-client/src/test/java/com/cloudant/test/main/RequiresCloudant.java

+@Retention(RetentionPolicy.RUNTIME)
+@Tag("RequiresCloudant")
+@Tag("RequiresDB")
+public @interface RequiresCloudant {


Do you feel this level of indirection is preferable to tagging the tests directly? I guess it made replacing the categories easier :)

These tags were straight out of the JUnit guide, so it's probably considered best practice.

ricellis · 2018-03-09T17:07:38Z

cloudant-client/src/test/java/com/cloudant/test/main/RequiresCloudantService.java

+@Tag("RequiresCloudant")
+@Tag("RequiresDB")
+public @interface RequiresCloudantService {
+}


ricellis · 2018-03-09T17:07:45Z

cloudant-client/src/test/java/com/cloudant/test/main/RequiresCouch.java

+@Tag("RequiresCouch")
+@Tag("RequiresDB")
+public @interface RequiresCouch {
+}


ricellis · 2018-03-09T17:07:51Z

cloudant-client/src/test/java/com/cloudant/test/main/RequiresDB.java

+@Retention(RetentionPolicy.RUNTIME)
+@Tag("RequiresDB")
+public @interface RequiresDB {
+}


ricellis · 2018-03-09T17:17:57Z

cloudant-client/src/test/java/com/cloudant/tests/AttachmentsTest.java

-        byte[] bytesToDB = "binary data".getBytes();
-        ByteArrayInputStream bytesIn = new ByteArrayInputStream(bytesToDB);
-        Response response = db.saveAttachment(bytesIn, "foo.txt", "text/plain", null, "1-abcdef");
+        assertThrows(IllegalArgumentException.class, new Executable() {


Since JUnit5 requires Java 8 anyway, can't we use Java8 features in the test code?
I think this would look a lot better as:

final byte[] bytesToDB = "binary data".getBytes(); final ByteArrayInputStream bytesIn = new ByteArrayInputStream(bytesToDB); assertThrows(IllegalArgumentException.class, () -> db.saveAttachment(bytesIn, "foo.txt", "text/plain", null, "1-abcdef"));

Even if we don't got that far, just executing the one line expected to throw inside the Executable would be more clear.

There are 49 instances of assertThrows and wrapping the whole thing in an Executable was easier, because:

That was what the IDE auto-prompted for (lazy, I know)

It's difficult to tell with each test which statement is the exact one responsible for throwing the exception, which i guess is one of the drawbacks of the old @Throws annotation.

So it takes time to understand each test and put the assertThrows in the right place, but I agree that the way you have suggested doing it is a lot clearer and more expressive.

Possibly something to look at towards the end of the PR?

ricellis · 2018-03-09T17:20:48Z

cloudant-client/src/test/java/com/cloudant/tests/AttachmentsTest.java

    }

-    @Test(expected = IllegalArgumentException.class)
+    @Test()


redundant parentheses

Done in 0b3645d

ricellis

Phase II, I've nearly made it halfway!

ricellis · 2018-03-12T18:14:24Z

cloudant-client/src/test/java/com/cloudant/tests/BulkDocumentTest.java

-        db = dbResource.get();
-    }
+@RequiresDB
+public class BulkDocumentTest extends TestWithDb {


Couldn't TestWithDb reasonably include the @RequiresDB annotation and avoid the need for it to be present throughout so many test classes. I'm assuming anything that extends TestWithDb also RequiresDB though I guess which DB could be important. Something to think about.

It's a nice idea but we have test classes like CloudantClientTests which extend TestWithDb but only need the database for a couple of tests, so the whole class need not be marked as @RequiresDB

ricellis · 2018-03-12T18:17:11Z

cloudant-client/src/test/java/com/cloudant/tests/CloudantClientHelper.java

@@ -47,6 +48,8 @@
        try {
            //a URL might be supplied, otherwise use the separate properties
            String URL = System.getProperty("test.couch.url");
+            System.out.println(System.getProperties());


If you think this and its counterpart on the line below are necessary for debugging can you make it a JUL style thing so we can turn it off if we want. Or if it was just overlooked delete it.

I'd left some debugging in, especially for when test parameters weren't being passed in properly from gradle. This is no longer needed, so I've got rid of all of these in 96ff132

ricellis · 2018-03-12T18:21:06Z

cloudant-client/src/test/java/com/cloudant/tests/DatabaseTest.java

-    public static void setUp() throws Exception {
-        account = clientResource.get();
-        db = dbResource.get();
+    // TODO before class?


Does this need doing?

See 1d42697

Per the commit comment "mockWebServer needs to be fetched from extension before each test because the extension is a "per test" extension (overrides beforeEach/afterEach)".

It's a bit wasteful to stop/start the mock web server for each test when currently only one test uses it, but I think the overhead is negligible.

mockWebServer needs to be fetched from extension before each test because the extension is a "per test" extension (overrides beforeEach/afterEach)

emlaver · 2018-03-13T19:53:41Z

cloudant-client/src/test/java/com/cloudant/tests/AttachmentsTest.java

+                ByteArrayInputStream bytesIn = new ByteArrayInputStream(bytesToDB);
+                Response response = db.saveAttachment(bytesIn, "foo.txt", "text/plain", null,
+                        "1-abcdef");
+            }


Can we consider moving the code in the block above into a helper util method since it occurs several times in this test class?

Probably outside the scope of what I'm doing, which is just translating the tests to junit5 without considering too closely how these tests are written.

emlaver · 2018-03-14T16:31:39Z

cloudant-client/src/test/java/com/cloudant/tests/DocumentsCRUDTest.java

@@ -93,13 +74,14 @@ public void findJsonObject() {
        assertNotNull(jsonObject);
    }

+    /*
    @Test


Is this commented because the test is failing? If so, should we instead use @Disabled?

The test wasn't commented out before so if it's failing now we need to fix it. In general though, I suppose this test need not use a Couch specific endpoint, which is perhaps what is pending some thought here?

Fixed in 3e9d231

emlaver · 2018-03-14T16:42:41Z

I'm seeing test classes that import org.junit.jupiter.api.Assertions and others that import the utility method such as org.junit.jupiter.api.Assertions.assertEquals. Should we pick one way to import for consistency in all the test classes?

emlaver · 2018-03-14T17:11:03Z

cloudant-client/src/test/java/com/cloudant/api/query/IndexLifecycleTest.java

@@ -49,14 +47,14 @@
 */
 public class IndexLifecycleTest {


Something for a future PR to possibly add clarity in the logs: adding @DisplayName for test classes e.g. @DisplayName("Index lifecycle test...")

Probably future PR, but it would be useful

emlaver · 2018-03-14T17:53:39Z

cloudant-client/src/test/java/com/cloudant/tests/UnicodeTest.java

-    }
+    private static final String TESTJSON = "{\"_id\":\"literal\"," +
+            "\"_rev\":\"1-39933759c7250133b6039d94ea09134f\",\"foo\":\"Grüße 日本語 \uD834\uDD1E.\"}\n";
+    private static final String TESTJSON_ESCAPED = "{\"_id\":\"escaped\"," +


Shouldn't this have \\ similar to TESTSTRING_ESCAPED above?

Thanks for spotting this inconsistency. These are actually used as "expected" values for assertions later, and the json deserialiser will always unescape these values, so they are correct in that regard.

However, the logic and naming of some variables is confusing so I've fixed that in e8a6acb

emlaver · 2018-03-14T18:01:00Z

cloudant-client/src/test/java/com/cloudant/tests/ViewsTest.java

@@ -878,11 +872,10 @@ public void multiRequest() throws IOException {
                .build();
        int i = 1;
        List<ViewResponse<String, Object>> responses = multi.getViewResponses();
-        assertEquals("There should be 3 respones for 3 requests", 3, responses.size());
+        assertEquals(3, responses.size(), "There should be 3 respones for 3 requests");


respones -> responses

Fixed in f378a13

emlaver · 2018-03-14T18:02:10Z

cloudant-client/src/test/java/com/cloudant/tests/ViewsTest.java

@@ -999,15 +1013,15 @@ public void multiRequestMixedReduced() throws IOException {
                .build();

        List<ViewResponse<String, Object>> responses = multi.getViewResponses();
-        assertEquals("There should be 2 respones for 2 requests", 2, responses.size());
+        assertEquals(2, responses.size(), "There should be 2 respones for 2 requests");


respones -> responses

Fixed in f378a13

emlaver · 2018-03-14T18:06:13Z

cloudant-client/src/test/java/com/cloudant/tests/base/TestWithDb.java

+import org.junit.jupiter.api.extension.RegisterExtension;
+
+// Base class for tests which require a DB which is used throughout the lifetime of the test class
+public class TestWithDb {


What about having a TestWithDbPerClass that would extend a base TestWithDb class?
TestWithDbPerTest would also extend TestWithDb.

@emlaver good idea, done in 6be59bb

emlaver · 2018-03-14T18:22:11Z

cloudant-client/src/test/java/com/cloudant/tests/base/TestWithDb.java

@@ -0,0 +1,30 @@
+package com.cloudant.tests.base;


Add copyright

emlaver · 2018-03-14T18:23:01Z

cloudant-client/src/test/java/com/cloudant/tests/base/TestWithDbPerTest.java

@@ -0,0 +1,35 @@
+package com.cloudant.tests.base;


Add copyright

ricellis

Last few comments, now I've completed a first pass.

ricellis · 2018-03-16T16:05:24Z

cloudant-client/src/test/java/com/cloudant/tests/DocumentsCRUDTest.java

@@ -93,13 +74,14 @@ public void findJsonObject() {
        assertNotNull(jsonObject);
    }

+    /*
    @Test


The test wasn't commented out before so if it's failing now we need to fix it. In general though, I suppose this test need not use a Couch specific endpoint, which is perhaps what is pending some thought here?

ricellis · 2018-03-19T13:09:32Z

cloudant-client/src/test/java/com/cloudant/tests/SessionInterceptorExpiryTests.java


-    // Note Parameter(0) okUsable is inherited
+        // because we fill in the args from the left, we can fill in the single argument "okUsable"


This comment is a bit confusing. AFAICT the boolean value of okUsable is provided in the Stream of TestTemplateInvocationContexts here and passed into the test method's parameters. I think what this comment is saying is that the resolution of the template parameters precedes the superclass's BeforeEach execution so we can safely provide the boolean this way i.e. the value is set before it is referenced by the superclass?

I've just removed it in 5d5abf5 because it was wrong and there's no comment needed here

ricellis · 2018-03-19T14:00:25Z

cloudant-client/src/test/java/com/cloudant/tests/ViewsTest.java

-        assertEquals("The results list should be of length 0", 0, db.getViewRequestBuilder
-                ("example", "by_tag").newRequest(Key.Type.STRING, Object.class).keys
-                ("javax").build().getResponse().getKeys().size());
+        assertEquals(0, db.getViewRequestBuilder("example", "by_tag").newRequest(Key.Type.STRING, Object.class).keys("javax").build().getResponse().getKeys().size(), "The results list should be of length 0");


It seems like some of these code replacements have produced very long lines. Perhaps at the end of the review process it is just worth applying the formatter to all the classes before the final merge?

Good idea, i've added it to the TODOs

ricellis · 2018-03-19T14:04:36Z

...nt-client/src/test/java/com/cloudant/tests/extensions/CloudantClientMockServerExtension.java

@@ -0,0 +1,60 @@
+/*
+ * Copyright © 2017 IBM Corp. All rights reserved.


As you said in the TODOs description for this PR there are a bunch of copyright updates to do.

ricellis · 2018-03-19T14:14:49Z

cloudant-client/src/test/java/com/cloudant/tests/extensions/MultiExtension.java

+import org.junit.jupiter.api.extension.Extension;
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+public class MultiExtension implements Extension, AfterAllCallback, AfterEachCallback,


Remind me why this is necessary, was it that @RegisterExtension doesn't currently guarantee an order and @ExtendWith doesn't allow arguments to be passed?

Yes, see junit-team/junit5#506 (comment)

Note some files have had line endings converted to UNIX in the process

ricellis

+1
Don't forget to remove the Jenkinsfile junit branch name check.
Also I think it would be good to open one or more follow up issues for these points that didn't make it into this:
i) Improve test tagging
ii) Throws assertions in java 8 style
iii) AttachmentsTest refactor
iv) @DisplayName

Thanks

emlaver

Looks good. I'm also +1 on future work item for improving test tagging.

tomblench force-pushed the junit5 branch from 9937221 to 5b2aaf1 Compare March 7, 2018 15:43

tomblench force-pushed the junit5 branch from c336d2a to 7814e1a Compare March 7, 2018 17:43

ricellis reviewed Mar 9, 2018

View reviewed changes

Use Java 1.6 for production code and 1.8 for test code

62e82ad

ricellis reviewed Mar 12, 2018

View reviewed changes

Tom Blench added 3 commits March 13, 2018 10:40

Remove debug printlns

96ff132

Remove comment

1d42697

mockWebServer needs to be fetched from extension before each test because the extension is a "per test" extension (overrides beforeEach/afterEach)

Remove parens

0b3645d

emlaver self-requested a review March 13, 2018 14:39

emlaver reviewed Mar 13, 2018

View reviewed changes

emlaver reviewed Mar 14, 2018

View reviewed changes

ricellis reviewed Mar 19, 2018

View reviewed changes

Tom Blench added 2 commits March 26, 2018 11:07

Fix findAny() to use _all_dbs which is available everywhere

3e9d231

Make per-test member non static

209e8e3

tomblench force-pushed the junit5 branch from ad3ac90 to 209e8e3 Compare March 26, 2018 10:08

Tom Blench added 3 commits March 26, 2018 11:19

Clarify logic and variable naming in UnicodeTest

e8a6acb

Typos

f378a13

Abstract class TestWithDB for per test/class variants

6be59bb

tomblench force-pushed the junit5 branch from fe8add3 to 6be59bb Compare March 26, 2018 15:52

Copyrights

f324961

Note some files have had line endings converted to UNIX in the process

Tom Blench added 5 commits March 27, 2018 15:31

Remove comment

5d5abf5

Format code and optimise imports

d67d464

Fix findAny

adb4f20

extend TestWithDbPerTest

fe2ecb7

Copyrights

b3a4d29

ricellis approved these changes Mar 27, 2018

View reviewed changes

Revert full matrix test change

6bb8b94

emlaver approved these changes Mar 28, 2018

View reviewed changes

tomblench merged commit 3529583 into master Mar 28, 2018

ricellis deleted the junit5 branch April 24, 2018 12:30

ricellis added this to the 2.next milestone Apr 25, 2018


		// Note Parameter(0) okUsable is inherited
		// because we fill in the args from the left, we can fill in the single argument "okUsable"

		@@ -0,0 +1,60 @@
		/*
		* Copyright © 2017 IBM Corp. All rights reserved.

Junit 5 #422

Junit 5 #422

Conversation

tomblench commented Feb 28, 2018 • edited Loading

What

How

Testing

TODO

ricellis commented Mar 9, 2018 • edited Loading

ricellis left a comment

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricellis left a comment

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

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

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

emlaver commented Mar 14, 2018

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

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

Choose a reason for hiding this comment

tomblench Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricellis left a comment

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

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

Choose a reason for hiding this comment

ricellis left a comment

Choose a reason for hiding this comment

emlaver left a comment

Choose a reason for hiding this comment

tomblench commented Feb 28, 2018 •

edited

Loading

ricellis commented Mar 9, 2018 •

edited

Loading

tomblench Mar 26, 2018 •

edited

Loading