8316412: javax/print tests fail without printer set up #15821

msobiers · 2023-09-19T14:45:47Z

For NullClipARGB we are throwing SkippedException only if instance of caught exception is PrinterException. All other exceptions are considered unrelated to printer configuration.

For CountPrintServices we want to throw SkippedException in the case when lpstat is not installed, where an IOException is caught in that case.

For ExceptionTest we want to throw SkippedException for all PrinterExceptions which were not caused by IndexOutOfBoundsException.

For rest of the tests if PrintService was not created instead RuntimeException we are throwing now SkippedException.

Tested on environment without printer installed.
Test are passing with skipped exception as expected.

Passed: java/awt/print/PrinterJob/ExceptionTest.java
Passed: java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java
Passed: java/awt/print/PrinterJob/PrtException.java
Passed: javax/print/attribute/AttributeTest.java
Passed: javax/print/attribute/GetCopiesSupported.java
Passed: javax/print/attribute/SidesPageRangesTest.java
Passed: javax/print/attribute/SupportedPrintableAreas.java
Passed: javax/print/PrintServiceLookup/CountPrintServices.java
Passed: javax/print/CheckDupFlavor.java

Additionally unused imports have been removed.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8316412: javax/print tests fail without printer set up (Bug - P4)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15821/head:pull/15821
$ git checkout pull/15821

Update a local copy of the PR:
$ git checkout pull/15821
$ git pull https://git.openjdk.org/jdk.git pull/15821/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15821

View PR using the GUI difftool:
$ git pr show -t 15821

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15821.diff

Webrev

Link to Webrev Comment

For NullClipARGB we are throwing SkippedException only if instance of caught exception is PrinterException. All other exceptions are considered unrelated to printer configuration. For CountPrintServices we want to throw SkippedException in the case when lpstat is not installed, where an IOException is caught in that case. For ExceptionTest we want to throw SkippedException for all PrinterExceptions which were not caused by IndexOutOfBoundsException. For rest of the tests if PrintService was not created instead RuntimeException we are throwing now SkippedException. Tested on environment without printer installed. Test are passing with skipped exception as expected. Passed: java/awt/print/PrinterJob/ExceptionTest.java Passed: java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java Passed: java/awt/print/PrinterJob/PrtException.java Passed: javax/print/attribute/AttributeTest.java Passed: javax/print/attribute/GetCopiesSupported.java Passed: javax/print/attribute/SidesPageRangesTest.java Passed: javax/print/attribute/SupportedPrintableAreas.java Passed: javax/print/PrintServiceLookup/CountPrintServices.java Passed: javax/print/CheckDupFlavor.java

bridgekeeper · 2023-09-19T14:46:58Z

Hi @msobiers, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user msobiers" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2023-09-19T14:49:06Z

@msobiers The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

msobiers · 2023-09-19T14:58:33Z

/covered

bridgekeeper · 2023-09-19T14:59:36Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

shipilev

The copyright years need to be adjusted a bit.

Copyright (c) 2007, Oracle and/or its affiliates. All rights reserved.

...should become:

Copyright (c) 2007, 2023, Oracle and/or its affiliates. All rights reserved.

shipilev · 2023-09-19T17:18:15Z

test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java

-             throw new RuntimeException(ex);
+        } catch (Exception ex) {
+            if (ex instanceof PrinterException) {
+                throw new SkippedException("Printer is required for this test.  TEST ABORTED");


I think for this test, we need to check PrintServiceLookup.lookupPrintServices(DocFlavor.SERVICE_FORMATTED.PRINTABLE, null);, like other tests do it. Let's not assume that PrinterException is thrown here only when printer is not configured. Add the block right at the beginning of main?

shipilev

Apart from stylistic comments, this looks good.

shipilev · 2023-09-20T11:02:15Z

test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java

        }
    }

    public int print(Graphics g, PageFormat pf, int pageIndex)
