Switch to JUnit 5 #2048

Marcono1234 · 2022-01-05T22:09:30Z

Resolves #1896 (see below)

Changes:

Remove Gradle build support
The Gradle build setup is outdated, and trying to keep two setups (Maven and Gradle) up to date is error-prone. See also build.gradle project version is outdated #1896.
Personally I think it would be useful to switch completely to Gradle at some point because its features such as toolchains make it easier to create a reliable build setup.
Switch to JUnit 5
- Reduce test class and method visibility to package-private (because JUnit 5 supports that). This is less verbose and also reduces unrelated test classes showing up in code completion while writing tests.
- Consistently use @Disabled to disable tests (though maybe I missed some). Previously prefixes such as disabled_ were used to disable test methods of TestCase subclasses.
- Use @BeforeEach and @AfterEach for setup and teardown methods
- Switch some method arguments because JUnit 4 has the assertion message as first argument while JUnit 5 has it as last
- Use Java 11 for compiling and running tests; IDE support for that is limited, see below

This pull request is 'complete', but I have marked it as Draft for now because there are some open issues:

The proto module uses Google Truth which has a transitive dependency on JUnit 4; we therefore need to be careful to not accidentially mix JUnit 4 and JUnit 5 usage in the proto module
Some IDEs (at least Eclipse IDE) only seem to support one Java version for the complete project. They do not understand that main and test sources should be compiled with different Java versions. JUnit 5 requires at least Java 8. This causes spurious IDE compiler errors, but running the tests might work nonetheless. However, it makes using newer Java features, such as lambdas for JUnit 5's assertThrows more annoying. And these spurious IDE compiler errors might be irritating for new contributors. Due to this I have also not replaced usage of try { ... fail(); } catch (...) { } constructs with assertThrows yet.
Note that Maven itself does not seem to have a problem with this.

Because this pull request removes a few test methods (see review comments), marks some as test but disables them with @Disabled and because some test methods are converted to parameterized tests, the number of executed tests changed slightly. However, it looks like all tests are still executed:

