RFR: 8320676 : Manual printer tests have no Pass/Fail buttons, instructions close [v3]
Alexey Ivanov
aivanov at openjdk.org
Wed Feb 7 19:20:04 UTC 2024
On Thu, 1 Feb 2024 10:06:21 GMT, Renjith Kannath Pariyangad <rkannathpari at openjdk.org> wrote:
>> Hi Reviewers,
>>
>> Updated manual printer test cases with 'PassFailJFrame', also removed unused variables. Added 'SkippedException' in case of printer missing or not configured.
>>
>> Please review and let me know your suggestions.
>>
>> Regards,
>> Renjith
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with one additional commit since the last revision:
>
> Disposed g2D object and similar test parm into one line
In general, I suggest removing all blank line at the start of methods. In a method, the first statement follows the declaration, there's no need for a blank line in such context.
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 82:
> 80: myHeightLabel.setText("Format Height = " + (float) myPageFormat.getHeight());
> 81: myImageableXLabel.setText
> 82: ("Format Left Margin = " + (float) myPageFormat.getImageableX());
~~Leave the opening parenthesis on the line of the call.~~
~~In this case, I suggest wrapping at `+`, it makes the code clearer:~~
Suggestion:
myImageableXLabel.setText("Format Left Margin = "
+ (float) myPageFormat.getImageableX());
It's existing code, let us keep it as is and clean up separately if there's a desire to do so.
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 82:
> 80: myHeightLabel.setText("Format Height = " + (float) myPageFormat.getHeight());
> 81: myImageableXLabel.setText
> 82: ("Format Left Margin = " + (float) myPageFormat.getImageableX());
I see this is the original code, you just moved it by two spaces to follow the standard Java indentation of 4 spaces.
It's okay to keep it as is.
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 85:
> 83: myImageableRightLabel.setText
> 84: ("Format Right Margin = " + (float) (myPageFormat.getWidth() -
> 85: (myPageFormat.getImageableX() + myPageFormat.getImageableWidth())));
~~This is confusing: the part of the second argument is completely lost out of view. Please avoid such style of wrapping.~~
I suggest such wrapping:
Suggestion:
myImageableRightLabel.setText("Format Right Margin = "
+ (float) (myPageFormat.getWidth()
- (myPageFormat.getImageableX() + myPageFormat.getImageableWidth())));
Here, the first part of the argument stays on the first line; the string is wrapped at `+` and then the expression is wrapped at `-` before a nested parenthesised expression starts. This way makes it easier to track what is going on.
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 108:
> 106: o == PageFormat.LANDSCAPE ? "LANDSCAPE" :
> 107: o == PageFormat.REVERSE_LANDSCAPE ? "REVERSE_LANDSCAPE" :
> 108: "<invalid>"));
I don't like so much of indentation. You may keep the original indentation.
Even better, use the `switch` statement — it works perfectly for such cases.
Suggestion:
("Format Orientation = " +
(switch (o) {
case PageFormat.PORTRAIT -> "PORTRAIT";
case PageFormat.LANDSCAPE -> "LANDSCAPE";
case PageFormat.REVERSE_LANDSCAPE -> "REVERSE_LANDSCAPE";
default -> "<invalid>";
}));
Isn't it cleaner?
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 180:
> 178: } catch (PrinterException pe) {
> 179: pe.printStackTrace();
> 180: }
Not sure what's the most appropriate way to handle the exception in this case.
Logging it into the test log is definitely better than silently swallowing it.
Since it's a manual test, showing a message to the user is a good option too. It could let the tester retry or fail the test. Otherwise the tester isn't even away a problem occurred.
test/jdk/java/awt/print/PageFormat/PageSetupDialog.java line 216:
> 214: g2d.drawString("Graphics is " + g2d.getClass().getName(), 100, 100);
> 215: g2d.drawRect(0, 0, (int) pageFormat.getImageableWidth(),
> 216: (int) pageFormat.getImageableHeight());
Suggestion:
g2d.drawRect(0, 0,
(int) pageFormat.getImageableWidth(),
(int) pageFormat.getImageableHeight());
Wrap both parameters? Align the wrapped parameters to the column following the opening parenthesis?
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 53:
> 51: "You must have a printer available to perform this test.\n" +
> 52: "This test silently starts a print job and while the job is\n" +
> 53: "still being printed, cancels the print job\n" +
Does it silently start the print job? It displays a dialog before printing starts.
As far as I can see the test is kind of automatic after the first user interaction with the print dialog.
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 57:
> 55: "was properly cancelled.\n" +
> 56: "You will need to kill the application manually since regression\n" +
> 57: "tests apparently aren't supposed to call System.exit()";
The tester cannot see the console. The test must handle this situation and provide the feedback to the tester as required or fail the test automatically.
The tester should not terminate the application manually, the test must exit gracefully in either case: success or failure.
It could be reasonable to submit a new bug for this test only and use the changes in this PR as the starting point. I didn't analyse thoroughly the tests when I submitted this bug. You already deemed, there were too many tests for a single changeset.
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 66:
> 64: }
> 65:
> 66: PassFailJFrame passFailJFrame = new PassFailJFrame.Builder()
Suggestion:
PassFailJFrame passFailJFrame = PassFailJFrame.builder()
Use helper method `build()` instead of explicitly creating the `Builder`. The class should perhaps be made private.
test/jdk/java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java line 111:
> 109: System.out.println("the expected PrintAbortException ");
> 110: }
> 111: }
`PrinterException` should be let to escape to fail the test; `PassFailJFrame.forceFail` could be called to achieve this effect.
The `if` statement in the `finally` block implies the test has to fail if `cancelWorked` remains `false` but the test will never fail.
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 40:
> 38: * @library /test/lib /java/awt/regtesthelpers
> 39: * @build PassFailJFrame jtreg.SkippedException
> 40: * @summary Font specified with face name loses style on printing
It is unclear to me what is implied by _“loses style on printing”_ and what _“with correct font style”_ in the instructions mean.
Printing code, derives a font with `PLAIN` style and size of 16. I see no other styles applied.
Could you check the bugid for more details?
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 48:
> 46: int fontNum = 0;
> 47: int startNum = 0;
> 48: int lineHeight = 18;
`lineHeight` is a constant, it better to declare it as such.
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 55:
> 53: "\n" +
> 54: "This bug is system dependent and is not always reproducible.\n" +
> 55: "A passing test will have all text printed with correct font style.";
Clarify the instructions to the tester: the test shows a print dialog first. What if the tester cancels that dialog? Would it be better to display a secondary UI with the Print button which starts printing and allows the tester to start again if they accidentally cancelled the first dialog?
This may be applicable to other tests.
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 62:
> 60: throw new SkippedException("Printer not configured or available."
> 61: + " Test cannot continue.");
> 62: }
Now that an exception is thrown, the part `Test cannot continue` is implied — the test stopped. So, I suggest updating all these instances, `"Printer not configured or available."` is enough to communicate the problem why the test throws `SkippedException`.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 1:
> 1: /*
I'd refactor the code to move button handlers into separate methods, otherwise the code which creates the UI objects is mixed with the handlers. It may be hard to follow.
It's not a requirement, it's a suggestion.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 91:
> 89: o == PageFormat.LANDSCAPE ? "LANDSCAPE" :
> 90: o == PageFormat.REVERSE_LANDSCAPE ? "REVERSE_LANDSCAPE" :
> 91: "<invalid>"));
The same suggestion to use `switch` statement here.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 180:
> 178: });
> 179:
> 180: Button pageButton = new Button("Page Setup..");
Suggestion:
Button pageButton = new Button("Page Setup...");
Three dots are usually used to indicate a button opens a dialog to request more details.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 192:
> 190: } catch (PrinterException pe) {
> 191: pe.printStackTrace();
> 192: }
Display an error message to the tester?
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 195:
> 193: });
> 194:
> 195: Button chooseButton = new Button("Printer..");
Suggestion:
Button chooseButton = new Button("Printer...");
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 219:
> 217: } catch (NumberFormatException nfe) {
> 218: nfe.printStackTrace();
> 219: }
Display an error message to the tester?
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 229:
> 227: add(panel);
> 228: TextArea ta = new TextArea(10, 45);
> 229: String ls = System.getProperty("line.Separator", "\n");
Does `\n` not work in `TextArea` on all the platforms? In particular on Windows? If it does, just embed it into the text below.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 259:
> 257: o == PageFormat.LANDSCAPE ? "LANDSCAPE" :
> 258: o == PageFormat.REVERSE_LANDSCAPE ? "REVERSE_LANDSCAPE" :
> 259: "<invalid>"));
The code for converting orientation to string is repeated here. Create a helper method which does it — eliminate duplicate code.
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 268:
> 266: g2d.drawString("Y THIS WAY", 60, 200);
> 267: g2d.drawRect(0, 0, (int) pageFormat.getImageableWidth(),
> 268: (int) pageFormat.getImageableHeight());
Suggestion:
g2d.drawRect(0, 0,
(int) pageFormat.getImageableWidth(),
(int) pageFormat.getImageableHeight());
Wrap width as well?
test/jdk/java/awt/print/PrinterJob/ValidatePage/ValidatePage.java line 273:
> 271: (int) pageFormat.getImageableHeight() - 2);
> 272:
> 273: g2d.dispose();
Never dispose of `Graphics` if you didn't explicitly create the object.
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 105:
> 103: pj.print();
> 104: } catch (PrinterException pe) {
> 105: pe.printStackTrace();
Display the error message and fail the test?
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 112:
> 110: }
> 111:
> 112: static class RasterCanvas extends Canvas implements Printable {
`private`?
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 116:
> 114: public int print(Graphics g, PageFormat pgFmt, int pgIndex) {
> 115: if (pgIndex > 0)
> 116: return Printable.NO_SUCH_PAGE;
Suggestion:
if (pgIndex > 0) {
return Printable.NO_SUCH_PAGE;
}
Use braces even for one-line blocks.
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 136:
> 134: Graphics2D g2 = (Graphics2D) g;
> 135:
> 136: g2.setColor(Color.black);
Setting the colour to black seems redundant. It's never used.
test/jdk/java/awt/print/PrinterJob/raster/RasterTest.java line 140:
> 138: BufferedImage bimg = new BufferedImage(200, 200,
> 139: BufferedImage.TYPE_INT_ARGB);
> 140: Graphics ig = bimg.getGraphics();
Here, you should dispose of `ig` — you created it.
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17607#pullrequestreview-1868278301
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481768439
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481789469
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481774732
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481796291
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481802628
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481815381
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481845665
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481827028
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481832105
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481840189
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481865125
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481861239
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481858341
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481851236
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481890888
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481868072
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481884318
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481871886
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481885107
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481872250
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481877477
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481879787
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481881435
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481882579
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481954095
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481958608
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481959688
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481956181
PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1481957282
More information about the client-libs-dev
mailing list