RFR: 8324807 : Manual printer tests have no Pass/Fail buttons, instructions close set 2 [v4]
Alexey Ivanov
aivanov at openjdk.org
Thu Feb 8 12:20:10 UTC 2024
On Thu, 8 Feb 2024 11:21:16 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:
>
> Review comments integrated
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 44:
> 42: import java.awt.print.PrinterJob;
> 43: import java.io.InputStream;
> 44: import java.io.Reader;
We haven't explicitly decided on the order of imports, yet usually `java.*` packages precede `javax.*` packages in the list.
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 51:
> 49: * @summary Collation should work.
> 50: * @key printer
> 51: * @library /test/lib /java/awt/regtesthelpers
Why did you decide to change the order?
At this time, the test doesn't even use any classes from `/test/lib`.
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 65:
> 63:
> 64: public Collate2DPrintingTest() {
> 65:
Suggestion:
public Collate2DPrintingTest() {
Remove the blank line.
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 73:
> 71: add("South", butPanel);
> 72:
> 73: defService = PrintServiceLookup.lookupDefaultPrintService();
Just below this line, the test verifies if a printer is available. You should move this block directly into `main`.
Unfortunately, `PassFailJFrame` doesn't play nicely if an exception is thrown in initialisation code.
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 109:
> 107: }
> 108: } catch (Exception e) {
> 109: PassFailJFrame.forceFail( ae.getActionCommand() + " test Failed");
Are you sure `ae.getActionCommand()` returns non-`null` value? No `Action`s are used in the test.
I suggest you also keep `printStackTrace` — it will provide valuable details when analysing test failure.
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 112:
> 110: }
> 111: }
> 112:
I also suggest resolving IDE warning for line 118: `flavor` variable is redundant in `getDocFlavor`.
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 140:
> 138: "Click on the 'PrintService', should get a print from default printer\n" +
> 139: "\n" +
> 140: "If you get only one copy or non 'Collated' prints from any of the above case, " +
Suggestion:
"If you get only one copy or non 'Collated' prints from any of the above cases, " +
test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 145:
> 143: public static void main(String[] args) throws Exception {
> 144:
> 145: PassFailJFrame.builder()
Suggestion:
public static void main(String[] args) throws Exception {
PassFailJFrame.builder()
Remove the blank line.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 51:
> 49: public class DrawImage {
> 50:
> 51: protected static final int objectBorder = 15;
Suggestion:
private static final int objectBorder = 15;
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 63:
> 61: }
> 62:
> 63: protected int printImage(Graphics g, PageFormat pf, int pageIndex) {
Suggestion:
private int printImage(Graphics g, PageFormat pf, int pageIndex) {
I'm for using `private`.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 66:
> 64: if (pageIndex == 0) {
> 65: return Printable.NO_SUCH_PAGE;
> 66: }
Did you run the test?
Suggestion:
if (pageIndex > 0) {
return Printable.NO_SUCH_PAGE;
}
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 79:
> 77: // make slightly smaller (25) than max possible width
> 78: float scaleFactor = ((float) ((paperW - 25) - objectBorder -
> 79: objectBorder) / (float) (image.getWidth()));
Suggestion:
float scaleFactor = (float) ((paperW - 25) - objectBorder * 2)
/ (image.getWidth());
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 87:
> 85: }
> 86:
> 87: public void print() throws PrinterException {
Adjust indentation in `print`.
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 131:
> 129: Graphics2D g2D = (Graphics2D) result.getGraphics();
> 130: g2D.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
> 131: RenderingHints.VALUE_ANTIALIAS_OFF);
This hasn't been agreed upon (yet).
I prefer the style where the wrapped parameters are aligned to the start of the expression. That is the way it was before. Generally, I'd avoid modifying lines you don't change.
What do other reviewers think?
test/jdk/java/awt/print/PrinterJob/DrawImage.java line 140:
> 138:
> 139: AffineTransform originXform = AffineTransform.getTranslateInstance(w /
> 140: 5.0, h / 5.0);
For clarity, wrap all the parameters:
AffineTransform originXform = AffineTransform.getTranslateInstance(
w / 5.0, h / 5.0);
By the way, this is the case where aligning parameters to start of expression, opening parenthesis doesn't do any good — follow the usual double-intent (8 spaces instead of 4) for the continuation line.
test/jdk/java/awt/print/PrinterJob/DrawStringMethods.java line 43:
> 41: * @key printer
> 42: * @summary Confirm that all of the drawString methods on Graphics2D
> 43: * work for printer graphics objects.
I suggest keeping the original style, I think it is easier to parse the summary when the wrapped line is indented with spaces.
test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 70:
> 68: } catch (PrinterException pe) {
> 69: PassFailJFrame.forceFail("Test Failed");
> 70: }
I suggest using `printStackTrace` for debugging purposes.
You may want to include the message from the exception as well as hint that the test fails because `PrinterException` is thrown/caught.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17608#pullrequestreview-1869951830
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482832339
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482825293
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482834178
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482845316
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482829518
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482842295
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482830283
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482833660
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482848782
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482849606
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482848266
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482853070
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482854460
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482860121
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482863774
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482868284
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1482872210
More information about the client-libs-dev
mailing list