-               throws PrinterException{
+            throws PrinterException{


Redundant whitespace change.

shipilev · 2023-09-20T11:02:19Z

test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java

@@ -59,7 +68,7 @@ public int print(Graphics g, PageFormat pf, int pageIndex)
        g2.setColor( Color.BLACK );
        g2.drawString("This text should be visible through the image", 0, 20);
        BufferedImage bi = new BufferedImage(100, 100,
-                                              BufferedImage.TYPE_INT_ARGB );
+                BufferedImage.TYPE_INT_ARGB );


Redundant whitespace change.

shipilev · 2023-09-20T11:04:17Z

test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java


 public class NullClipARGB implements Printable {

    public static void main( String[] args ) {
+        PrintService[] svc = PrintServiceLookup.lookupPrintServices(DocFlavor.SERVICE_FORMATTED.PRINTABLE, null);


Do we need to ask for DocFlavor.SERVICE_FORMATTED.PRINTABLE here? Can we just ask for null?

Correct me if I'm wrong, but I think if we ask for null here it may find a PrintService that does not support PRINTABLE flavor so SkippedException will not be thrown because svc.length will be greater than 0 and we may end up with an exception thrown later in pj.print().

Oh, OK. So the idea is that if we use setPrintable, we need to lookup the service that accepts .PRINTABLE?

This is my understanding.

shipilev · 2023-09-20T11:05:49Z

test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java


        try {
            PrinterJob pj = PrinterJob.getPrinterJob();
            pj.setPrintable(new NullClipARGB());
            pj.print();
-            } catch (Exception ex) {


I see this is a cleanup, but we don't need to do it here, to keep patch simple and backportable.

shipilev · 2023-09-20T11:06:17Z

test/jdk/javax/print/CheckDupFlavor.java

 * @run main CheckDupFlavor
 */

 import javax.print.*;
-import javax.print.attribute.*;


Here and later, keep the imports in place, for the same reason: targeted backportable patch.

shipilev · 2023-09-20T14:28:47Z

test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java


        try {
            PrinterJob pj = PrinterJob.getPrinterJob();
            pj.setPrintable(new NullClipARGB());
            pj.print();
            } catch (Exception ex) {
-             throw new RuntimeException(ex);
+            throw new RuntimeException(ex);


One more redundant whitespace change.

shipilev · 2023-09-20T14:28:50Z

test/jdk/java/awt/print/PrinterJob/ImagePrinting/NullClipARGB.java

        }
    }

    public int print(Graphics g, PageFormat pf, int pageIndex)
-               throws PrinterException{
+                throws PrinterException{


One more redundant whitespace change.

shipilev

Looks good to me. Once OCA clears, someone familiar with this area would need to take a look too.

/reviewers 2

openjdk · 2023-09-20T15:13:43Z

@shipilev
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

mlbridge · 2023-09-20T16:48:07Z

Webrevs

00: Full (608a3add)

prrace · 2023-09-25T19:00:34Z

Just like
@key headful
is used for headful tests, these tests have
@key printer
which you are supposed to use to exclude them if you don't have a printer configured., eg

jtreg -k:!printer ...

In other words none of these changes should be necessary and I'm fairly sure are actually undesirable in some cases.
Notably skipping if lpstat isn't installed and you are trying to test printing is bad.

shipilev · 2023-09-28T17:17:55Z

In other words none of these changes should be necessary and I'm fairly sure are actually undesirable in some cases.
Notably skipping if lpstat isn't installed and you are trying to test printing is bad.

Right! Argh, I missed that the tests have keywords on them. I think we can close this as WNF, @msobiers. Sorry for leading you the wrong way here.

msobiers · 2023-09-29T08:39:24Z

In other words none of these changes should be necessary and I'm fairly sure are actually undesirable in some cases.
Notably skipping if lpstat isn't installed and you are trying to test printing is bad.

Right! Argh, I missed that the tests have keywords on them. I think we can close this as WNF, @msobiers. Sorry for leading you the wrong way here.

Np. Closing now.

bridgekeeper bot added the oca label Sep 19, 2023

openjdk bot added the client label Sep 19, 2023

bridgekeeper bot added the oca-verify label Sep 19, 2023

shipilev reviewed Sep 19, 2023

View reviewed changes

msobiers added 2 commits September 20, 2023 10:57

Updated copyright years.

0b9ae45

Added PrintService check.

b256cb1

shipilev reviewed Sep 20, 2023

View reviewed changes

Reverting code cleanup changes.

3e47f4c

shipilev reviewed Sep 20, 2023

View reviewed changes

Reverted indentation.

608a3ad

shipilev approved these changes Sep 20, 2023

View reviewed changes

bridgekeeper bot removed oca oca-verify labels Sep 20, 2023

openjdk bot added the rfr label Sep 20, 2023

msobiers closed this Sep 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8316412: javax/print tests fail without printer set up #15821

8316412: javax/print tests fail without printer set up #15821

msobiers commented Sep 19, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 19, 2023

openjdk bot commented Sep 19, 2023

msobiers commented Sep 19, 2023

bridgekeeper bot commented Sep 19, 2023

shipilev left a comment

shipilev Sep 19, 2023

shipilev left a comment

shipilev Sep 20, 2023

shipilev Sep 20, 2023

shipilev Sep 20, 2023

msobiers Sep 20, 2023 •

edited

Loading

shipilev Sep 20, 2023

msobiers Sep 20, 2023

shipilev Sep 20, 2023

shipilev Sep 20, 2023

shipilev Sep 20, 2023

shipilev Sep 20, 2023

shipilev left a comment

openjdk bot commented Sep 20, 2023

mlbridge bot commented Sep 20, 2023

prrace commented Sep 25, 2023

shipilev commented Sep 28, 2023

msobiers commented Sep 29, 2023

8316412: javax/print tests fail without printer set up #15821

8316412: javax/print tests fail without printer set up #15821

Conversation

msobiers commented Sep 19, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 19, 2023

openjdk bot commented Sep 19, 2023

msobiers commented Sep 19, 2023

bridgekeeper bot commented Sep 19, 2023

shipilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msobiers Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 20, 2023

mlbridge bot commented Sep 20, 2023

Webrevs

prrace commented Sep 25, 2023

shipilev commented Sep 28, 2023

msobiers commented Sep 29, 2023

msobiers commented Sep 19, 2023 •

edited by openjdk bot

Loading

msobiers Sep 20, 2023 •

edited

Loading