On Wed, 22 Nov 2023 19:26:40 GMT, Phil Race <p...@openjdk.org> wrote:
> Many printing tests do not have the @printer keyword. This adds them to those
> that need it.
> I also found one test that has nothing to do with printing in the print
> folder and moved it out.
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/PrintJob/EdgeTest/EdgeTest.java line 30:
> 28: * @summary Verifies that (0, 0) is the upper-left corner of the page, not
> 29: * the upper-left corner adjusted for the margins.
> 30: * @author dpm
Suggestion:
Perhaps, remove the `@author` tag here too.
test/jdk/java/awt/PrintJob/RoundedRectTest/RoundedRectTest.java line 29:
> 27: * @bug 4061440
> 28: * @summary Checks that rounded rectangles print correctly.
> 29: * @author dpm
Suggestion:
test/jdk/java/awt/print/PageFormat/SetOrient.java line 28:
> 26: * @summary Confirm that the clip and transform of the Graphics2D is
> 27: * affected by the landscape orientation of the PageFormat.
> 28: */
Since this test calls `pjob.print();`, it requires a printer. Indeed, it fails
without a printer:
runner starting test: java/awt/print/PageFormat/SetOrient.html
runner finished test: java/awt/print/PageFormat/SetOrient.html
Failed. Execution failed: Applet thread threw exception:
java.lang.RuntimeException: No print service found.
Test results: failed: 1
test/jdk/java/awt/print/PageFormat/SmallPaperPrinting.java line 30:
> 28:
> 29: import java.awt.*;
> 30: import java.awt.print.*;
Could you expand the wildcard imports, please?
I would appreciate if you would add the jtreg tags *after* the imports.
test/jdk/java/awt/print/PageFormat/SmallPaperPrinting.java line 35:
> 33: {
> 34: public static void main(String args[])
> 35: {
Suggestion:
public class SmallPaperPrinting {
public static void main(String[] args) {
Since you're reformatting the test source, it's better to follow the Java code
style… throughout the entire source file.
test/jdk/java/awt/print/PageFormat/SmallPaperPrinting.java line 42:
> 40: System.out.println("A passing test should catch a PrinterException");
> 41: System.out.println("and should display \"Print error: (exception
> msg)\".");
> 42:
> System.out.println("---------------------------------------------------\n");
According to these instructions, the test is to contain a set of `@test` tags:
/*
* @test
* @key printer
* @run main/othervm SmallPaperPrinting
*/
/*
* @test
* @key printer
* @run main/othervm SmallPaperPrinting 1
*/
/*
* @test
* @key printer
* @run main/othervm SmallPaperPrinting 2
*/
Otherwise, it won't run all the cases and no one will ever see these
instructions.
For me, the test with the added `@test` tags as above prints an error message
in the first two cases:
# id0.jtr
Print error:
Paper's imageable height is too small.
# id1.jtr
Print error:
Paper's imageable width is too small.
Yet it does not print any error message in the third case where `width=-1`, and
**it does not fail**.
If I run it on a system without a printer, the test also *passes successfully*.
Perhaps, we can ignore it as the `@key printer` ensures there's a printer the
system.
Having said the above, this test requires its own bug to fix the test.
test/jdk/java/awt/print/PageFormat/WrongPaperPrintingTest.java line 29:
> 27: @key printer
> 28: @run main/manual WrongPaperPrintingTest
> 29: */
It's minor but still, in majority of tests, the `@key` tag follows the `@bug`
tag, and you mostly follow the convention in this PR. Keeping the order
consistent helps scanning the test description with eyes quickly.
test/jdk/java/awt/print/PrinterJob/EmptyFill.java line 30:
> 28: * @key printer
> 29: * @run main EmptyFill
> 30: */
This test does not need a printer, it prints into a byte array and analyses the
output. The test passes successfully on a system without a printer.
test/jdk/java/awt/print/PrinterJob/GetUserNameTest.java line 26:
> 24: * @test
> 25: * @bug 6197099
> 26: * @key printer
The test works correctly without a printer, it prints the expected message:
SecurityException thrown as user.name permission not given
No other exceptions are thrown but the warnings that `SecurityManager` is
deprecated.
test/jdk/java/awt/print/PrinterJob/MultiMonPrintDlgTest.java line 40:
> 38: * @test
> 39: * @bug 8138749
> 40: * @key printer
Suggestion:
* @key printer multimon
The test requires two monitors.
test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 26:
> 24: /**
> 25: *
> 26: * @bug 4884389 7183516
Is it intentional that there's no `@test` tag?
test/jdk/java/awt/print/PrinterJob/PrintTranslatedFont.java line 29:
> 27: * @key printer
> 28: * @summary Test that fonts with a translation print where they should.
> 29: * @author prr
Suggestion:
test/jdk/java/awt/print/PrinterJob/PrinterDevice.java line 31:
> 29: * @key printer
> 30: * @run main/othervm PrinterDevice
> 31: */
There's no `@test` tag, is it intentional?
Probably, you should remove this test from this PR because you're modifying in
#16773.
test/jdk/java/awt/print/PrinterJob/PrinterDialogsModalityTest/PrinterDialogsModalityTest.html
line 30:
> 28: @key printer
> 29: @summary check whether Print- and Page- dialogs are modal and correct
> window activated after their closing
> 30: @author Your Name: area=PrinterJob.modality
Suggestion:
test/jdk/java/awt/print/PrinterJob/RemoveListener.java line 25:
> 23:
> 24: /*
> 25: * @test 1.1 01/05/17
Suggestion:
* @test
-------------
PR Review: https://git.openjdk.org/jdk/pull/16785#pullrequestreview-1746215963
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403233563
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403232953
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403250610
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403257236
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403254342
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403283083
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403301368
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403332726
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403341309
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403576760
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403595968
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403608444
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403613621
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403613833
PR Review Comment: https://git.openjdk.org/jdk/pull/16785#discussion_r1403616874
