Upgrade JUnit4 tests to JUnit5

[Rinder5]

Our tests use JUnit4. Let's upgrade them to use JUnit5 instead.

• Change imports to JUnit5 version. (e.g. org.junit.jupiter.api.Test instead of
  org.junit.Test)
• Remove Test being used for specifying expectations. We use this to
  specify timeout in PersonListPanelTest. Instead we should now use
  assertTimeoutPreemptively
• Rename @Before to @BeforeEach
• Rename @After to @AfterEach
• Rename @BeforeClass to @BeforeAll
• Rename @AfterClass to @AfterAll
• Group assertions together using assertAll for related assertions
• Remove @Rule and @ClassRule. Instead, we need to
  @ExtendWith, which generally requires creating an extension class.

Mentor: @Zhiyuan-Amos
Assigned to: @sijie123

[sijie123]

While migrating from JUnit 4 to 5, I ran into a small issue...
In JUnit 5, assertThrows only checks that it is of the correct type.
If we want to assert the error message, we have to take the exception re-thrown by assertThrows and do comparisons ourselves. As this routine is commonly used, I feel that a custom method within our custom Assert class is the best way to go.
However, this means that I have to import static both Junit's assertThrows (the plain one), as well as our Assert.assertThrows (the enhanced one) in test classes that use both types of assertions (e.g. AppUtilTest).
I'm not sure if this is a good idea, since it will be confusing for readers -- why are we importing assertThrows from 2 places?

The alternative is that I add a wrapper to the plain assertThrows in our custom Assert. However, we will still import things like AssertEquals or AssertTrue from JUnit, which becomes the same problem as above.

Any suggestions?

[sijie123]

Looking through git blame for existing Assert class,
I think the reason why the current Assert class exists with 2 methods is because earlier versions of JUnit didn't have good support for assertThrown, hence we implemented our own versions.

[Zhiyuan-Amos]

In JUnit 5, assertThrows only checks that it is of the correct type. If we want to assert the error message, we have to take the exception re-thrown by assertThrows and do comparisons ourselves.

Does the following assertThrows API work?

public static <T extends Throwable> T assertThrows(Class<T> expectedType,
                                                   Executable executable,
                                                   String message)

[sijie123]

Does the following assertThrows API work?

Nope, message is the message to be displayed when the test fails.
(The JUnit guys need to do something about their documentation :P)

[sijie123]

Additionally, I was thinking of migrating to JUnit 5.4.0. It's currently on release candidate, hopefully in a couple of weeks when I finish migrating everything it will become stable...
I'm inclined to use JUnit 5.4.0 even though its RC because JUnit will only supporting TemporaryFolder in JUnit 5.4.0 onwards (Source)
TemporaryFolder currently belongs to JUnit-Pioneer, which is another module/extension to include and would only further complicate things.

[pyokagan]

I'm inclined to use JUnit 5.4.0 even though its RC because JUnit will only supporting TemporaryFolder in JUnit 5.4.0 onwards (Source)
TemporaryFolder currently belongs to JUnit-Pioneer, which is another module/extension to include and would only further complicate things.

If possible, I'd like to unify our use of TemporaryFolder and the "sandbox directory" into one single mechanism.

[Zhiyuan-Amos]

Nope, message is the message to be displayed when the test fails. (The JUnit guys need to do something about their documentation :P)

Haha I just realized, actually, this design is consistent with the rest of the assertX methods.

If we want to assert the error message, we have to take the exception re-thrown by assertThrows and do comparisons ourselves. As this routine is commonly used, I feel that a custom method within our custom Assert class is the best way to go.

Yes, good line of thought 👍

The alternative is that I add a wrapper to the plain assertThrows in our custom Assert. However, we will still import things like AssertEquals or AssertTrue from JUnit, which becomes the same problem as above.

If I'm reading rightly, I think this is a non-issue, since the current code base also uses custom assertThrows and assertEquals / assertTrue from JUnit (the only difference being that in our custom Assert class, we will have to import assertThrows from JUnit).

I think your suggestion helps to improve clarity in addressing the issue of "why are we importing assertThrows from 2 places?" We can write header comments to explain that we are wrapping the assertThrows(Class<T> expectedType, Executable executable) for the purpose of clarity.

[sijie123]

If possible, I'd like to unify our use of TemporaryFolder and the "sandbox directory" into one single mechanism.

Yes, that was one qualm I had when I did AddressBook for my CS2103T. I'm definitely going to look into how we can unify these two things.

We can write header comments to explain that we are wrapping the assertThrows(Class expectedType, Executable executable) for the purpose of clarity.

Yea, this sounds good.