On Wed, 8 Mar 2023 19:20:21 GMT, Phil Race <p...@openjdk.org> wrote:
>> tests/system/src/test/java/test/javafx/scene/text/ArabicWrappingTest.java
>> line 46:
>>
>>> 44: import java.util.TimerTask;
>>> 45:
>>> 46: public class ArabicWrappingTest extends Application {
>>
>> The main test class should not extend Application. See
>> [JDK-8234876](https://bugs.openjdk.org/browse/JDK-8234876). Instead, create
>> an static nested class that extends Application.
>
> My @Test does not depend on the instance that junit creates so I don't need
> to worry about this.
> So it was deliberate. I will add a comment explaining this.
OK, given the comment you added.
>> tests/system/src/test/java/test/javafx/scene/text/ArabicWrappingTest.java
>> line 83:
>>
>>> 81: System.setErr(systemErrFilter);
>>> 82: new Thread(() -> {
>>> 83: Application.launch(ArabicWrappingTest.class);
>>
>> I recommend using the recently-added
>> [Util.launch](https://github.com/openjdk/jfx/blob/ba0f28dc072605c1ccd30e2736d39b1fcb11c4cd/tests/system/src/test/java/test/util/Util.java#L319)
>> utility method.
>
> I looked at it but found it un-necessary and it means you need to understand
> that to use it
> and your test is vulnerable to changes there.
Should be OK given that your test method will now timeout if the launch fails
for some reason. If we see failures in our CI system we might need to revisit
this.
-------------
PR: https://git.openjdk.org/jfx/pull/1055
