Migrate from JUnit 4 to JUnit 5 #959
Conversation
|
Click here to submit a new iteration when this PR is ready for review. See this repository's contribution guide for more information. |
sijie123
force-pushed
the
migration_junit5
branch
from
a7c83a7
to
f590e41
Compare
v1@sijie123 submitted v1 for review.
Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/959/1/head:BRANCHNAMEwhere |
|
@sijie123 Can we put everything into one single "migrate to JUnit5" PR if possible? Long ago, we introduced a Turns out, after the Lesson: We never know if a commit is actually required in accomplishing a goal, unless we actually accomplish that goal :-) |
Sure. In that case, I'll make this PR the gigantic one then. |
sijie123
changed the title
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
We don't use deprecated because we are going to replace all usages of this API with the newer one and remove this method from the codebase :P
There was a problem hiding this comment.
Yup. Will remove the old version once I finish migrating all the API calls. Then I will rebase the Assert.java on top.
| */ | ||
| public static void assertThrows(Class<? extends Throwable> expected, VoidCallable callable) { | ||
| assertThrows(expected, null, callable); | ||
| public static void assertThrows(Class<? extends Throwable> expectedType, Executable executable, |
There was a problem hiding this comment.
Sorry, I think I wasn't clear in my earlier reply. What I meant was: We just need:
assertThrows(Class<? extends Throwable> expectedType, Executable executable)which you have implemented.assertThrows(Class<? extends Throwable> expectedType, Executable executable, String expectedMessage)
There's no need for this method because we don't use it; the purpose of this PR is to simplify existing code upon upgrading the JUnit version.
There was a problem hiding this comment.
We don't need (2)? I thought it's quite commonly used in AB4.
For example:
I was thinking of putting them all in one place under Assert.java
There was a problem hiding this comment.
We need (2), but we don't need this :P
What this method is doing is what you have said earlier:
messageis the message to be displayed when the test fails.
Which we don't use in our existing codebase.
There was a problem hiding this comment.
Oh. I see. I saw some code that used it, but it's okay, I'll probably just write a function only in the one or two places that use it.
Or perhaps just remove the message out right.
sijie123
force-pushed
the
migration_junit5
branch
7 times, most recently
from
36056a1
to
c8d460e
Compare
|
I'm ready for the onslaught... or maybe not :P Jokes aside, I would need help with organising my commits. I've tried to separate them as much as possible, although it's still not very ideal. For example, adding support for TempDir relies on using JUnit 5 Runner API, and JUnit 5 runner API won't support the old TemporaryFolder rule, so I'm not sure how to split the concerns in this case. |
There was a problem hiding this comment.
[1/16]
Assert: Use JUnit 5 assertions Currently, Assert implements our custom assertThrows using try/catch.
Yes, that's true.
This implementation was done when we used JUnit 4 which has no proper support for assertThrows. As we migrate to JUnit 5[1], we can make use of JUnit 5's Assertions.assertThrows.
Agreed.
However, our current assertThrows supports checking the correctness of error's message, whcih JUnit 5 does not support.
whcih -> which
And yeah, that seems to be true.
Let's * migrate Assert#assertThrows to use JUnit 5's assertThrows. * Remove VoidCallable interface within Assert
Huh? Why do we need to do this?
(At this point I actually skipped straight to the diff to find out)
* create an overloaded assertThrows(Class<? extends Throwable>, String, Executable) to support checking the correctness of error messages.
Huh, why?
* standardise to use only seedu.address.testutil.Assert#assertThrows instead of org.junit.jupiter.api.Assertions.assertThrows
Looking at your diff, this standardization turned out to be pretty hairy, and
would likely benefit from being split out into separate commits. See the end of this review comment.
By creating an overloaded assertThrows method, we can extract the common routine of checking error message into a single utility function within Assert.java. This encourages reusability and follows the DRY principle. In addition, by using org.junit.jupiter.api.function.Executable that already accepts lambda expressions, we no longer need the VoidCallable interface previously. Hence, we can remove the unused VoidCallable interface.
OK, this finally answers my questions above.
Alright, I had a fair bit of trouble grokking the commit message, and I think
it's because the explanations are too far away from the "What I am doing". When
reading what you are trying to do with this commit, the (skeptical) reader
would naturally immediately ask "Why?", and in that case it would be helpful to
immediately follow with the rationale.
Something like this:
Currently, Assert implements our custom assertThrows using try/catch.
This implementation was done when we used JUnit 4 which had no proper
support for assertThrows. As we migrate to JUnit 5[1], we can make use
of JUnit 5's Assertions.assertThrows.
As such, let's migration the implementation of Assert#assertThrows to use JUnit
5's assertThrows.
This is not a straightforward migration, however. This is because
Assert#assertThrows supports checking the correctness of the exception's error
message, which JUnit5's assertThrows does not support.
Thus, we ...
Now, given the commit message that I have outlined above, I would expect the
diff of this commit to just be changes in testutil/Assert.java -- to migrate
our own homegrown assertThrows(...) implementation to use JUnit's
assertThrows. Since our API remains the same, there should not be any other
changes to our test code.
Looking at the diff, it seems we are doing more than that though.
One common pattern I see is that we are turning
thrown.expect(IllegalArgumentException.class);
Version.fromString("This is not a version string");into
assertThrows(IllegalArgumentException.class, () ->
Version.fromString("This is not a version string")
);which seems like it deserves its own commit.
There is also a pretty hairy diff in LogicManagerTest.java that does not
seem that straightforward to me. Maybe some explanation in a commit message would help?
This standardization seems hairy enough that it might even deserve its own PR.
There was a problem hiding this comment.
Great job on this PR so far! :) It's really great work for a first-timer, especially on such a large PR.
I agree with Paul that the first commit seems to be doing too much. I think this can be a separate PR, one of the reasons being that we can be more or less assured that this gets merged before the semester ends.
I see the following changes which can be broken up into individual commits (I may not have included everything, so don't just follow):
- Migrate
Assert#assertThrowsto use JUnit 5'sassertThrows ConfigTest: remove unusedExpectedException- Replace
VoidCallablewithAssert#assertThrows - Replace
thrown.expect()withassertThrows - Standardise
assertThrowsto be statically imported - Update
assertCommandBehaviortoassertCommandFailure
You can work on the next iteration. :)
| assertExceptionThrown(IllegalArgumentException.class, "typical sentence", "aaa BBB", | ||
| Optional.of("Word parameter should be a single word")); | ||
| assertThrows(IllegalArgumentException.class, "Word parameter should be a single word", () | ||
| -> StringUtil.containsWordIgnoreCase("typical scenario", "aaa BBB")); |
There was a problem hiding this comment.
scenario -> sentence :P
| @@ -70,7 +66,7 @@ public void execute_commandExecutionError_throwsCommandException() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void execute_validCommand_success() { | |||
| public void execute_validCommand_success() throws Exception { | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Thanks @pyokagan and @Zhiyuan-Amos for your reviews. I'll start by splitting up the very first commit (Assert: Use JUnit 5 assertions). During this process, I will probably split up some parts which I find less relevant to the migration, into other separate PRs. |
|
Hi @pyokagan and @Zhiyuan-Amos In JUnit 5, there's no longer support for extensions. So both TemporaryFolder and UiPartRule/StageRule need to go. I thought it'd be too large a commit to change both things at once, but I can't change one without breaking the other. I'm not sure how to break this down into a smaller, "single responsibility" change. Can I get some advice on this? |
Didn't verify this, but if this is indeed true, then make these changes in a single commit and note it down in your commit message. Your commit message can look something like: (I copied the 1st 2 paras from 10/16's commit message). Edit: Take a look at this commit, it's similar to what you have to do. |
sijie123
force-pushed
the
migration_junit5
branch
from
c8d460e
to
a88dfb5
Compare
I think it should be done as a preparatory PR or in this PR itself. It would not be correct if, after a PR called "Migrate from JUnit 4 to JUnit 5" is merged, a JUnit4 dependency still remains. |
src/test/java/seedu/address/storage/JsonAddressBookStorageTest.java
Outdated
Show resolved
Hide resolved
src/test/java/seedu/address/storage/JsonUserPrefsStorageTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks, except for some stylistic nits (maybe check the postimage of this PR to see if there are any other cases of double empty lines introduced?) I think this is nearly ready to go. (Plus we need to fix the testfx-junit dependency problem as a preparatory PR or in this PR itself)
We use CheckStyle to check and uphold some of our coding standards. While CheckStyle works properly for majority of our code, some test APIs work differently. For example, it is necessary to declare public variables in the case of: @rule public TemporaryFolder testFolder = new TemporaryFolder(); To suppress CheckStyle warnings for these "poor" coding practice, we allow exceptions when we detect certain annotations. For example, anything immediately succeeding a @rule tag can be declared as public. As we migrate to JUnit 5, we need to add exceptions for the equivalent test annotations used in JUnit 5 [1]. For example, BeforeClass is now BeforeAll. JUnit 5 also introduces new annotations such as the TempDir and RegisterExtension annotation. Hence, let's * add JUnit 5 annotations such as BeforeAll/BeforeEach to allowedAnnotations in Javadoc module * add TempDir and RegisterExtension annotation to ignoreAnnotationCanonicalNames in VisibilityModifier module. Keeping in mind that some tests are still written in JUnit 4 style during this transition phase, when adding the TempDir and RegisterExtension annotations, we cannot remove exceptions for existing annotations (such as Rule and ClassRule). Hence, in the VisibilityModifier module, when we override the default value to add TempDir and RegisterExtension, we also need to add Rule and ClassRule tags which are in use by existing code [2]. [1] As of March 2019, http://checkstyle.sourceforge.net/config_javadoc.html#JavadocMethod does not have JUnit 5 annotations. [2] Default modifiers are Rule, ClassRule and VisibleForTesting. http://checkstyle.sourceforge.net/config_design.html#VisibilityModifier
Assert contains a custom assertThrows method. This implementation was done when we used JUnit 4 which has no proper support for assertThrows. This is now unnecessary as JUnit 5's Assertions.assertThrows performs the same functionality. Let's replace Assert#assertThrows with JUnit 5's assertThrows. With this change, VoidCallable becomes redundant as its behaviour is replicated with JUnit 5's Executable interface. Let's remove it. [1] https://github.com/junit-team/junit5/blob/5e41ebe612fd67e905f96f1dd3184a071b65be17/junit-jupiter-api/src/main/java/org/junit/jupiter/api/function/Executable.java
We use JUnit 4's Assert API. For example, in most classes, we import org.junit.Assert.assertFalse or org.junit.Assert.assertEquals. In JUnit 5, the API has migrated to org.junit.jupiter.api.Assertions.*. While the functionality remains largely similar, JUnit 4 APIs are no longer supported by the JUnit 5 runner (unless a legacy Vintage engine is used [1]). Thus, it is necessary to use JUnit 5 APIs as we migrate our tests to JUnit 5. Let's migrate our API calls to assertion statements to use the ones provided by JUnit 5 instead. This can be done by changing all references of org.junit.Assert.* to org.junit.jupiter.Assertions.*. In this migration, some methods signatures such as assertTrue(String, Boolean) has been changed to assertTrue(Boolean, String). The relevant method calls have been updated to reflect this signature change. [1]: https://junit.org/junit5/docs/current/user-guide/#migrating-from-junit4
We write our tests using the JUnit 5.1.0 API. The JUnit 5.1.0 API contains most features we require, such as assertTrue or assertThrows. We have unit tests such as StorageManagerTest#prefsReadSave that require writing to the filesystem. These tests need to make use of temporary directories. This is so that (1) we can begin with a clean, known state at the start of each test and (2) we can prevent our test output from polluting the user's file system. However, JUnit 5.1.0 does not support creating temporary directories natively, so developers need to create a custom TemporaryDirectory implementation or rely on JUnit-Pioneer library [1]. Both of these solutions make it complicated to implement and for future developers to understand. JUnit 5.4.0 is the first version of JUnit that has a native temporary directory API [2]. By upgrading to JUnit 5.4.0, we can directly use the org.junit.jupiter.api.io.TempDir API. This reduces the complexity of implementing a temporary directory. Let's upgrade to use JUnit 5.4.0. [1] https://medium.com/@GalletVictor/migration-from-junit-4-to-junit-5-d8fe38644abe [2] junit-team/junit5#1247
We use JUnit 4 test runner (org.junit.Test) to execute our unit and system testing. To use new features such as callbacks, extensions and temporary directories in JUnit 5, we need to use the new JUnit 5 test runner. Let's * use the new org.junit.jupiter.api.Test instead of org.junit.Test * use BeforeEach instead of Before * use BeforeAll instead of BeforeClass * use AfterEach instead of After * use AfterAll instead of AfterClass Note: Tests that involve TemporaryFolders, JavaFX, UiPartRule, StageRule and ClockRule will be migrated in another commit, so as to limit the scope of each commit.
We use JUnit 4's TemporaryFolder rule to initialise a temporary folder. Rules are no longer supported by JUnit 5 test runners. In addition, JUnit 5.4.0 introduces the TempDir extension that supersedes the TemporaryFolder rule. Let's migrate our usage of TemporaryFolder rules to TempDir. As TempDirs in JUnit 5 are Paths, it is no longer necessary to invoke methods like getRoot().toPath() on the TempDir. Additionally, the way to get a URL resource from the TempDir is also different from TemporaryFolder. The relevant methods have been updated. In LogicManagerTest and MainWindowCloseTest, the newFile() API is not supported by JUnit 5's TempDir. However, as the random file name provided by newFile() is not necessary (we have full control over the contents of the temporary directory), instead of re-implementing newFile() to match the behavior of existing tests, we explicitly give the file names and use Path.resolve() instead.
…xtension We use JUnit 4 style tests in the UI segment of unit tests. JUnit 5 offers a more declarative and extension-based unit test, to encourage reuse of components common to all tests. As we migrate towards JUnit 5, it is essential that we migrate the UI unit tests to adopt JUnit 5's style. Most importantly, JUnit 5 replaces Rules with Extensions, hence we need to migrate UiPartRule to UiPartExtension and StageRule to StageExtension. Let's replace UiPartRule with UiPartExtension, and StageRule with StageExtension, taking advantage of JUnit 5's extensive hook interface (BeforeEachCallback and AfterEachCallback) to simplify our implementation of hooks. Notably, we no longer need to implement our own before/hook/after sequence. However, this migration introduces a complication. As extensions are specific to JUnit 5, we cannot use extensions without switching the test runner to JUnit 5's Jupiter test runner. However, this switch means that we also need to migrate other API changes. Notably, * Before/BeforeClass becomes BeforeEach/BeforeAll * After/AfterClass becomes AfterEach/AfterAll Furthermore, HelpWindowTest needs to continue skipping the test in headless mode, due to a bug (still yet to be fixed) in JavaFX. The equivalent API in JUnit 5 is located in org.junit.jupiter.api.Assumptions.*, so in order to continue skipping the test, we need to migrate the call and update its method signature as necessary.
We use ClockRule to implement a clock for unit testing. Rules are part of JUnit 4, and have been replaced by extensions in JUnit 5. As we migrate to JUnit 5, we should update ClockRule to follow the new naming convenion and use JUnit 5 extension style. Let's rewrite ClockRule to implement an injectable clock using JUnit5 callbacks. With a ClockExtension now migrated to JUnit 5 API, we need to update AddressBookSystemTest to use the JUnit 5 runner too, so that the ClockExtension can be activated. As a result, all system tests that extend from AddressBookSystemTest also need to be updated to use the JUnit 5 runner. Migration to the JUnit 5 runner means that we also need to update other API changes such as: * Before/BeforeClass becomes BeforeEach/BeforeAll * After/AfterClass becomes AfterEach/AfterAll * Rules are no longer supported, extensions are registed with RegisterExtension instead.
Checkstyle contains exceptions to allow certain exceptions in coding style for JUnit 4 tests. For example, Checkstyle allows declaration of a public variable if it is preceded by a Rule tag so as to ensure correct variable assignment by JUnit 4. After migrating all tests to JUnit 5, we no longer need these exceptions for annotations specific to JUnit 4. Hence, we can remove these annotations to tighten our strictness on coding style. Hence, let's remove exceptions in checkstyle for JUnit 4 annotations. Notably, let's remove * Before/BeforeClass/After/AfterClass tags used to denote functions to run before/after a test * Rule/ClassRule tags used to denote rules (now extensions in JUnit 5)
Currently, we use a mix of both JUnit 4 and 5 libraries, using JUnit Vintage to maintain compatibility with JUnit 4 tests. After migrating all tests to JUnit 5, we no longer need these libraries to support JUnit 4. Hence, we can remove these libraries from build.gradle to reduce the number of unncessary components, thus streamlining our codebase. Let's remove JUnit 4 test and support libraries from build.gradle.
sijie123
force-pushed
the
migration_junit5
branch
from
d36dc1c
to
cc73829
Compare
|
After this PR, there's still 3 more places (StringUtilTest, ConfigUtilTest, ConfigTest) where there are still empty lines. I couldn't find a good place to put in this PR, as I didn't really touch the code around the blank lines in my commits. Maybe after this PR, I can do a small PR to fix these blank lines separately. |
v13@sijie123 submitted v13 for review.
(📚 Archive) (📈 Interdiff between v12 and v13) (📈 Range-Diff between v12 and v13) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level4.git refs/pr/959/13/head:BRANCHNAMEwhere |
There was a problem hiding this comment.
Good work @sijie123
Thanks for the guidance @Zhiyuan-Amos and @pyokagan
pyokagan
changed the title
Our tests use JUnit4.
JUnit 5 provides many APIs that simplify our tests. For example, instead of a lengthy try/catch procedure to test for correct exception handling behaviour, we can now use assertThrows. This makes it easier for readers to understand our code, and to implement more tests.
Let's upgrade to use JUnit 5.
Resolves #874
Progress/To-do:
[x] Change imports to JUnit5 version. (e.g. org.junit.jupiter.api.Test instead of org.junit.Test)
[x] Remove Test being used for specifying expectations. We use this to specify timeout in PersonListPanelTest. Instead we should now use assertTimeoutPreemptively. (Done in #863)
[x] Rename @before to @beforeeach
[x] Rename @after to @AfterEach
[x] Rename @BeforeClass to @BeforeAll
[x] Rename @afterclass to @afterall
[-] Group assertions together using assertAll for related assertions (Not sure if we want to do this in this PR? Leaving it as separate asserts right now as feedback to user is clearer when tests fail)
[x] Remove @rule and @ClassRule. Instead, we need to @ExtendWith, which generally requires creating an extension class.