On Fri, 13 Sep 2024 15:14:39 GMT, Lukasz Kostyra <lkost...@openjdk.org> wrote:
> This PR converts all tests in `modules/javafx.graphics` to use JUnit5.
>
> ## Details
>
> Trivial changes resolved by first four commits:
> - Import changes to use `org.junit.jupiter` package instead of `org.junit` or
> `junit.framework` and all changes that follow it (ex. `@Before` ->
> `@BeforeEach`)
> - Message Strings in `assert*` calls moved to the end to follow 5's argument
> order
> - Some simple Parametrized tests (aka. those which only did a straightforward
> assignment of test parameters) adjusted to use JUnit5's `@ParametrizedTest`
> and `@MethodSource` annotations
> - Some test formatting unification - keeping annotations on a separate line
> from method definition
> - Some tests used `@Rule` annotation which is missing from JUnit5. Luckily,
> all those uses just set `ExpectedException thrown = ExpectedException.none()`
> and then mid-test set it to some expected exception class. This was easily
> replaceable with `assertThrows`.
>
> Non-trivial changes:
> - Mutliple tests use common test bases and derive from it to check specific
> behaviors. Those include:
> - `DirtyRegionTestBase`
> - `CssMethodsTestBase`
> - `ObjectMethodsTestBase`
> - `OnInvalidateMethodsTestBase`
> - `PropertiesTestBase`
> - `ServiceTestBase` & derivatives required some rework due to
> `ServiceExceptionTest` being a derived parametrized class while other
> `ServiceTestBase` were not parametrized
> - `TransformOperationsTest` not only expanded 3 test parameters onto 6
> different test class members, but also exposed its `getParams()` call to
> other testers - `AffineOperationsTest` and `TransformUtilsTest`
> - Other classes used `@BeforeEach` annotation which accessed test parameters,
> those setup methods were called manually in-test as per discussion in #1561
>
> At the end I added a change converting `assertTimeout` uses I added to
> `@Timeout` annotation, as recommended in other PRs.
>
> As for conversion practices and methods, most of these were discussed as part
> of #1561 so I will skip them here.
>
> ## Results
>
> Below are results of the test conversion and runs of `gradle --info
> :graphics:test` on my machine:
>
> | Category | JUnit4 ( 4647367ce ) | JUnit5 (this PR) |
> | ------------ | ------ | ------ |
> | Test count | 23272 | 23275 |
> | Failures | 0 | 0 |
> | Ignored | 99 | 102 |
> | Successful % | 100% | 100% |
>
> The difference in test count and ignored comes from
> `Popup_parentWindow_Test.java` where the entire test class was annotated as
> `@Ignored`. After conversion, the `@Disabled` annotation applies to each test
> individually rather than t...
modules/javafx.graphics/src/test/java/test/javafx/scene/TreeShowingPropertyTest.java
line 68:
> 66: };
> 67:
> 68: return Arrays.asList(new Object[][] { { supplier1 }, { supplier2
> } });
the change may not be fully equivalent: the use of `Supplier` defers the moment
of creation until each test is started.
it is very likely that the parameters() method is called once for each test,
creating all the instances before any of the tests cases are executed.
this still might be ok because
a) the tests are headless
b) the nodes are not inserted into the scene graph
c) instances are not reused anywhere
modules/javafx.graphics/src/test/java/test/javafx/scene/effect/BoxBlur_properties_Test.java
line 44:
> 42: public final class BoxBlur_properties_Test extends PropertiesTestBase {
> 43:
> 44: public static Stream<Arguments> data() {
you _could_ return the `List<Arguments>` directly
modules/javafx.graphics/src/test/java/test/javafx/scene/effect/EffectInputTest.java
line 71:
> 69: }
> 70: }
> 71: return stream;
(here and elsewhere, you could return the List directly)
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1767383945
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1767389182
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1767392303
