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

[Lukasz Kostyra]

On Thu, 19 Sep 2024 18:20:03 GMT, Andy Goryachev <angorya 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...
>
> 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

You're right, someone probably did this originally for a reason. I didn't know `Supplier` would work like this, but the more you know!

Reverted to how it used to be, but JUnit5-ified.

> 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

Following above comments, I'll leave it as `Stream<Arguments>`

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1768597561
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1768599900