The message from this sender included one or more files
which could not be scanned for virus detection; do not
open these files unless you are certain of the sender's intent.
----------------------------------------------------------------------
On Wed, 8 Mar 2023 12:29:45 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8302797
>
> 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.
> 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.
> tests/system/src/test/java/test/javafx/scene/text/ArabicWrappingTest.java
> line 89:
>
>> 87: @AfterClass
>> 88: public static void exitTest() {
>> 89: Platform.exit();
>
> I recommend using the recently-added
> [Util.shutdown](https://github.com/openjdk/jfx/blob/ba0f28dc072605c1ccd30e2736d39b1fcb11c4cd/tests/system/src/test/java/test/util/Util.java#L380)
> utility method.
Same as above.
> tests/system/src/test/java/test/javafx/scene/text/ArabicWrappingTest.java
> line 95:
>
>> 93: static volatile boolean testPassed;
>> 94:
>> 95: @Test
>
> Can you add a timeout value? That way if something goes wrong the test won't
> hang indefinitely. Something like:
>
>
> @Test (timeout=60000)
Will do. I actually had one in there .. but I was having some problems with
@Test not being recognised and I removed it in case it was part of the problem.
I will put it back.
> tests/system/src/test/java/test/javafx/scene/text/ArabicWrappingTest.java
> line 150:
>
>> 148: stage.setTitle("Test bidi text wrapping");
>> 149: stage.setWidth(MAX_WW+50);
>> 150: stage.setHeight(600);
>
> In connection with `Util.launch(startupLatch, ...)`, here is where you can
> add:
>
>
> stage.setOnShown(e -> Platform.runLater(startupLatch::countDown));
Because of the sleep loop in my test I don't need this.
-------------
PR: https://git.openjdk.org/jfx/pull/1055
