RFR: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex() [v2]

[Phil Race]

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 <kcr at 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