RFR: 8339512: [TestBug] Convert graphics tests to JUnit 5

[Andy Goryachev]

On Fri, 13 Sep 2024 15:14:39 GMT, Lukasz Kostyra <lkostyra at 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...

next batch.  will continue with Node_effectiveOrientation_Test.java

modules/javafx.graphics/src/test/java/test/javafx/concurrent/ScheduledServiceTest.java line 95:

> 93:     }
> 94: 
> 95:     @Override public void setup(TestServiceFactory factory) {

could have inserted a newline after the annotation...

modules/javafx.graphics/src/test/java/test/javafx/concurrent/ServiceLifecycleTest.java line 2107:

> 2105:     }
> 2106: 
> 2107:     private void assertThrowsException(Class exceptionClass, final ServiceTestExecution c) throws Throwable {

minor: maybe `Class<? extends Throwable> exceptionClass` ?

modules/javafx.graphics/src/test/java/test/javafx/concurrent/ServiceTestBase.java line 31:

> 29: import java.util.concurrent.Executor;
> 30: import javafx.concurrent.Service;
> 31: //import org.junit.jupiter.api.BeforeEach; NOTE: revert once parametrized classes are added

minor: this line is probably unnecessary

modules/javafx.graphics/src/test/java/test/javafx/concurrent/ServiceTestBase.java line 65:

> 63:     // NOTE: This should be reverted once parametrized class tests are added to JUnit5
> 64:     //       For now, tests call this manually
> 65:     // @BeforeEach

I wonder if we should log a JBS ticket to fix parameterized tests by reverting `@BeforeEach`, and then simply refer to it?
This way, once junit supports that, we can easily grep the code.

(I've used a different comment for same purpose)

What do you think?

-------------

PR Review: https://git.openjdk.org/jfx/pull/1566#pullrequestreview-2315838830
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1767007023
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1767029812
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1767031814
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1767036118