RFR: 8339511: [TestBug] Convert Non parametrized base tests to JUnit 5 [v2]

[Kevin Rushforth]

On Thu, 19 Sep 2024 16:27:26 GMT, Jay Bhaskar <jbhaskar at openjdk.org> wrote:

>> Successfully converted Non-parametrized base tests to JUnit 5
>
> Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Revert "8339515: [TestBug] Convert web tests to JUnit 5"
>   
>   This reverts commit 86f73271142049a2c0162d987aee6bd0fcb2f82a.

I left a few (mostly minor) comments, a few suggestions, and three problems that must be fixed:

1. You removed a total of 9 tests in three files:

Before: 5538 tests (0 failures, 28 ignored)

After: 5529 tests (0 failures, 28 ignored)

2. Three of the files that you converted still have some JUnit 4 imports

3. You missed converting the `expected=` parameter in the `@Test` annotation to a `convertThrows` in `VetoableObservableListTest.java`.

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ContentBindingMapTest.java line 104:

> 102:         Bindings.bindContent(op2, op2);
> 103:     }
> 104: 

Why did you remove these test methods? They should be restored.

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ContentBindingSetTest.java line 99:

> 97:     public void testBind_X_Self() {
> 98:         Bindings.bindContent(op2, op2);
> 99:     }

These test methods should be restored.

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java line 49:

> 47: import static org.junit.jupiter.api.Assertions.assertThrows;
> 48: 
> 49: import static org.junit.Assert.*;

This file still has a JUnit 4 import.

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java line 388:

> 386:     public void createWithRootNull() {
> 387:         assertThrows(NullPointerException.class, () -> {
> 388:             Bindings.selectString(null, "next", "name");

Did you mean to remove the assignment? Since this is the last statement in the test method (and it will throw anyway), it doesn't really matter, but it also doesn't seem necessary to have made the change.

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/StringFormatterTest.java line 195:

> 193:         assertEquals(
> 194:                 "" + double0 + double1 + float0 + float1 + long0 + long1 + int0 + int1 + boolean0 + boolean1 + string0 +
> 195:                         string1 + date0 + date1, s.get());

This looks identical (I hope) except the reformatting, which appears to be unneeded. I recommend reverting the unchanged lines here and below.

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/StringFormatterTest.java line 317:

> 315:     }
> 316: 
> 317:     public class BindingsTest {

Why did you add an inner class here? It is unnecessary, and without an extra annotation, it prevents the 3 tests that are inside it from running. Please revert.

modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ListListenerHelperTest.java line 47:

> 45: import static org.junit.Assert.assertEquals;
> 46: import static org.junit.Assert.assertFalse;
> 47: import static org.junit.Assert.assertTrue;

This file still has JUnit 4 imports.

modules/javafx.base/src/test/java/test/javafx/beans/property/adapter/ReadOnlyJavaBeanPropertyTestBase.java line 70:

> 68:                 this.property = extractProperty(null);
> 69:             }
> 70:             catch (NoSuchMethodException e) {

Minor: the previous formatting -- `} catch (NoSuchMethodException e) {` -- matches our code style guidelines, and is preferred.

modules/javafx.base/src/test/java/test/javafx/binding/expression/AbstractNumberExpressionTest.java line 53:

> 51: import javafx.beans.value.ObservableNumberValue;
> 52: import test.javafx.binding.DependencyUtils;
> 53: import javafx.collections.FXCollections;

This file still has JUnit 4 imports.

modules/javafx.base/src/test/java/test/javafx/collections/ModifiableObservableListBaseTest.java line 207:

> 205:                     new ArrayList<>(List.of("a", "b", "c")),
> 206:                     new ArrayList<>(List.of("a", "b")))
> 207:                     .subList(0, 2);

There are only whitespace changes in this file, since it was already converted to JUnit 5. I recommend to revert your changes.

modules/javafx.base/src/test/java/test/javafx/collections/VetoableObservableListTest.java line 93:

> 91: 
> 92:     @Test
> 93:     @Disabled

You need to replace the annotation with an `assertThrows` call in the method. Otherwise, if the test ever gets re-enabled it will not do what we expect.

This same comment applies to several other methods in this class.

-------------

Changes requested by kcr (Lead).

PR Review: https://git.openjdk.org/jfx/pull/1576#pullrequestreview-2316745565
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767594945
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767597056
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767650479
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767599801
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767602205
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767609190
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767650863
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767626096
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767651424
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767676037
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767677538