On Fri, 4 Apr 2025 06:08:28 GMT, Jay Bhaskar <[email protected]> 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