-[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0 in com.google.gson.functional.ConcurrencyTest
+[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0 in com.google.gson.functional.ConcurrencyTest

-[INFO] Tests run: 19, Failures: 0, Errors: 0, Skipped: 0 in com.google.gson.functional.CustomTypeAdaptersTest
+[WARNING] Tests run: 21, Failures: 0, Errors: 0, Skipped: 2 in com.google.gson.functional.CustomTypeAdaptersTest

-[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0 in com.google.gson.functional.MapAsArrayTypeAdapterTest
+[WARNING] Tests run: 6, Failures: 0, Errors: 0, Skipped: 1 in com.google.gson.functional.MapAsArrayTypeAdapterTest

-[INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0 in com.google.gson.GsonBuilderTest
+[INFO] Tests run: 10, Failures: 0, Errors: 0, Skipped: 0 in com.google.gson.GsonBuilderTest

-[INFO] Running com.google.gson.metrics.PerformanceTest
-[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 in com.google.gson.metrics.PerformanceTest

-[INFO] Tests run: 12, Failures: 0, Errors: 0, Skipped: 0 in com.google.gson.MixedStreamTest
+[WARNING] Tests run: 13, Failures: 0, Errors: 0, Skipped: 1 in com.google.gson.MixedStreamTest

-[WARNING] Tests run: 20, Failures: 0, Errors: 0, Skipped: 1 in com.google.gson.stream.JsonReaderPathTest
+[INFO] Tests run: 19, Failures: 0, Errors: 0, Skipped: 0 in com.google.gson.stream.JsonReaderPathTest

-[INFO] Tests run: 121, Failures: 0, Errors: 0, Skipped: 0 in com.google.gson.stream.JsonReaderTest
+[WARNING] Tests run: 125, Failures: 0, Errors: 0, Skipped: 4 in com.google.gson.stream.JsonReaderTest

(difference to 4ec67c0)

If this pull request is merged, it is important to pay attention that new or existing PRs properly use @Test because merely naming a method test... will not suffice anymore.

No worries if you don't want to merge this pull request or if you want to wait until (eventually) the target Java version of this project is Java 8 or newer.

Build script was outdated and not actively maintained anymore.

Marcono1234 · 2022-01-05T22:16:47Z

gson/pom.xml

@@ -50,7 +50,6 @@
      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-surefire-plugin</artifactId>
-        <version>3.0.0-M5</version>


Is managed by parent now.

Marcono1234 · 2022-01-05T22:18:32Z

gson/src/main/java/com/google/gson/internal/JavaVersion.java

@@ -37,7 +37,7 @@ static int getMajorJavaVersion(String javaVersion) {
      version = extractBeginningInt(javaVersion);
    }
    if (version == -1) {
-      return 6;  // Choose minimum supported JDK version as default
+      return 7;  // Choose minimum supported JDK version as default


Missed that for #2043, but this change currently has no effect anyway because Gson only uses JavaVersion.isJava9OrLater().

Marcono1234 · 2022-01-05T22:20:22Z

gson/src/test/java/com/google/gson/GsonBuilderTest.java

    Gson gson = new GsonBuilder()
        .excludeFieldsWithModifiers(Modifier.VOLATILE, Modifier.PRIVATE)
        .create();
    assertEquals("{\"d\":\"d\"}", gson.toJson(new HasModifiers()));
  }

-  public void testRegisterTypeAdapterForCoreType() {


Previously this test was not performing any assertion (and was only testing that no exception is thrown?).
Have refactored it; see below.

Marcono1234 · 2022-01-05T22:21:49Z

gson/src/test/java/com/google/gson/JsonObjectTest.java

-    assertEquals(a, a);
+    assertTrue(a.equals(a));


Changed this because assertEquals should not be used to test equals implementation (it might return fast without calling equals at all).

Marcono1234 · 2022-01-05T22:26:47Z

gson/src/test/java/com/google/gson/functional/ConcurrencyTest.java

    gson = new Gson();
  }

  /**
   * Source-code based on
   * http://groups.google.com/group/google-gson/browse_thread/thread/563bb51ee2495081
   */
-  public void testSingleThreadSerialization() { 


Removed these two test methods because they were not performing any assertion. And there are (probably) enough tests already performing basic reflection-based serialization and deserialization.

Marcono1234 · 2022-01-05T22:37:18Z

gson/src/test/java/com/google/gson/functional/MapTest.java

-  		+ "{\n    \"test\": 1,\n    \"TestStringArray\": "
-  		+ "[\n      \"one\",\n      \"two\"\n    ]\n  }\n}",
+        + "{\n    \"test\": 1,\n    \"TestStringArray\": "
+        + "[\n      \"one\",\n      \"two\"\n    ]\n  }\n}",


Was mixing tabs and spaces.

Marcono1234 · 2022-01-05T22:47:57Z

gson/src/test/java/com/google/gson/internal/bind/RecursiveTypesResolveTest.java

    @SuppressWarnings("rawtypes")
    TypeAdapter<Foo1> adapter = new Gson().getAdapter(Foo1.class);
    assertNotNull(adapter);
  }

-  /**


Changed this here and a few lines below because they are not Javadoc comments for a specific method.

Marcono1234 · 2022-01-05T22:52:41Z

gson/src/test/java/com/google/gson/stream/JsonReaderTest.java

-	  	JsonReader reader = new JsonReader(reader("[-0]"));
-	    reader.setLenient(false);
-	    reader.beginArray();
-	    assertEquals(NUMBER, reader.peek());
-	    assertEquals("-0", reader.nextString());


Was using tabs and spaces for indentation.

POM has been configured to compile and run unit tests with Java 11 (because JUnit 5 requires at least Java 8). However, IDEs might not support different Java versions for main code and test code. Another issue is that the proto module uses Google Truth which has JUnit 4 as transitive dependency.

eamonnmcmanus · 2022-01-14T01:07:46Z

I think it would make sense to switch the Gradle removal to a separate PR. I agree with the idea; we clearly haven't been maintaining the Gradle build and it's better to have nothing than something that misleads people into wasting time trying to make it work. It's conceivable that we would switch the build to Gradle at some point but given that Gson is in maintenance mode that might be hard to justify.

Concerning the switch to JUnit 5, internally Google is still on JUnit 4 with no concrete plans to migrate to JUnit 5. So I don't think we can take that change. We'd certainly be interested in a change upgrading the JUnit 3 tests to JUnit 4. Though that would lead to a lot of churn, the improvement is enough to justify it.

Marcono1234 · 2022-02-16T20:08:15Z

Ok, I am closing this pull request then for now. Maybe it becomes relevant in the future again, or it might serve as basis for converting JUnit 3 tests to JUnit 4 tests, in case someone wants to do that.

Remove Gradle build support

e0fb02c

Build script was outdated and not actively maintained anymore.

Marcono1234 commented Jan 5, 2022

View reviewed changes

Marcono1234 added 2 commits January 5, 2022 23:56

Remove jdkToolchain requirement for Surefire plugin

55bb8fa

Marcono1234 force-pushed the marcono1234/junit5 branch from 5a26681 to 55bb8fa Compare January 5, 2022 23:02

Remove Gradle wrapper validation

ce8fdfb

Marcono1234 mentioned this pull request Jan 23, 2022

Remove Gradle build support #2063

Merged

Marcono1234 closed this Feb 16, 2022

Marcono1234 mentioned this pull request Dec 13, 2022

Clean up Gson source code #2284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to JUnit 5 #2048

Switch to JUnit 5 #2048

Marcono1234 commented Jan 5, 2022 •

edited

Loading

Marcono1234 Jan 5, 2022

Marcono1234 Jan 5, 2022

Marcono1234 Jan 5, 2022

Marcono1234 Jan 5, 2022

Marcono1234 Jan 5, 2022

Marcono1234 Jan 5, 2022

Marcono1234 Jan 5, 2022

Marcono1234 Jan 5, 2022

eamonnmcmanus commented Jan 14, 2022

Marcono1234 commented Feb 16, 2022

Switch to JUnit 5 #2048

Switch to JUnit 5 #2048

Conversation

Marcono1234 commented Jan 5, 2022 • 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

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

eamonnmcmanus commented Jan 14, 2022

Marcono1234 commented Feb 16, 2022

Marcono1234 commented Jan 5, 2022 •

edited

Loading