Re: RFR: 8320443: [macos] Test java/awt/print/PrinterJob/PrinterDevice.java fails on macOS [v2]
On Tue, 5 Dec 2023 20:32:49 GMT, Phil Race wrote:
>> With JDK 17, I still see the exception:
>>
>>
>> Exception in thread "main" java.awt.print.PrinterException
>> at java.desktop/sun.lwawt.macosx.CPrinterJob.print(CPrinterJob.java:393)
>> at PrinterDevice.main(PrinterDevice.java:68)
>> Caused by: java.lang.NullPointerException: Cannot read field "m00" because
>> "Tx" is null
>> at
>> java.desktop/java.awt.geom.AffineTransform.(AffineTransform.java:490)
>> at
>> java.desktop/sun.print.PrinterGraphicsConfig.getDefaultTransform(PrinterGraphicsConfig.java:105)
>> at PrinterDevice.print(PrinterDevice.java:87)
>>
>>
>> Yet it's fine to ensure no exception is swallowed.
>
> That exception you are seeing is from the printStackTrace()
> If you were to comment out these lines as shown below
> @@ -90,9 +90,9 @@ public int print(Graphics g, PageFormat pf, int pageIndex) {
> AffineTransform gt = g2.getTransform();
> System.out.println("Graphics2D transform = " + gt);
> } catch (Exception e) {
> -failed = true;
> -System.err.println("Unexpected exception getting transform.");
> -e.printStackTrace();
> +//failed = true;
> +//System.err.println("Unexpected exception getting transform.");
> +//e.printStackTrace();
> throw e;
>
> then the test would pass on JDK 17
Nope, I checked that it's the real exception. Yet I used a recent version of
JDK 17 where [JDK-8262731](https://bugs.openjdk.org/browse/JDK-8262731) was
fixed, it's present in 17.0.1.
Indeed, the test passes with GA version of JDK 17 without the `failed` field.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416254033
Re: RFR: 8320443: [macos] Test java/awt/print/PrinterJob/PrinterDevice.java fails on macOS [v2]
On Tue, 5 Dec 2023 20:25:46 GMT, Alexey Ivanov wrote:
>> On JDK 17 and earlier you will reach this line with this test as written
>> And without that exception, the test would pass when it should fail.
>
> With JDK 17, I still see the exception:
>
>
> Exception in thread "main" java.awt.print.PrinterException
> at java.desktop/sun.lwawt.macosx.CPrinterJob.print(CPrinterJob.java:393)
> at PrinterDevice.main(PrinterDevice.java:68)
> Caused by: java.lang.NullPointerException: Cannot read field "m00" because
> "Tx" is null
> at
> java.desktop/java.awt.geom.AffineTransform.(AffineTransform.java:490)
> at
> java.desktop/sun.print.PrinterGraphicsConfig.getDefaultTransform(PrinterGraphicsConfig.java:105)
> at PrinterDevice.print(PrinterDevice.java:87)
>
>
> Yet it's fine to ensure no exception is swallowed.
That exception you are seeing is from the printStackTrace()
If you were to comment out these lines as shown below
@@ -90,9 +90,9 @@ public int print(Graphics g, PageFormat pf, int pageIndex) {
AffineTransform gt = g2.getTransform();
System.out.println("Graphics2D transform = " + gt);
} catch (Exception e) {
-failed = true;
-System.err.println("Unexpected exception getting transform.");
-e.printStackTrace();
+//failed = true;
+//System.err.println("Unexpected exception getting transform.");
+//e.printStackTrace();
throw e;
then the test would pass on JDK 17
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416246478
Re: RFR: 8320443: [macos] Test java/awt/print/PrinterJob/PrinterDevice.java fails on macOS [v2]
On Tue, 5 Dec 2023 20:07:56 GMT, Phil Race wrote:
>> test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 71:
>>
>>> 69: if (failed) {
>>> 70: throw new RuntimeException("Test failed but no exception
>>> propagated.");
>>> 71: }
>>
>> A comment that `pj.print` should not throw exception would suffice, even
>> though it's implied by jtreg any way.
>>
>> This statement is essentially unreachable if `failed` is set to `true`.
>
> On JDK 17 and earlier you will reach this line with this test as written
> And without that exception, the test would pass when it should fail.
With JDK 17, I still see the exception:
Exception in thread "main" java.awt.print.PrinterException
at java.desktop/sun.lwawt.macosx.CPrinterJob.print(CPrinterJob.java:393)
at PrinterDevice.main(PrinterDevice.java:68)
Caused by: java.lang.NullPointerException: Cannot read field "m00" because "Tx"
is null
at
java.desktop/java.awt.geom.AffineTransform.(AffineTransform.java:490)
at
java.desktop/sun.print.PrinterGraphicsConfig.getDefaultTransform(PrinterGraphicsConfig.java:105)
at PrinterDevice.print(PrinterDevice.java:87)
Yet it's fine to ensure no exception is swallowed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416240089
Re: RFR: 8320443: [macos] Test java/awt/print/PrinterJob/PrinterDevice.java fails on macOS [v2]
On Tue, 5 Dec 2023 19:41:54 GMT, Alexey Ivanov wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8320443
>
> test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 52:
>
>> 50: public class PrinterDevice implements Printable {
>> 51:
>> 52: static volatile boolean failed = false;
>
> Is it really needed? In all the cases where you assign `true` to the `failed`
> field, you also explicitly throw an exception, which is enough to fail the
> test.
It is to test the "the macOS printing implementation was swallowing exceptions
it should not."
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416221561
Re: RFR: 8320443: [macos] Test java/awt/print/PrinterJob/PrinterDevice.java fails on macOS [v2]
On Tue, 5 Dec 2023 20:07:15 GMT, Alexander Zvegintsev
wrote:
>> test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 52:
>>
>>> 50: public class PrinterDevice implements Printable {
>>> 51:
>>> 52: static volatile boolean failed = false;
>>
>> Is it really needed? In all the cases where you assign `true` to the
>> `failed` field, you also explicitly throw an exception, which is enough to
>> fail the test.
>
> It is to test the "the macOS printing implementation was swallowing
> exceptions it should not."
Not sufficient if something catches the exception as used to happen
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416222101
Re: RFR: 8320443: [macos] Test java/awt/print/PrinterJob/PrinterDevice.java fails on macOS [v2]
On Tue, 5 Dec 2023 19:45:34 GMT, Alexey Ivanov wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8320443
>
> test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 30:
>
>> 28: * @summary Checks that the PrinterGraphics is for a Printer
>> GraphicsDevice.
>> 29: * Test doesn't run unless there's a printer on the system.
>> 30: * @key printer
>
> Minor: we agreed to put `@key` after `@bug`.
ok ..
> test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 71:
>
>> 69: if (failed) {
>> 70: throw new RuntimeException("Test failed but no exception
>> propagated.");
>> 71: }
>
> A comment that `pj.print` should not throw exception would suffice, even
> though it's implied by jtreg any way.
>
> This statement is essentially unreachable if `failed` is set to `true`.
On JDK 17 and earlier you will reach this line with this test as written
And without that exception, the test would pass when it should fail.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416216067
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416222142
Re: RFR: 8320443: [macos] Test java/awt/print/PrinterJob/PrinterDevice.java fails on macOS [v2]
On Mon, 4 Dec 2023 23:31:47 GMT, Phil Race wrote:
>> For as long as we've had the macOS port, it seems that queries on the
>> GraphicsConfiguration associated with
>> a printer would give you null for the defaultTransform.
>> Clearly this API isn't a popular one to call in such a context else we'd
>> have had lots of reports.
>> We have a test that would have caught it except that until recently the
>> macOS printing implementation
>> was swallowing exceptions it should not.
>
> Phil Race has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8320443
Could you also update the copyright year in `CPrinterJob.java` and
`PrinterDevice.java`?
src/java.desktop/share/classes/sun/print/RasterPrinterJob.java line 2096:
> 2094:
> 2095: protected synchronized void setGraphicsConfigInfo(AffineTransform
> at,
> 2096: double pw, double ph) {
Suggestion:
protected synchronized void setGraphicsConfigInfo(AffineTransform at,
double pw, double ph) {
To preserve alignment.
test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 30:
> 28: * @summary Checks that the PrinterGraphics is for a Printer
> GraphicsDevice.
> 29: * Test doesn't run unless there's a printer on the system.
> 30: * @key printer
Minor: we agreed to put `@key` after `@bug`.
test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 52:
> 50: public class PrinterDevice implements Printable {
> 51:
> 52: static volatile boolean failed = false;
Is it really needed? In all the cases where you assign `true` to the `failed`
field, you also explicitly throw an exception, which is enough to fail the test.
test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 71:
> 69: if (failed) {
> 70: throw new RuntimeException("Test failed but no exception
> propagated.");
> 71: }
A comment that `pj.print` should not throw exception would suffice, even though
it's implied by jtreg any way.
This statement is essentially unreachable if `failed` is set to `true`.
-
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16773#pullrequestreview-1765958594
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416186891
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416198629
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416195194
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1416196288
Re: RFR: 8320443: [macos] Test java/awt/print/PrinterJob/PrinterDevice.java fails on macOS [v2]
> For as long as we've had the macOS port, it seems that queries on the > GraphicsConfiguration associated with > a printer would give you null for the defaultTransform. > Clearly this API isn't a popular one to call in such a context else we'd have > had lots of reports. > We have a test that would have caught it except that until recently the macOS > printing implementation > was swallowing exceptions it should not. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8320443 - Changes: - all: https://git.openjdk.org/jdk/pull/16773/files - new: https://git.openjdk.org/jdk/pull/16773/files/62b94cd7..a60c47e6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16773&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16773&range=00-01 Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16773.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16773/head:pull/16773 PR: https://git.openjdk.org/jdk/pull/16773
Re: RFR: 8320443: [macos] Test java/awt/print/PrinterJob/PrinterDevice.java fails on macOS [v2]
On Wed, 22 Nov 2023 07:49:07 GMT, Andrey Turbanov wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8320443
>
> src/java.desktop/share/classes/sun/print/RasterPrinterJob.java line 2096:
>
>> 2094:
>> 2095: synchronized protected void setGraphicsConfigInfo(AffineTransform
>> at,
>> 2096: double pw, double ph) {
>
> Let's use blessed modifiers order
> Suggestion:
>
> protected synchronized void setGraphicsConfigInfo(AffineTransform at,
> double pw, double ph) {
fixed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1414628370
Re: RFR: 8320443: [macos] Test java/awt/print/PrinterJob/PrinterDevice.java fails on macOS [v2]
On Thu, 23 Nov 2023 17:06:01 GMT, Alexey Ivanov wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8320443 > > test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 26: > >> 24: /* >> 25: * >> 26: * @bug 4276227 8320443 > > There's no `@test` tag, the test will not run without it. > > Is it intentional? @test is added - PR Review Comment: https://git.openjdk.org/jdk/pull/16773#discussion_r1414628583
