On Mon, 28 Mar 2022 17:06:41 GMT, lawrence.andrews <d...@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:
>
> Dispose the Frame based in EDT
The ability to have instructions as HTML text (hosted in `JEditorPane`) would
make the solution more flexible. Obviously, we shouldn't implement everything
we can think of. Yet we should still think about possible advanced features.
test/jdk/java/awt/print/PrinterJob/PrintLatinCJKTest.java line 48:
> 46: static PrintLatinCJKTest testInstance = new PrintLatinCJKTest();
> 47: static String info =
> 48: "To test 8022536, if a remote printer is the system default, \n"+
It's unclear to me whether a remote printer is required for the test.
We should probably clarify the test instructions. Yet I haven't looked into the
bug for which the test was written.
test/jdk/java/awt/print/PrinterJob/PrintLatinCJKTest.java line 91:
> 89: public static void main(String[] args) throws InterruptedException,
> InvocationTargetException {
> 90: PassFailJFrame passFailJFrame = new PassFailJFrame("Test
> Instruction" +
> 91: "Frame", info, 7, 30, 3);
This already breaks the threading rules of Swing: every access to Swing
components must be done on EDT.
The thing is `PassFailJFrame` extends `JFrame`, thus super constructor (the
inherited one from `JFrame`) will be called on main thread. It's not what we
want, isn't it?
I'd suggest not to extend `JFrame` at all. We're not developing new
functionality to `JFrame` but build UI using Swing components.
As a benefit, you'll be able to initialise GUI on EDT if the constructor is
called on another thread.
I support Phil that the framework should document threading and mark methods
which are allowed to be called on any thread. Probably all the methods should
allow for it.
test/jdk/java/awt/print/PrinterJob/PrintLatinCJKTest.java line 93:
> 91: "Frame", info, 7, 30, 3);
> 92: PrintLatinCJKTest.showFrame();
> 93: passFailJFrame.awaitAndCheck();
I believe `awaitAndCheck()` is the method that must be called from the main
thread. It waits until the test is completed.
If it was called on EDT, it would block the event queue.
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 42:
> 40: private final CountDownLatch latch = new CountDownLatch(1);
> 41: private boolean failed = false;
> 42: private String testFailedReason;
These two are accessed by EDT and main thread. The boolean field should be
declared `volatile` or use `AtomicBoolean`. The same applies for
`testFailedReason`.
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 45:
> 43: private JTextArea instructionsText;
> 44: private final int maxRowLength;
> 45: private final int maxStringLength;
These aren't needed as fields, they're used only to create instruction text.
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 65:
> 63: * @throws HeadlessException
> 64: * @throws InterruptedException
> 65: * @throws InvocationTargetException
I suggest adding generic descriptions of exceptions to avoid IDE warnings.
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 78:
> 76: invokeAndWait(() -> {
> 77: setLayout(new BorderLayout());
> 78: instructionsText = new JTextArea(instructions, maxRowLength,
> maxStringLength);
You could use line wrap functionality of JTextArea to avoid adding `\n` to the
instructions.
Consider placing it into JScrollPane for flexibility, though not required.
`maxRowLength` is actually the number of rows (lines), and `maxStringLength` is
the number of columns. These parameters are used to calculate the preferred
size of the text area, they don't limit the text the component can store.
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 80:
> 78: instructionsText = new JTextArea(instructions, maxRowLength,
> maxStringLength);
> 79: instructionsText.setEditable(false);
> 80: instructionsText.setFocusable(false);
I think leaving the instructions focusable won't hurt. It would allow selecting
text (and copying it) if the tester needs it for whatever reason.
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 95:
> 93: add(buttonsPanel, BorderLayout.SOUTH);
> 94: pack();
> 95: setLocation(10, 10);
The instruction window should probably be placed around the centre of the
screen.
The class should provide a method to position a secondary frame near the
instruction frame so that both frames don't overlap.
A possibility to include test UI to the instructions could be beneficial in
some cases: dealing with one window is easier than with several ones. It's just
a suggestion for further improvement though.
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 96:
> 94: pack();
> 95: setLocation(10, 10);
> 96: setVisible(true);
Closing the window with Close button should also fail the test, now it doesn't.
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 125:
> 123: public void getFailureReason() {
> 124: final JDialog dialog = new JDialog();
> 125: dialog.setTitle("Failure reason");
I suggest making the dialog Toolkit-modal so that only the dialog can be
interacted with.
final JDialog dialog = new JDialog(this, "Failure reason",
TOOLKIT_MODAL);
At this time, I can click Fail again several times and I'll have several
dialogs open. I can also click Pass which completes the test and disposes of
the dialogs.
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 135:
> 133: dialog.dispose();
> 134: latch.countDown();
> 135: });
This doesn't handle closing the dialog via Close button or Esc. I believe it
should also do everything but copying the entered text.
test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 144:
> 142: jPanel.add(okayBtnPanel, BorderLayout.SOUTH);
> 143: dialog.add(jPanel);
> 144: dialog.setLocationRelativeTo(null);
We should probably position the dialog relatively to the frame or the Fail
button.
Now the frame is displayed on the left edge of the screen but the dialog is in
the centre of the screen as if they're unrelated.
-------------
PR: https://git.openjdk.java.net/jdk/pull/7966
