On Thu, 19 Sep 2024 16:27:26 GMT, Jay Bhaskar <jbhas...@openjdk.org> wrote:
>> Successfully converted Non-parametrized base tests to JUnit 5
>
> Jay Bhaskar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revert "8339515: [TestBug] Convert web tests to JUnit 5"
>
> This reverts commit 86f73271142049a2c0162d987aee6bd0fcb2f82a.
I left a few (mostly minor) comments, a few suggestions, and three problems
that must be fixed:
1. You removed a total of 9 tests in three files:
Before: 5538 tests (0 failures, 28 ignored)
After: 5529 tests (0 failures, 28 ignored)
2. Three of the files that you converted still have some JUnit 4 imports
3. You missed converting the `expected=` parameter in the `@Test` annotation to
a `convertThrows` in `VetoableObservableListTest.java`.
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ContentBindingMapTest.java
line 104:
> 102: Bindings.bindContent(op2, op2);
> 103: }
> 104:
Why did you remove these test methods? They should be restored.
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ContentBindingSetTest.java
line 99:
> 97: public void testBind_X_Self() {
> 98: Bindings.bindContent(op2, op2);
> 99: }
These test methods should be restored.
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
line 49:
> 47: import static org.junit.jupiter.api.Assertions.assertThrows;
> 48:
> 49: import static org.junit.Assert.*;
This file still has a JUnit 4 import.
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
line 388:
> 386: public void createWithRootNull() {
> 387: assertThrows(NullPointerException.class, () -> {
> 388: Bindings.selectString(null, "next", "name");
Did you mean to remove the assignment? Since this is the last statement in the
test method (and it will throw anyway), it doesn't really matter, but it also
doesn't seem necessary to have made the change.
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/StringFormatterTest.java
line 195:
> 193: assertEquals(
> 194: "" + double0 + double1 + float0 + float1 + long0 + long1
> + int0 + int1 + boolean0 + boolean1 + string0 +
> 195: string1 + date0 + date1, s.get());
This looks identical (I hope) except the reformatting, which appears to be
unneeded. I recommend reverting the unchanged lines here and below.
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/StringFormatterTest.java
line 317:
> 315: }
> 316:
> 317: public class BindingsTest {
Why did you add an inner class here? It is unnecessary, and without an extra
annotation, it prevents the 3 tests that are inside it from running. Please
revert.
modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ListListenerHelperTest.java
line 47:
> 45: import static org.junit.Assert.assertEquals;
> 46: import static org.junit.Assert.assertFalse;
> 47: import static org.junit.Assert.assertTrue;
This file still has JUnit 4 imports.
modules/javafx.base/src/test/java/test/javafx/beans/property/adapter/ReadOnlyJavaBeanPropertyTestBase.java
line 70:
> 68: this.property = extractProperty(null);
> 69: }
> 70: catch (NoSuchMethodException e) {
Minor: the previous formatting -- `} catch (NoSuchMethodException e) {` --
matches our code style guidelines, and is preferred.
modules/javafx.base/src/test/java/test/javafx/binding/expression/AbstractNumberExpressionTest.java
line 53:
> 51: import javafx.beans.value.ObservableNumberValue;
> 52: import test.javafx.binding.DependencyUtils;
> 53: import javafx.collections.FXCollections;
This file still has JUnit 4 imports.
modules/javafx.base/src/test/java/test/javafx/collections/ModifiableObservableListBaseTest.java
line 207:
> 205: new ArrayList<>(List.of("a", "b", "c")),
> 206: new ArrayList<>(List.of("a", "b")))
> 207: .subList(0, 2);
There are only whitespace changes in this file, since it was already converted
to JUnit 5. I recommend to revert your changes.
modules/javafx.base/src/test/java/test/javafx/collections/VetoableObservableListTest.java
line 93:
> 91:
> 92: @Test
> 93: @Disabled
You need to replace the annotation with an `assertThrows` call in the method.
Otherwise, if the test ever gets re-enabled it will not do what we expect.
This same comment applies to several other methods in this class.
-------------
Changes requested by kcr (Lead).
PR Review: https://git.openjdk.org/jfx/pull/1576#pullrequestreview-2316745565
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767594945
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767597056
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767650479
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767599801
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767602205
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767609190
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767650863
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767626096
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767651424
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767676037
PR Review Comment: https://git.openjdk.org/jfx/pull/1576#discussion_r1767677538
