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

[Andy Goryachev]

On Thu, 3 Apr 2025 12:34:37 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.

First batch of comments (up until `ObservableArrayTest`) - the general direction is good, but I would suggest to re-work the set up in all the parameterized tests, mainly for simplicity and consistency.

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/BidirectionalBindingTest.java line 73:

> 71:     private T[] v;
> 72: 
> 73:     public void BidirectionalBindingTest_(Factory<T> factory) {

general comment:
1. the factory (or any other parameterized argument or arguments) should be passed to setUp() in each parameterized test
2. the setUp() should be made private (unless it's overriden by the child class)
3. what used to be the constructor should be removed

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/BidirectionalBindingTest.java line 77:

> 75:     }
> 76: 
> 77:     //@BeforeEach

this comment can be removed

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/BidirectionalBindingWithConversionTest.java line 68:

> 66:     private PropertyMock<T> op1;
> 67: 
> 68:     public void BidirectionalBindingWithConversionTest_(Functions<S, T> func, S[] v0, T[] v1) {

same thing, rename this method to setup() or something like that and make it private

modules/javafx.base/src/test/java/test/com/sun/javafx/event/EventDispatchChainTest.java line 51:

> 49:                 Arguments.of(EventDispatchChainImpl.class),
> 50:                 Arguments.of(EventDispatchTreeImpl.class)
> 51:         );

even though the return value is ok (`Stream<Arguments>`) the parameter supplier method can simply return a List of `Class<? extends EventDispatchChain>` - the framework is smart enough to figure it out.

this is just FYI, no need to make the change.

modules/javafx.base/src/test/java/test/javafx/beans/property/adapter/JavaBeanPropertyTestBase.java line 54:

> 52:     protected abstract JavaBeanProperty<T> extractProperty(Object bean) throws NoSuchMethodException;
> 53: 
> 54:     @BeforeEach

private?

modules/javafx.base/src/test/java/test/javafx/beans/property/adapter/JavaBeanPropertyTestBase.java line 68:

> 66:         assertThrows(NullPointerException.class, () -> {
> 67:             this.property = extractProperty(null);
> 68:         });

my first though about this was - what about `NoSuchMethodException`?
then I realized that in this case the test will fail, and it will similarly fail if any other exception gets thrown.
all good.

modules/javafx.base/src/test/java/test/javafx/binding/BindingsCreateBindingTest.java line 75:

> 73:     }
> 74: 
> 75:     public void BindingsCreateBindingTest_(Property<T> p0, Property<T> p1, Functions<T> f, T value0, T value1, T defaultValue) {

it's better to rename the method to something like `setup()` and make it `private`.

modules/javafx.base/src/test/java/test/javafx/binding/BindingsEqualsTest.java line 75:

> 73:     private InvalidationListenerMock observer;
> 74: 
> 75:     public void BindingsEqualsTest_(ObservableValue op1, ObservableValue op2, Functions<T> func, T... v) {

move the functionality to `setUp()`

modules/javafx.base/src/test/java/test/javafx/binding/BindingsNumberCalculationsTest.java line 71:

> 69:     private InvalidationListenerMock observer;
> 70: 
> 71:     public void BindingsNumberCalculationsTest_(ObservableValue op1, ObservableValue op2, Functions<T> func, T[] v) {

move the functionality to `private setup(...)`?

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

> 77:     private IntegerProperty integer1;
> 78: 
> 79:     @BeforeEach

private?

modules/javafx.base/src/test/java/test/javafx/binding/GenericBindingTest.java line 66:

> 64:     private ChangeListenerMock<Object> changeListener;
> 65: 
> 66:     public void GenericBindingTest_(

move the functionality to `private setUp()`?

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

PR Review: https://git.openjdk.org/jfx/pull/1759#pullrequestreview-2740196090
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027184146
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027184640
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027187159
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027191526
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027199933
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027207548
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027209316
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027211638
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027215741
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027225296
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027232870