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.
Providing a few minor comments.
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) {
may be change method name to `setFactory`.
modules/javafx.base/src/test/java/test/com/sun/javafx/property/adapter/PropertyDescriptorTest.java
line 25:
> 23: * questions.
> 24: */
> 25: //test is disabled already
No other change in the file apart from this comment, should be reverted.
modules/javafx.base/src/test/java/test/com/sun/javafx/property/adapter/ReadOnlyPropertyDescriptorTest.java
line 25:
> 23: * questions.
> 24: */
> 25: //test is disabled already
No other change in the file apart from this comment, should be reverted.
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
line 28:
> 26: package test.javafx.util.converter;
> 27:
> 28: // Imports remain the same, except JUnit 4 imports are replaced with
> JUnit 5
Comment not required, should revert.
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
line 53:
> 51: public class LocalDateTimeStringConverterTest {
> 52:
> 53: // Constants and enums remain the same
Comment not required, should revert.
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
line 122:
> 120: if (version.major() < 20) {
> 121: // TODO: This can be removed when the minimum version of
> boot jdk
> 122: // for JFX build is updated to JDK20 or above.
The comments removed from this method, should be retained.
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
line 131:
> 129: @AfterClass
> 130: public static void teardownAfterAll() {
> 131: // Restore VM's old locale
The comments removed from this method, should be retained.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1759#pullrequestreview-2739878525
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027061383
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027016199
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027016487
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027038675
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027040164
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027044122
PR Review Comment: https://git.openjdk.org/jfx/pull/1759#discussion_r2027044519
