On Thu, 3 Apr 2025 12:34:37 GMT, Jay Bhaskar <jbhas...@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
