On Fri, 20 Sep 2024 13:31:06 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...
>
> Lukasz Kostyra has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review comments
Found some things that may should be improved, but looks good otherwise!
modules/javafx.graphics/src/test/java/test/javafx/scene/Node_bind_Test.java
line 57:
> 55: @Test
> 56: public void testIllegalClip() {
> 57: Thread.currentThread().setUncaughtExceptionHandler((thread,
> throwable) -> {
I think this needs to be reset in an `@AfterEach`, see for example the
`TableCellTest`.
modules/javafx.graphics/src/test/java/test/javafx/scene/effect/EffectInputTest.java
line 215:
> 213:
> 214: assertEquals(2, countIllegalArgumentException, "Cycle in effect
> chain detected, exception should occur 2 times.");
> 215: Thread.currentThread().setUncaughtExceptionHandler(null);
This should be done in an `@AfterEach`, because this code do not run when an
exception is thrown before.
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundTest.java
line 789:
> 787:
> 788: @Test
> 789: public void testSingleFill() {
Indentation looks off?
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BackgroundTest.java
line 796:
> 794:
> 795: @Test
> 796: public void testSingleFillWithNullPaint() {
Indentation looks off?
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderPaneTest.java
line 949:
> 947:
> 948: @Test
> 949: public void testResizeBelowMinimum() {
Indentation looks off?
modules/javafx.graphics/src/test/java/test/javafx/scene/transform/TransformOperationsTest.java
line 577:
> 575: boolean isIdentity,
> 576: boolean
> isInvertible,
> 577: Class inverseType) {
Minor: Can use `Class<?>`
-------------
PR Review: https://git.openjdk.org/jfx/pull/1566#pullrequestreview-2320858125
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1770558488
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1770559868
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1770562151
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1770562169
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1770562200
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1770560911
