Switch to JUnit 5 - #2048
Conversation
Build script was outdated and not actively maintained anymore.
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <version>3.0.0-M5</version> |
There was a problem hiding this comment.
Is managed by parent now.
| } | ||
| if (version == -1) { | ||
| return 6; // Choose minimum supported JDK version as default | ||
| return 7; // Choose minimum supported JDK version as default |
There was a problem hiding this comment.
Missed that for #2043, but this change currently has no effect anyway because Gson only uses JavaVersion.isJava9OrLater().
| assertEquals("{\"d\":\"d\"}", gson.toJson(new HasModifiers())); | ||
| } | ||
|
|
||
| public void testRegisterTypeAdapterForCoreType() { |
There was a problem hiding this comment.
Previously this test was not performing any assertion (and was only testing that no exception is thrown?).
Have refactored it; see below.
| assertEquals(a, a); | ||
| assertTrue(a.equals(a)); |
There was a problem hiding this comment.
Changed this because assertEquals should not be used to test equals implementation (it might return fast without calling equals at all).
| * Source-code based on | ||
| * http://groups.google.com/group/google-gson/browse_thread/thread/563bb51ee2495081 | ||
| */ | ||
| public void testSingleThreadSerialization() { |
There was a problem hiding this comment.
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.
| + "{\n \"test\": 1,\n \"TestStringArray\": " | ||
| + "[\n \"one\",\n \"two\"\n ]\n }\n}", | ||
| + "{\n \"test\": 1,\n \"TestStringArray\": " | ||
| + "[\n \"one\",\n \"two\"\n ]\n }\n}", |
There was a problem hiding this comment.
Was mixing tabs and spaces.
| assertNotNull(adapter); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Changed this here and a few lines below because they are not Javadoc comments for a specific method.
| JsonReader reader = new JsonReader(reader("[-0]")); | ||
| reader.setLenient(false); | ||
| reader.beginArray(); | ||
| assertEquals(NUMBER, reader.peek()); | ||
| assertEquals("-0", reader.nextString()); |
There was a problem hiding this comment.
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.
5a26681 to
55bb8fa
Compare
|
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. |
|
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. |
2 participants
Resolves #1896 (see below)
Changes:
The Gradle build setup is outdated, and trying to keep two setups (Maven and Gradle) up to date is error-prone. See also
build.gradleproject 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.
@Disabledto disable tests (though maybe I missed some). Previously prefixes such asdisabled_were used to disable test methods ofTestCasesubclasses.@BeforeEachand@AfterEachfor setup and teardown methodsThis pull request is 'complete', but I have marked it as Draft for now because there are some open issues:
protomodule 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 theprotomoduleassertThrowsmore annoying. And these spurious IDE compiler errors might be irritating for new contributors. Due to this I have also not replaced usage oftry { ... fail(); } catch (...) { }constructs withassertThrowsyet.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
@Disabledand 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:(difference to 4ec67c0)
If this pull request is merged, it is important to pay attention that new or existing PRs properly use
@Testbecause merely naming a methodtest...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.