RFR: 8324807 : Manual printer tests have no Pass/Fail buttons, instructions close set 2 [v10]

Alexey Ivanov aivanov at openjdk.org
Tue Mar 5 14:10:54 UTC 2024 

On Mon, 4 Mar 2024 06:33:09 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:
> 
>   Updated instruction

Changes requested by aivanov (Reviewer).

test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 1:

> 1: /*

Could you please add `@Override` annotations to all the methods which implement the interfaces.

test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 59:

> 57:  */
> 58: public class Collate2DPrintingTest
> 59:         extends Frame implements Doc, Printable {

Suggestion:

        implements Doc, Printable {

`Frame` used to be the container to display the buttons, now it's not needed.

test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 74:

> 72:                 if (pj.printDialog(prSet)) {
> 73:                     pj.print(prSet);
> 74:                 }

In this case, the print dialog has *Collated* checked and disabled; the number of copies is set to 1 and I can't change the value.

Does it work for you? Is it a bug in JDK?

test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 102:

> 100:         main.add(Box.createVerticalGlue());
> 101:         main.add(print2D);
> 102:         main.add(printMerlin);

Suggestion:

        main.add(print2D);
        main.add(Box.createVerticalStrut(4));
        main.add(printMerlin);

Add some margin between the buttons.

test/jdk/java/awt/print/PrinterJob/DrawImage.java line 49:

> 47: public class DrawImage {
> 48:     private static final int OBJECT_BORDER = 15;
> 49:     private static final String INSTRUCTIONS =

Suggestion:

    private static final int OBJECT_BORDER = 15;

    private static final String INSTRUCTIONS =

I think it's better with a blank line, the two constants aren't closely related.

test/jdk/java/awt/print/PrinterJob/DrawImage.java line 57:

> 55:     private final PageFormat pageFormat;
> 56: 
> 57:     public DrawImage(BufferedImage image) {

Let's make it private too.
Suggestion:

    private DrawImage(BufferedImage image) {

test/jdk/java/awt/print/PrinterJob/DrawImage.java line 73:

> 71:         int y = (int) pageFormat.getImageableY();
> 72: 
> 73:         // make slightly smaller (25) than max possible width

Suggestion:

        // Make the image slightly smaller (25) than max possible width

test/jdk/java/awt/print/PrinterJob/DrawImage.java line 87:

> 85:     }
> 86: 
> 87:     public void print() throws PrinterException {

Suggestion:

    private void print() throws PrinterException {

Make it `private` to avoid any confusion with implementation of `Printable` interface which must be `public`.

test/jdk/java/awt/print/PrinterJob/DrawImage.java line 93:

> 91:         if (pj.printDialog()) {
> 92:             pj.print();
> 93:         }

Fail the test if the tester clicks Cancel in the print dialog?

test/jdk/java/awt/print/PrinterJob/DrawImage.java line 104:

> 102:         if (image == null) {
> 103:             throw new RuntimeException("Image creation failed");
> 104:         }

It seems impossible for the `image` to be `null`. Let's drop this condition. If for whatever reason it is `null`, printing will fail with NPE.

test/jdk/java/awt/print/PrinterJob/DrawImage.java line 117:

> 115:     }
> 116: 
> 117:     public static BufferedImage prepareFrontImage() {

Let's make it private too.
Suggestion:

    private static BufferedImage prepareFrontImage() {

test/jdk/java/awt/print/PrinterJob/DrawImage.java line 121:

> 119:         BufferedImage result = new BufferedImage(400, 200,
> 120:                                    BufferedImage.TYPE_BYTE_GRAY);
> 121:         int w = result.getWidth(), h = result.getHeight();

Suggestion:

        int w = result.getWidth();
        int h = result.getHeight();

Avoid declaring several variables at the same line.

test/jdk/java/awt/print/PrinterJob/DrawStringMethods.java line 50:

> 48:             " This test will automatically initiate a print\n" +
> 49:             "\n" +
> 50:             " Confirm that the methods are printed.\n" +

Suggestion:

            " This test will automatically initiate a print.\n" +
            "\n" +
            " Confirm that the following methods are printed:\n" +

test/jdk/java/awt/print/PrinterJob/DrawStringMethods.java line 76:

> 74:     }
> 75: 
> 76:     public static AttributedCharacterIterator getIterator(String s) {

Suggestion:

    private static AttributedCharacterIterator getIterator(String s) {

Let's make it private.

test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 49:

> 47:  * @run main/manual InvalidPage
> 48:  */
> 49: public class InvalidPage extends Frame implements Printable {

Suggestion:

public class InvalidPage implements Printable {

No `Frame` is needed.

test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 116:

> 114:             " Repeat for all the printers as you have installed\n" +
> 115:             " On Solaris and Linux just one printer is sufficient.\n" +
> 116:             " Collect the output and examine it, each print job has two pages\n" +

Suggestion:

            "\n" +
            " Repeat for all the printers you have installed.\n" +
            " On Solaris and Linux just one printer is sufficient.\n" +
            "\n" +
            " Collect the output and examine it, each print job has two pages\n" +

Add blank lines to separate paragraphs for easier parsing / reading.

test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 123:

> 121:             " are positioned identically on both pages\n" +
> 122:             " The test fails if the output from the two\n" +
> 123:             " pages of a job is aligned differently";

Suggestion:

            "\n" +
            " The test fails if the output from the two\n" +
            " pages of a job is aligned differently";

test/jdk/java/awt/print/PrinterJob/InvalidPage.java line 132:

> 130:         PassFailJFrame.builder()
> 131:                 .instructions(INSTRUCTIONS)
> 132:                 .splitUI(InvalidPage::createTestUI)

Suggestion:

                .instructions(INSTRUCTIONS)
                .testTimeOut(10)
                .splitUI(InvalidPage::createTestUI)

Increase the timeout as the tester needs to perform this operation for all the printers installed and verify the printouts.

test/jdk/java/awt/print/PrinterJob/JobName/PrinterJobName.java line 40:

> 38:  */
> 39: public class PrinterJobName implements Printable {
> 40:     private static final String THE_NAME = "Testing the Jobname setting";

Suggestion:

    private static final String THE_NAME = "Testing the Job name setting";

Spell it with space.

test/jdk/java/awt/print/PrinterJob/JobName/PrinterJobName.java line 41:

> 39: public class PrinterJobName implements Printable {
> 40:     private static final String THE_NAME = "Testing the Jobname setting";
> 41:     private static final String INSTRUCTIONS =

Keep the blank line between these two constants.

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

PR Review: https://git.openjdk.org/jdk/pull/17608#pullrequestreview-1916838124
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512798993
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512791435
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512798212
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512792702
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512803855
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512811330
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512812240
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512807572
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512808081
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512822219
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512808749
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512809270
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512828005
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512829017
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512839764
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512836220
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512836833
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512837873
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512881276
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1512872881

More information about the client-libs-dev mailing list