RFR: 8284535 : Fix PrintLatinCJKTest.java test that is failing with Parse Exception [v6]

[Alexey Ivanov]

On Thu, 7 Apr 2022 15:09:27 GMT, lawrence.andrews <duke at openjdk.java.net> wrote:

>> We need a common manual test framework code that can be shared across all the client manual test. This framework class should have the following
>> 1) Frame which contains test instruction .
>> 2) Pass & Fail button so that user can mark the test pass or fail
>> 3) Upon failing the test user should be able to elaborate why the test failed and this can be added to the test failure. 
>> 4) Disposing of all the frames.
>> 
>> @aivanov-jdk
>
> lawrence.andrews has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8284535 : Fix PrintLatinCJKTest.java test that is failing with Parse Exception

test/jdk/java/awt/print/PrinterJob/PrintLatinCJKTest.java line 26:

> 24: /*
> 25:  * @test
> 26:  * @bug 8022536

Why is [800535](https://bugs.openjdk.java.net/browse/JDK-8008535) removed? It's the bug this test is written for.

The additional bug [8022536](https://bugs.openjdk.java.net/browse/JDK-8022536) may cause NPE on Linux if a (default) remote printer is unavailable.

test/jdk/java/awt/print/PrinterJob/PrintLatinCJKTest.java line 52:

> 50:     private static String info = """
> 51:             To test 8022536, if a remote printer is the system default,
> 52:             it should show in the dialog as the selected printer.

Should these two lines be moved to the bottom of the instructions? This case is not the main test case of this test and it's also covered by another test `javax/print/TextFlavorTest.java`.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 58:

> 56:     private static final Timer timer = new Timer(0, null);
> 57: 
> 58:     public enum POSITION {HORIZONTAL, VERTICAL}

Suggestion:

    public enum Position {HORIZONTAL, VERTICAL}

The type shouldn't be all-capital, should it?

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 78:

> 76:      * @throws HeadlessException         HeadlessException
> 77:      * @throws InterruptedException      exception thrown for invokeAndWait
> 78:      * @throws InvocationTargetException exception thrown for invokeAndWait

I expected a more reasonable descriptions when any of the exceptions may be thrown. Anyway, it's unimportant. :)

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 102:

> 100:         instructionsText.setLineWrap(true);
> 101: 
> 102:         int testTimeout = (int) TimeUnit.MINUTES.toMillis(timeoutInMinutes);

Use `long`, it's safer and it's used everywhere when working with time.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 109:

> 107:         timer.setDelay(1000);
> 108:         timer.addActionListener((e) -> {
> 109:             int leftTime = testTimeout - (int) (System.currentTimeMillis() - startTime);

`long timeLeft` is better.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 113:

> 111:                 timer.stop();
> 112:                 testFailedReason = "Failure Reason:\n Timeout " +
> 113:                         "User did not perform testing.";

A suggestion: break string at the line break.
Suggestion:

                testFailedReason = "Failure Reason:\n"
                        + "Timeout - User did not perform testing.";

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 144:

> 142:                 super.windowClosing(e);
> 143:                 testFailedReason = "Failure Reason:\n User closed the " +
> 144:                         "instruction Frame";

Suggestion:

                testFailedReason = "Failure Reason:\n"
                        + "User closed the instruction frame";

I think it's easier to read this way, the parts of the phrase aren't broken.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 181:

> 179:         if (isEventDispatchThread()) {
> 180:             disposeFrames();
> 181:         } else invokeAndWait(PassFailJFrame::disposeFrames);

This is bad from Java Code Style point of view.

Additionally, you stated this method is not to be called on EDT, therefore you know `disposeFrames` should be called via `invokeAndWait`.

Shall this method throw an exception if it's called on EDT? It will prevent accidental mistakes.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 208:

> 206:      * example in the jtreg .jtr file.
> 207:      */
> 208:     public static void getFailureReason() {

It's not public actually.

test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 232:

> 230:                 super.windowClosing(e);
> 231:                 testFailedReason = "User closed the " +
> 232:                         "dialog";

I see no reason to break this string, it fits into 80-column.

Yet it's not an error: I'd rather leave the failure reason empty. In this case, the window listener can be dropped.

You can safely remove `super.windowClosing(e);`, the implementation in `WindowAdapter` does nothing.

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

PR: https://git.openjdk.java.net/jdk/pull/7966