RFR: 8340464: [TestBug] Convert parametrized base tests to JUnit 5 [v5]

[Andy Goryachev]

On Fri, 4 Apr 2025 06:08:28 GMT, Jay Bhaskar <jbhaskar at openjdk.org> wrote:

>> Migrated JUnit 4 tests in the jafax.base module to JUnit 5, replacing deprecated APIs, updating assertions, and refactoring test structures to align with JUnit 5's improved features.
>
> Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   re-order imports

Found some issues, please take a look.

modules/javafx.base/src/test/java/test/javafx/binding/BindingsNumberCastTest.java line 96:

> 94:     void testBindings(Functions func) {
> 95:         this.func = func;
> 96:         setUp(); // Call setup before each parameterized run

let's apply the same treatment here (for the sake of any possible future tests):
- remove `@BeforeEach` from `setUp()`
- add `(Function func)` argument to `setUp()`
- probably can remove the comment here or move it to `setUp()`

(I like how you combined multiple tests in one, it's ok in this case, as we don't expect these tests to fail intermittently or fail altogether.  Generally speaking, we should not be doing that as a failure in one use case would mask any possible failures that might happen down the line.)

modules/javafx.base/src/test/java/test/javafx/collections/ObservableListEmptyTest.java line 44:

> 42: public class ObservableListEmptyTest {
> 43: 
> 44:     static List<String> EMPTY = Collections.emptyList();

`EMPTY` can be final

modules/javafx.base/src/test/java/test/javafx/collections/ObservableListEmptyTest.java line 60:

> 58:     }
> 59: 
> 60:     public void setUp(Callable<ObservableList<String>> listFactory) throws Exception {

private?

modules/javafx.base/src/test/java/test/javafx/collections/ObservableListEmptyTest.java line 61:

> 59: 
> 60:     public void setUp(Callable<ObservableList<String>> listFactory) throws Exception {
> 61:         listFactory = listFactory;

Look carefully: this code has a big issue.

Interestingly, I've seen code in production that had issues like this one that went unnoticed for years.  

A good IDE would warn here (Eclipse does).

Should be

`this.listFactory = listFactory;`

modules/javafx.base/src/test/java/test/javafx/collections/ObservableListTest.java line 68:

> 66: 
> 67:     private void setUp(Callable<ObservableList<String>> listFactory) throws Exception {
> 68:         listFactory = listFactory;

same issue: should be 

`this.listFactory = listFactory;`

modules/javafx.base/src/test/java/test/javafx/collections/ObservableSetTest.java line 62:

> 60: 
> 61:     private void setUp(Callable<ObservableSet<String>> setFactory) throws Exception {
> 62:         setFactory = setFactory;

and this is the third place

`this.setFactory = setFactory;`

modules/javafx.base/src/test/java/test/javafx/collections/ObservableSubListIteratorTest.java line 61:

> 59:     private Callable<? extends List<String>> listFactory;
> 60:     private List<String> list;
> 61:     private ListIterator<String> iter;

this is likely incorrect:

`listFactory`, `list`, and `iter` are present in the base class.  please remove from here.

modules/javafx.base/src/test/java/test/javafx/util/converter/DateStringConverterTest.java line 108:

> 106:         this.dateFormat = dateFormat;
> 107: 
> 108:         if (dateFormat != null) {

what was the reason for this code (LL108 - 114) removal?
and for the removal of the tests from L136?

this removal also makes `private DateFormat validFormatter;` unused

modules/javafx.base/src/test/java/test/javafx/util/converter/DateTimeStringConverterTest.java line 104:

> 102:     private DateFormat validFormatter;
> 103: 
> 104:     public DateTimeStringConverterTest(DateTimeStringConverter converter, Locale locale, int dateStyle, int timeStyle, Date validDate, String pattern, DateFormat dateFormat) {

Is there a reason you did not follow the conversion pattern using setup() method?
I think the test is mangled now.  Could you please double check?

modules/javafx.base/src/test/java/test/javafx/util/converter/DateTimeStringConverterTest.java line 216:

> 214:             DateFormat dateFormat = DateTimeStringConverterShim.getDateFormatVar(converter);
> 215:             assertNotNull(dateFormat);
> 216:         }

please double-check: the code LL213-L126 looks misplaced here.  it used to be a part of the constructor, and in this case should probably precede the assertions.

modules/javafx.base/src/test/java/test/javafx/util/converter/ParameterizedConverterTest.java line 41:

> 39: 
> 40:     static Stream<Arguments> converterClasses() {
> 41:         return Stream.of(

FYI: I think you could simply return a `List<Class<? extends StringConverter<?>>>`, but this code is ok too.

modules/javafx.base/src/test/java/test/javafx/util/converter/TimeStringConverterTest.java line 149:

> 147: 
> 148:     @ParameterizedTest
> 149:     @MethodSource("provideConvertersForGetDateFormat")

ooh, using two different `@MethodSource`s, nice.

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

Changes requested by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1759#pullrequestreview-2743583859
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029121644
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029133625
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029134174
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029136424
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029145526
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029148630
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029152054
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029162216
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029167960
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029166902
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029176382
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2029179457