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...
first batch of comments.
will continue tomorrow from ObjectMethodsTestBase.java
modules/javafx.graphics/src/test/java/test/com/sun/javafx/font/PrismFontFactoryTest.java
line 65:
> 63: public void tearDown() {
> 64: }
> 65:
this is good.
modules/javafx.graphics/src/test/java/test/com/sun/javafx/sg/prism/DirtyRegionClipTest.java
line 66:
> 64: // We will populate this list with the parameters with which we
> will test.
> 65: // Each Object[] within the params is composed of a Creator and a
> Polluter.
> 66: Stream<Arguments> params = Stream.empty();
I think junit5 also accepts a `List<Arguments>`
modules/javafx.graphics/src/test/java/test/com/sun/javafx/sg/prism/DirtyRegionTestBase.java
line 76:
> 74: return Stream.concat(stream, Stream.of(arg));
> 75: }
> 76:
same comment: `List<Arguments>` might also work
(here and possibly elsewhere, I don't want to add duplicate comments)
modules/javafx.graphics/src/test/java/test/com/sun/javafx/sg/prism/DirtyRegionTestBase.java
line 333:
> 331: Math.min(expected.getMaxY() + 1, dirtyRegion.getMaxY()));
> 332: // Now make the check, and print useful error information in
> case it fails.
> 333: assertEquals(expected, dirtyRegion);
should probably avoid dropping creator/polluter from output
modules/javafx.graphics/src/test/java/test/com/sun/javafx/sg/prism/DirtyRegionTestBase.java
line 431:
> 429: private void
> assertOnlyTheseNodesWereAskedToAccumulateDirtyRegions(NGNode start,
> Set<NGNode> nodes) {
> 430: assertEquals(
> 431: "creator=" + creator + ", polluter=" + polluter,
same comment here (and possibly elsewhere)
one thing you could do is to retain instance variables, just set them in the
setup method - this should be safe, I don't think junit will invoke test
methods for the same instance from multiple threads.
modules/javafx.graphics/src/test/java/test/com/sun/javafx/test/CssMethodsTestBase.java
line 113:
> 111: }
> 112:
> 113: public static Arguments config(final Configuration configuration) {
I think this can be simplified: use Configuration directly, no need to wrap it
into Arguments.
But this code will work too.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1566#pullrequestreview-2313918810
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1765796331
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1765836676
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1765841780
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1765844768
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1765849330
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1765854608
