[9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

shashi
Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.

The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.

The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.

Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>

Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>

Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.

Thanks and regards,
Shashi
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

prasanta sadhukhan

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta
On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:
Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.

The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.

The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.

Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>

Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>

Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.

Thanks and regards,
Shashi

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

shashi

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah <[hidden email]>; [hidden email]
Cc: Philip Race <[hidden email]>
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

prasanta sadhukhan

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta
On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

shashi

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah <[hidden email]>; [hidden email]
Cc: Philip Race <[hidden email]>
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

Sergey Bylokhov
I did not check the all code, but small comment: Note that all Swing components should be always accessed on EDT. If InvokeAnDWait does not work then you can use InvokeLater().



The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.
 
Thanks and regards,
Shashi
From: Prasanta Sadhukhan 
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah <[hidden email]>; [hidden email]
Cc: Philip Race <[hidden email]>
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop
 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta
On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:
Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/
 
Thanks and regards,
Shashi
 
From: Prasanta Sadhukhan 
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop
 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException. 

Regards
Prasanta
On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:
Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi
 
 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

prasanta sadhukhan
In reply to this post by shashi

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);

should be done in other thread.

Regards
Prasanta
On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

shashi

Hi All,

  I have altered the manual test template per the comments.

 

1.      Have moved the test instructions window under newly created thread.

2.      Have moved the print dialog(main test module) under EDT.

3.      Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah <[hidden email]>; [hidden email]
Cc: Philip Race <[hidden email]>
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

prasanta sadhukhan

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,
79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.

Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.

Regards
Prasanta
On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.      Have moved the test instructions window under newly created thread.

2.      Have moved the print dialog(main test module) under EDT.

3.      Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

Philip Race
.. and please make sure all lines are <= 80 chars as per the coding standards.

-phil.

On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,
79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.

Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.

Regards
Prasanta
On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.      Have moved the test instructions window under newly created thread.

2.      Have moved the print dialog(main test module) under EDT.

3.      Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

shashi

Hi All, Please find the updated Webrev with fixes for the comments @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_04/

 

Thanks and regards,
Shashi

 

From: Philip Race
Sent: Thursday, June 8, 2017 3:32 AM
To: Prasanta Sadhukhan <[hidden email]>
Cc: Shashidhara Veerabhadraiah <[hidden email]>; [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

.. and please make sure all lines are <= 80 chars as per the coding standards.

-phil.

On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,

79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.
 
Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.
 
Regards
Prasanta

On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.     Have moved the test instructions window under newly created thread.

2.     Have moved the print dialog(main test module) under EDT.

3.     Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

prasanta sadhukhan

looks good to me.

Regards
Prasanta
On 6/9/2017 3:49 PM, Shashidhara Veerabhadraiah wrote:

Hi All, Please find the updated Webrev with fixes for the comments @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_04/

 

Thanks and regards,
Shashi

 

From: Philip Race
Sent: Thursday, June 8, 2017 3:32 AM
To: Prasanta Sadhukhan [hidden email]
Cc: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

.. and please make sure all lines are <= 80 chars as per the coding standards.

-phil.

On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,

79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.
 
Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.
 
Regards
Prasanta

On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.     Have moved the test instructions window under newly created thread.

2.     Have moved the print dialog(main test module) under EDT.

3.     Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 

 


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

Philip Race
private static void setValuesForPrintPageSetup(PageFormat pageFormat,  int
 118             marginValue) throws PrinterException {
 119         Paper paper = new Paper();
 double paperHeight = paper.getHeight();
 122         double paperWidth = paper.getWidth();
 123         double paperX = paper.getImageableX();
 124         double paperY = paper.getImageableY();
 125         paper.setImageableArea(paperX * marginValue, paperY * marginValue,
 126                 paperWidth - (paperX * 2 * marginValue),
 127                 paperHeight - (paperY * 2 * marginValue));


 105         setValuesForPrintPageSetup(pageFormat, 3);


I see you call new Paper() above
https://docs.oracle.com/javase/8/docs/api/java/awt/print/Paper.html#Paper--

Did you really intend to use a default paper instead of getting the one
from the pageFormat ? On some label printer your Letter Paper may not
even be supported. US (aka NA) Letter is 8.5" wide.

Also although it probably will work out OK the maths isn't checking
for boundary problems.

default margin will be 1" so that's what you'll get for paperX and paperY

Using your value of 3 we set the imagable area such that
imageable X = 1 * 3 = 3
imageableWidth = 8.5 - (1 * 2 *3) = 8.5 - 6 = 2.5;

Fortunately that worked out positive .. but it does not seem to be enforced.

If we'd used 5 it would be a different story : 

ix = 5, iw = 8.5 - ( 1 * 2 * 5) = -1.5

The implementation will (should) clamp it to non-negative but it
might still be better to have some defensive logic of your own.

nit: there's a missing space here

 75                 } catch(PrinterException e) {

-phil.

On 06/09/2017 03:27 AM, Prasanta Sadhukhan wrote:

looks good to me.

Regards
Prasanta
On 6/9/2017 3:49 PM, Shashidhara Veerabhadraiah wrote:

Hi All, Please find the updated Webrev with fixes for the comments @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_04/

 

Thanks and regards,
Shashi

 

From: Philip Race
Sent: Thursday, June 8, 2017 3:32 AM
To: Prasanta Sadhukhan [hidden email]
Cc: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

.. and please make sure all lines are <= 80 chars as per the coding standards.

-phil.

On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,

79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.
 
Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.
 
Regards
Prasanta

On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.     Have moved the test instructions window under newly created thread.

2.     Have moved the print dialog(main test module) under EDT.

3.     Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 

 



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

shashi

Hi Phil, Please see below for the comments:

 

The updated Webrev is at:

http://cr.openjdk.java.net/~aghaisas/shashi/6949753/webrev_05/

 

Thanks and regards,
Shashi

 

From: Phil Race
Sent: Saturday, June 10, 2017 3:07 AM
To: Prasanta Sadhukhan <[hidden email]>; Shashidhara Veerabhadraiah <[hidden email]>
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

private static void setValuesForPrintPageSetup(PageFormat pageFormat,  int
 118             marginValue) throws PrinterException {
 119         Paper paper = new Paper();
 double paperHeight = paper.getHeight();
 122         double paperWidth = paper.getWidth();
 123         double paperX = paper.getImageableX();
 124         double paperY = paper.getImageableY();
 125         paper.setImageableArea(paperX * marginValue, paperY * marginValue,
 126                 paperWidth - (paperX * 2 * marginValue),
 127                 paperHeight - (paperY * 2 * marginValue));
 
 
 105         setValuesForPrintPageSetup(pageFormat, 3);
 
 
I see you call new Paper() above
https://docs.oracle.com/javase/8/docs/api/java/awt/print/Paper.html#Paper--
 
Did you really intend to use a default paper instead of getting the one
from the pageFormat ? On some label printer your Letter Paper may not
even be supported. US (aka NA) Letter is 8.5" wide.
 
Also although it probably will work out OK the maths isn't checking
for boundary problems.
 
default margin will be 1" so that's what you'll get for paperX and paperY
 
Using your value of 3 we set the imagable area such that
imageable X = 1 * 3 = 3
imageableWidth = 8.5 - (1 * 2 *3) = 8.5 - 6 = 2.5;
 
Fortunately that worked out positive .. but it does not seem to be enforced.
 
If we'd used 5 it would be a different story : 
 
ix = 5, iw = 8.5 - ( 1 * 2 * 5) = -1.5
 
The implementation will (should) clamp it to non-negative but it
might still be better to have some defensive logic of your own.
[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog. In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may vary as you pointed out, depending on the locale.
Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are constants), it won’t cause an undesirable behavior like going into negative space.
 
nit: there's a missing space here
 
 75                 } catch(PrinterException e) {
[Shashi] This is fixed now.
 
-phil.
 

On 06/09/2017 03:27 AM, Prasanta Sadhukhan wrote:

looks good to me.

Regards
Prasanta

On 6/9/2017 3:49 PM, Shashidhara Veerabhadraiah wrote:

Hi All, Please find the updated Webrev with fixes for the comments @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_04/

 

Thanks and regards,
Shashi

 

From: Philip Race
Sent: Thursday, June 8, 2017 3:32 AM
To: Prasanta Sadhukhan [hidden email]
Cc: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

.. and please make sure all lines are <= 80 chars as per the coding standards.

-phil.

On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,

79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.
 
Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.
 
Regards
Prasanta

On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.     Have moved the test instructions window under newly created thread.

2.     Have moved the print dialog(main test module) under EDT.

3.     Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 

 

 

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

Philip Race
>[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog.
> In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may
> vary as you pointed out, depending on the locale.
>Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are const
> it won’t cause an undesirable behavior like going into negative space.


Sorry, that is not a valid thing to do. The print dialog is free to ignore this
or workaround the incompatibility of that paper with the supported media so your test is not reliable.
And I don't see what the cross platform print dialog has to do with it.
It is, or should be, just as aware of the printer sizes as the native one.

-phil.


On 06/12/2017 03:34 AM, Shashidhara Veerabhadraiah wrote:

Hi Phil, Please see below for the comments:

 

The updated Webrev is at:

http://cr.openjdk.java.net/~aghaisas/shashi/6949753/webrev_05/


Modified in what way from the previous version ?

-phil.

 

Thanks and regards,
Shashi

 

From: Phil Race
Sent: Saturday, June 10, 2017 3:07 AM
To: Prasanta Sadhukhan [hidden email]; Shashidhara Veerabhadraiah [hidden email]
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

private static void setValuesForPrintPageSetup(PageFormat pageFormat,  int
 118             marginValue) throws PrinterException {
 119         Paper paper = new Paper();
 double paperHeight = paper.getHeight();
 122         double paperWidth = paper.getWidth();
 123         double paperX = paper.getImageableX();
 124         double paperY = paper.getImageableY();
 125         paper.setImageableArea(paperX * marginValue, paperY * marginValue,
 126                 paperWidth - (paperX * 2 * marginValue),
 127                 paperHeight - (paperY * 2 * marginValue));
 
 
 105         setValuesForPrintPageSetup(pageFormat, 3);
 
 
I see you call new Paper() above
https://docs.oracle.com/javase/8/docs/api/java/awt/print/Paper.html#Paper--
 
Did you really intend to use a default paper instead of getting the one
from the pageFormat ? On some label printer your Letter Paper may not
even be supported. US (aka NA) Letter is 8.5" wide.
 
Also although it probably will work out OK the maths isn't checking
for boundary problems.
 
default margin will be 1" so that's what you'll get for paperX and paperY
 
Using your value of 3 we set the imagable area such that
imageable X = 1 * 3 = 3
imageableWidth = 8.5 - (1 * 2 *3) = 8.5 - 6 = 2.5;
 
Fortunately that worked out positive .. but it does not seem to be enforced.
 
If we'd used 5 it would be a different story : 
 
ix = 5, iw = 8.5 - ( 1 * 2 * 5) = -1.5
 
The implementation will (should) clamp it to non-negative but it
might still be better to have some defensive logic of your own.
[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog. In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may vary as you pointed out, depending on the locale.
Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are constants), it won’t cause an undesirable behavior like going into negative space.
 
nit: there's a missing space here
 
 75                 } catch(PrinterException e) {
[Shashi] This is fixed now.
 
-phil.
 

On 06/09/2017 03:27 AM, Prasanta Sadhukhan wrote:

looks good to me.

Regards
Prasanta

On 6/9/2017 3:49 PM, Shashidhara Veerabhadraiah wrote:

Hi All, Please find the updated Webrev with fixes for the comments @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_04/

 

Thanks and regards,
Shashi

 

From: Philip Race
Sent: Thursday, June 8, 2017 3:32 AM
To: Prasanta Sadhukhan [hidden email]
Cc: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

.. and please make sure all lines are <= 80 chars as per the coding standards.

-phil.

On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,

79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.
 
Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.
 
Regards
Prasanta

On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.     Have moved the test instructions window under newly created thread.

2.     Have moved the print dialog(main test module) under EDT.

3.     Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 

 

 

 


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

shashi

Just to clarify Phil for the part of cross platform print dialog, here is that dialog representation:

 

 

Since this dialog can appear on any platform, the Paper class would represent the backend object for the front end dialog properties(of a generic printer) as shown in the pic above. Hence the use of the raw Paper object than obtaining it from the page format.

At the same time, as you pointed out, the test is not reliable with the actual conditions. The only problem I see is that the moment we change the test case to stay close to the actual scenario we may have certain failures because the settings are different for different locales and it may be difficult to adapt to every possible locales.

 

Thanks and regards,

Shashi  

 

From: Phil Race
Sent: Monday, June 12, 2017 10:01 PM
To: Shashidhara Veerabhadraiah <[hidden email]>; Prasanta Sadhukhan <[hidden email]>
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

>[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog.
> In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may
> vary as you pointed out, depending on the locale.
>Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are const
> it won’t cause an undesirable behavior like going into negative space.
 
 
Sorry, that is not a valid thing to do. The print dialog is free to ignore this
or workaround the incompatibility of that paper with the supported media so your test is not reliable.
And I don't see what the cross platform print dialog has to do with it.
It is, or should be, just as aware of the printer sizes as the native one.
 
-phil.

 

On 06/12/2017 03:34 AM, Shashidhara Veerabhadraiah wrote:

Hi Phil, Please see below for the comments:

 

The updated Webrev is at:

http://cr.openjdk.java.net/~aghaisas/shashi/6949753/webrev_05/


Modified in what way from the previous version ?

-phil.


 

Thanks and regards,
Shashi

 

From: Phil Race
Sent: Saturday, June 10, 2017 3:07 AM
To: Prasanta Sadhukhan [hidden email]; Shashidhara Veerabhadraiah [hidden email]
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

private static void setValuesForPrintPageSetup(PageFormat pageFormat,  int
 118             marginValue) throws PrinterException {
 119         Paper paper = new Paper();
 double paperHeight = paper.getHeight();
 122         double paperWidth = paper.getWidth();
 123         double paperX = paper.getImageableX();
 124         double paperY = paper.getImageableY();
 125         paper.setImageableArea(paperX * marginValue, paperY * marginValue,
 126                 paperWidth - (paperX * 2 * marginValue),
 127                 paperHeight - (paperY * 2 * marginValue));
 
 
 105         setValuesForPrintPageSetup(pageFormat, 3);
 
 
I see you call new Paper() above
https://docs.oracle.com/javase/8/docs/api/java/awt/print/Paper.html#Paper--
 
Did you really intend to use a default paper instead of getting the one
from the pageFormat ? On some label printer your Letter Paper may not
even be supported. US (aka NA) Letter is 8.5" wide.
 
Also although it probably will work out OK the maths isn't checking
for boundary problems.
 
default margin will be 1" so that's what you'll get for paperX and paperY
 
Using your value of 3 we set the imagable area such that
imageable X = 1 * 3 = 3
imageableWidth = 8.5 - (1 * 2 *3) = 8.5 - 6 = 2.5;
 
Fortunately that worked out positive .. but it does not seem to be enforced.
 
If we'd used 5 it would be a different story : 
 
ix = 5, iw = 8.5 - ( 1 * 2 * 5) = -1.5
 
The implementation will (should) clamp it to non-negative but it
might still be better to have some defensive logic of your own.
[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog. In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may vary as you pointed out, depending on the locale.
Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are constants), it won’t cause an undesirable behavior like going into negative space.
 
nit: there's a missing space here
 
 75                 } catch(PrinterException e) {
[Shashi] This is fixed now.
 
-phil.
 

On 06/09/2017 03:27 AM, Prasanta Sadhukhan wrote:

looks good to me.

Regards
Prasanta

On 6/9/2017 3:49 PM, Shashidhara Veerabhadraiah wrote:

Hi All, Please find the updated Webrev with fixes for the comments @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_04/

 

Thanks and regards,
Shashi

 

From: Philip Race
Sent: Thursday, June 8, 2017 3:32 AM
To: Prasanta Sadhukhan [hidden email]
Cc: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

.. and please make sure all lines are <= 80 chars as per the coding standards.

-phil.

On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,

79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.
 
Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.
 
Regards
Prasanta

On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.     Have moved the test instructions window under newly created thread.

2.     Have moved the print dialog(main test module) under EDT.

3.     Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 

 

 

 

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

Philip Race
The dialog does not show "Letter" because you said so. It shows it because the printer reports it supports letter.

-phil.

On 06/12/2017 10:44 AM, Shashidhara Veerabhadraiah wrote:

Just to clarify Phil for the part of cross platform print dialog, here is that dialog representation:

 

 

Since this dialog can appear on any platform, the Paper class would represent the backend object for the front end dialog properties(of a generic printer) as shown in the pic above. Hence the use of the raw Paper object than obtaining it from the page format.

At the same time, as you pointed out, the test is not reliable with the actual conditions. The only problem I see is that the moment we change the test case to stay close to the actual scenario we may have certain failures because the settings are different for different locales and it may be difficult to adapt to every possible locales.

 

Thanks and regards,

Shashi  

 

From: Phil Race
Sent: Monday, June 12, 2017 10:01 PM
To: Shashidhara Veerabhadraiah [hidden email]; Prasanta Sadhukhan [hidden email]
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

>[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog.
> In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may
> vary as you pointed out, depending on the locale.
>Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are const
> it won’t cause an undesirable behavior like going into negative space.
 
 
Sorry, that is not a valid thing to do. The print dialog is free to ignore this
or workaround the incompatibility of that paper with the supported media so your test is not reliable.
And I don't see what the cross platform print dialog has to do with it.
It is, or should be, just as aware of the printer sizes as the native one.
 
-phil.

 

On 06/12/2017 03:34 AM, Shashidhara Veerabhadraiah wrote:

Hi Phil, Please see below for the comments:

 

The updated Webrev is at:

http://cr.openjdk.java.net/~aghaisas/shashi/6949753/webrev_05/


Modified in what way from the previous version ?

-phil.


 

Thanks and regards,
Shashi

 

From: Phil Race
Sent: Saturday, June 10, 2017 3:07 AM
To: Prasanta Sadhukhan [hidden email]; Shashidhara Veerabhadraiah [hidden email]
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

private static void setValuesForPrintPageSetup(PageFormat pageFormat,  int
 118             marginValue) throws PrinterException {
 119         Paper paper = new Paper();
 double paperHeight = paper.getHeight();
 122         double paperWidth = paper.getWidth();
 123         double paperX = paper.getImageableX();
 124         double paperY = paper.getImageableY();
 125         paper.setImageableArea(paperX * marginValue, paperY * marginValue,
 126                 paperWidth - (paperX * 2 * marginValue),
 127                 paperHeight - (paperY * 2 * marginValue));
 
 
 105         setValuesForPrintPageSetup(pageFormat, 3);
 
 
I see you call new Paper() above
https://docs.oracle.com/javase/8/docs/api/java/awt/print/Paper.html#Paper--
 
Did you really intend to use a default paper instead of getting the one
from the pageFormat ? On some label printer your Letter Paper may not
even be supported. US (aka NA) Letter is 8.5" wide.
 
Also although it probably will work out OK the maths isn't checking
for boundary problems.
 
default margin will be 1" so that's what you'll get for paperX and paperY
 
Using your value of 3 we set the imagable area such that
imageable X = 1 * 3 = 3
imageableWidth = 8.5 - (1 * 2 *3) = 8.5 - 6 = 2.5;
 
Fortunately that worked out positive .. but it does not seem to be enforced.
 
If we'd used 5 it would be a different story : 
 
ix = 5, iw = 8.5 - ( 1 * 2 * 5) = -1.5
 
The implementation will (should) clamp it to non-negative but it
might still be better to have some defensive logic of your own.
[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog. In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may vary as you pointed out, depending on the locale.
Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are constants), it won’t cause an undesirable behavior like going into negative space.
 
nit: there's a missing space here
 
 75                 } catch(PrinterException e) {
[Shashi] This is fixed now.
 
-phil.
 

On 06/09/2017 03:27 AM, Prasanta Sadhukhan wrote:

looks good to me.

Regards
Prasanta

On 6/9/2017 3:49 PM, Shashidhara Veerabhadraiah wrote:

Hi All, Please find the updated Webrev with fixes for the comments @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_04/

 

Thanks and regards,
Shashi

 

From: Philip Race
Sent: Thursday, June 8, 2017 3:32 AM
To: Prasanta Sadhukhan [hidden email]
Cc: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

.. and please make sure all lines are <= 80 chars as per the coding standards.

-phil.

On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,

79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.
 
Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.
 
Regards
Prasanta

On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.     Have moved the test instructions window under newly created thread.

2.     Have moved the print dialog(main test module) under EDT.

3.     Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 

 

 

 

 


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

shashi

Ok Phil. I shall update the test and send it for re-review tomorrow.

 

Thanks and regards,
Shashi

 

From: Phil Race
Sent: Monday, June 12, 2017 11:15 PM
To: Shashidhara Veerabhadraiah <[hidden email]>; Prasanta Sadhukhan <[hidden email]>
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

The dialog does not show "Letter" because you said so. It shows it because the printer reports it supports letter.

-phil.

On 06/12/2017 10:44 AM, Shashidhara Veerabhadraiah wrote:

Just to clarify Phil for the part of cross platform print dialog, here is that dialog representation:

 

 

Since this dialog can appear on any platform, the Paper class would represent the backend object for the front end dialog properties(of a generic printer) as shown in the pic above. Hence the use of the raw Paper object than obtaining it from the page format.

At the same time, as you pointed out, the test is not reliable with the actual conditions. The only problem I see is that the moment we change the test case to stay close to the actual scenario we may have certain failures because the settings are different for different locales and it may be difficult to adapt to every possible locales.

 

Thanks and regards,

Shashi  

 

From: Phil Race
Sent: Monday, June 12, 2017 10:01 PM
To: Shashidhara Veerabhadraiah [hidden email]; Prasanta Sadhukhan [hidden email]
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

>[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog.
> In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may
> vary as you pointed out, depending on the locale.
>Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are const
> it won’t cause an undesirable behavior like going into negative space.
 
 
Sorry, that is not a valid thing to do. The print dialog is free to ignore this
or workaround the incompatibility of that paper with the supported media so your test is not reliable.
And I don't see what the cross platform print dialog has to do with it.
It is, or should be, just as aware of the printer sizes as the native one.
 
-phil.

 

On 06/12/2017 03:34 AM, Shashidhara Veerabhadraiah wrote:

Hi Phil, Please see below for the comments:

 

The updated Webrev is at:

http://cr.openjdk.java.net/~aghaisas/shashi/6949753/webrev_05/


Modified in what way from the previous version ?

-phil.



 

Thanks and regards,
Shashi

 

From: Phil Race
Sent: Saturday, June 10, 2017 3:07 AM
To: Prasanta Sadhukhan [hidden email]; Shashidhara Veerabhadraiah [hidden email]
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

private static void setValuesForPrintPageSetup(PageFormat pageFormat,  int
 118             marginValue) throws PrinterException {
 119         Paper paper = new Paper();
 double paperHeight = paper.getHeight();
 122         double paperWidth = paper.getWidth();
 123         double paperX = paper.getImageableX();
 124         double paperY = paper.getImageableY();
 125         paper.setImageableArea(paperX * marginValue, paperY * marginValue,
 126                 paperWidth - (paperX * 2 * marginValue),
 127                 paperHeight - (paperY * 2 * marginValue));
 
 
 105         setValuesForPrintPageSetup(pageFormat, 3);
 
 
I see you call new Paper() above
https://docs.oracle.com/javase/8/docs/api/java/awt/print/Paper.html#Paper--
 
Did you really intend to use a default paper instead of getting the one
from the pageFormat ? On some label printer your Letter Paper may not
even be supported. US (aka NA) Letter is 8.5" wide.
 
Also although it probably will work out OK the maths isn't checking
for boundary problems.
 
default margin will be 1" so that's what you'll get for paperX and paperY
 
Using your value of 3 we set the imagable area such that
imageable X = 1 * 3 = 3
imageableWidth = 8.5 - (1 * 2 *3) = 8.5 - 6 = 2.5;
 
Fortunately that worked out positive .. but it does not seem to be enforced.
 
If we'd used 5 it would be a different story : 
 
ix = 5, iw = 8.5 - ( 1 * 2 * 5) = -1.5
 
The implementation will (should) clamp it to non-negative but it
might still be better to have some defensive logic of your own.
[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog. In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may vary as you pointed out, depending on the locale.
Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are constants), it won’t cause an undesirable behavior like going into negative space.
 
nit: there's a missing space here
 
 75                 } catch(PrinterException e) {
[Shashi] This is fixed now.
 
-phil.
 

On 06/09/2017 03:27 AM, Prasanta Sadhukhan wrote:

looks good to me.

Regards
Prasanta

On 6/9/2017 3:49 PM, Shashidhara Veerabhadraiah wrote:

Hi All, Please find the updated Webrev with fixes for the comments @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_04/

 

Thanks and regards,
Shashi

 

From: Philip Race
Sent: Thursday, June 8, 2017 3:32 AM
To: Prasanta Sadhukhan [hidden email]
Cc: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

.. and please make sure all lines are <= 80 chars as per the coding standards.

-phil.

On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,

79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.
 
Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.
 
Regards
Prasanta

On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.     Have moved the test instructions window under newly created thread.

2.     Have moved the print dialog(main test module) under EDT.

3.     Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 

 

 

 

 

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

shashi

Hi, Here is the updated Webrev with comment fixes:

 

http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_06/

 

Thanks and regards,

Shashi

 

From: Shashidhara Veerabhadraiah
Sent: Monday, June 12, 2017 11:19 PM
To: Philip Race <[hidden email]>; Prasanta Sadhukhan <[hidden email]>
Cc: [hidden email]
Subject: RE: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Ok Phil. I shall update the test and send it for re-review tomorrow.

 

Thanks and regards,
Shashi

 

From: Phil Race
Sent: Monday, June 12, 2017 11:15 PM
To: Shashidhara Veerabhadraiah <[hidden email]>; Prasanta Sadhukhan <[hidden email]>
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

The dialog does not show "Letter" because you said so. It shows it because the printer reports it supports letter.

-phil.

On 06/12/2017 10:44 AM, Shashidhara Veerabhadraiah wrote:

Just to clarify Phil for the part of cross platform print dialog, here is that dialog representation:

 

 

Since this dialog can appear on any platform, the Paper class would represent the backend object for the front end dialog properties(of a generic printer) as shown in the pic above. Hence the use of the raw Paper object than obtaining it from the page format.

At the same time, as you pointed out, the test is not reliable with the actual conditions. The only problem I see is that the moment we change the test case to stay close to the actual scenario we may have certain failures because the settings are different for different locales and it may be difficult to adapt to every possible locales.

 

Thanks and regards,

Shashi  

 

From: Phil Race
Sent: Monday, June 12, 2017 10:01 PM
To: Shashidhara Veerabhadraiah [hidden email]; Prasanta Sadhukhan [hidden email]
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

>[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog.
> In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may
> vary as you pointed out, depending on the locale.
>Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are const
> it won’t cause an undesirable behavior like going into negative space.
 
 
Sorry, that is not a valid thing to do. The print dialog is free to ignore this
or workaround the incompatibility of that paper with the supported media so your test is not reliable.
And I don't see what the cross platform print dialog has to do with it.
It is, or should be, just as aware of the printer sizes as the native one.
 
-phil.

 

On 06/12/2017 03:34 AM, Shashidhara Veerabhadraiah wrote:

Hi Phil, Please see below for the comments:

 

The updated Webrev is at:

http://cr.openjdk.java.net/~aghaisas/shashi/6949753/webrev_05/


Modified in what way from the previous version ?

-phil.


 

Thanks and regards,
Shashi

 

From: Phil Race
Sent: Saturday, June 10, 2017 3:07 AM
To: Prasanta Sadhukhan [hidden email]; Shashidhara Veerabhadraiah [hidden email]
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

private static void setValuesForPrintPageSetup(PageFormat pageFormat,  int
 118             marginValue) throws PrinterException {
 119         Paper paper = new Paper();
 double paperHeight = paper.getHeight();
 122         double paperWidth = paper.getWidth();
 123         double paperX = paper.getImageableX();
 124         double paperY = paper.getImageableY();
 125         paper.setImageableArea(paperX * marginValue, paperY * marginValue,
 126                 paperWidth - (paperX * 2 * marginValue),
 127                 paperHeight - (paperY * 2 * marginValue));
 
 
 105         setValuesForPrintPageSetup(pageFormat, 3);
 
 
I see you call new Paper() above
https://docs.oracle.com/javase/8/docs/api/java/awt/print/Paper.html#Paper--
 
Did you really intend to use a default paper instead of getting the one
from the pageFormat ? On some label printer your Letter Paper may not
even be supported. US (aka NA) Letter is 8.5" wide.
 
Also although it probably will work out OK the maths isn't checking
for boundary problems.
 
default margin will be 1" so that's what you'll get for paperX and paperY
 
Using your value of 3 we set the imagable area such that
imageable X = 1 * 3 = 3
imageableWidth = 8.5 - (1 * 2 *3) = 8.5 - 6 = 2.5;
 
Fortunately that worked out positive .. but it does not seem to be enforced.
 
If we'd used 5 it would be a different story : 
 
ix = 5, iw = 8.5 - ( 1 * 2 * 5) = -1.5
 
The implementation will (should) clamp it to non-negative but it
might still be better to have some defensive logic of your own.
[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog. In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may vary as you pointed out, depending on the locale.
Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are constants), it won’t cause an undesirable behavior like going into negative space.
 
nit: there's a missing space here
 
 75                 } catch(PrinterException e) {
[Shashi] This is fixed now.
 
-phil.
 

On 06/09/2017 03:27 AM, Prasanta Sadhukhan wrote:

looks good to me.

Regards
Prasanta

On 6/9/2017 3:49 PM, Shashidhara Veerabhadraiah wrote:

Hi All, Please find the updated Webrev with fixes for the comments @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_04/

 

Thanks and regards,
Shashi

 

From: Philip Race
Sent: Thursday, June 8, 2017 3:32 AM
To: Prasanta Sadhukhan [hidden email]
Cc: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

.. and please make sure all lines are <= 80 chars as per the coding standards.

-phil.

On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,

79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.
 
Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.
 
Regards
Prasanta

On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.     Have moved the test instructions window under newly created thread.

2.     Have moved the print dialog(main test module) under EDT.

3.     Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 

 

 

 

 

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

Philip Race
displays is misspelt as diplsays in at least two cases.
imageable is misspelt as imagable in at least one case.

"imageable area retains even after closing in the"

Are you trying to say "is retained" ?

I am still not happy that you are hard-coding values "2" and "3" in the instructions
If the printer's default paper is < 6 inches wide this will fail.

The whole thing should be adaptive to what it actually sees.

I am also unclear how you are so sure it'll be inches ?
I'd expect most locales to use centimetres, but what you see depends
on the platform .. which I expect is why you excluded Mac ?
But why are you excluding Solaris too ?

java.awt.print.PrinterJob.getPrinterJob()

You are importing all the other classes from java.awt.print,
so why not this one too ?

Whilst I don't have any objection to throwing a runtime exception
if the tester presses your FAIL button, you could have just used
the "yes/no" support in the jtreg test harness.

-phil.


On 6/21/17, 1:33 AM, Shashidhara Veerabhadraiah wrote:

Hi, Here is the updated Webrev with comment fixes:

 

http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_06/

 

Thanks and regards,

Shashi

 

From: Shashidhara Veerabhadraiah
Sent: Monday, June 12, 2017 11:19 PM
To: Philip Race [hidden email]; Prasanta Sadhukhan [hidden email]
Cc: [hidden email]
Subject: RE: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Ok Phil. I shall update the test and send it for re-review tomorrow.

 

Thanks and regards,
Shashi

 

From: Phil Race
Sent: Monday, June 12, 2017 11:15 PM
To: Shashidhara Veerabhadraiah <[hidden email]>; Prasanta Sadhukhan <[hidden email]>
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

The dialog does not show "Letter" because you said so. It shows it because the printer reports it supports letter.

-phil.

On 06/12/2017 10:44 AM, Shashidhara Veerabhadraiah wrote:

Just to clarify Phil for the part of cross platform print dialog, here is that dialog representation:

 

 

Since this dialog can appear on any platform, the Paper class would represent the backend object for the front end dialog properties(of a generic printer) as shown in the pic above. Hence the use of the raw Paper object than obtaining it from the page format.

At the same time, as you pointed out, the test is not reliable with the actual conditions. The only problem I see is that the moment we change the test case to stay close to the actual scenario we may have certain failures because the settings are different for different locales and it may be difficult to adapt to every possible locales.

 

Thanks and regards,

Shashi  

 

From: Phil Race
Sent: Monday, June 12, 2017 10:01 PM
To: Shashidhara Veerabhadraiah [hidden email]; Prasanta Sadhukhan [hidden email]
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

>[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog.
> In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may
> vary as you pointed out, depending on the locale.
>Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are const
> it won’t cause an undesirable behavior like going into negative space.
 
 
Sorry, that is not a valid thing to do. The print dialog is free to ignore this
or workaround the incompatibility of that paper with the supported media so your test is not reliable.
And I don't see what the cross platform print dialog has to do with it.
It is, or should be, just as aware of the printer sizes as the native one.
 
-phil.

 

On 06/12/2017 03:34 AM, Shashidhara Veerabhadraiah wrote:

Hi Phil, Please see below for the comments:

 

The updated Webrev is at:

http://cr.openjdk.java.net/~aghaisas/shashi/6949753/webrev_05/


Modified in what way from the previous version ?

-phil.


 

Thanks and regards,
Shashi

 

From: Phil Race
Sent: Saturday, June 10, 2017 3:07 AM
To: Prasanta Sadhukhan [hidden email]; Shashidhara Veerabhadraiah [hidden email]
Cc: [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

private static void setValuesForPrintPageSetup(PageFormat pageFormat,  int
 118             marginValue) throws PrinterException {
 119         Paper paper = new Paper();
 double paperHeight = paper.getHeight();
 122         double paperWidth = paper.getWidth();
 123         double paperX = paper.getImageableX();
 124         double paperY = paper.getImageableY();
 125         paper.setImageableArea(paperX * marginValue, paperY * marginValue,
 126                 paperWidth - (paperX * 2 * marginValue),
 127                 paperHeight - (paperY * 2 * marginValue));
 
 
 105         setValuesForPrintPageSetup(pageFormat, 3);
 
 
I see you call new Paper() above
https://docs.oracle.com/javase/8/docs/api/java/awt/print/Paper.html#Paper--
 
Did you really intend to use a default paper instead of getting the one
from the pageFormat ? On some label printer your Letter Paper may not
even be supported. US (aka NA) Letter is 8.5" wide.
 
Also although it probably will work out OK the maths isn't checking
for boundary problems.
 
default margin will be 1" so that's what you'll get for paperX and paperY
 
Using your value of 3 we set the imagable area such that
imageable X = 1 * 3 = 3
imageableWidth = 8.5 - (1 * 2 *3) = 8.5 - 6 = 2.5;
 
Fortunately that worked out positive .. but it does not seem to be enforced.
 
If we'd used 5 it would be a different story : 
 
ix = 5, iw = 8.5 - ( 1 * 2 * 5) = -1.5
 
The implementation will (should) clamp it to non-negative but it
might still be better to have some defensive logic of your own.
[Shashi] Yes. I intend to use the default paper object as this is a test related to the cross platform default printer dialog. In the modified test file, I have set the size of the physical paper instead of relying it on the default setting which may vary as you pointed out, depending on the locale.
Now that we have our own paper object(with a constant paper size) and based on the margin setting(which are constants), it won’t cause an undesirable behavior like going into negative space.
 
nit: there's a missing space here
 
 75                 } catch(PrinterException e) {
[Shashi] This is fixed now.
 
-phil.
 

On 06/09/2017 03:27 AM, Prasanta Sadhukhan wrote:

looks good to me.

Regards
Prasanta

On 6/9/2017 3:49 PM, Shashidhara Veerabhadraiah wrote:

Hi All, Please find the updated Webrev with fixes for the comments @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_04/

 

Thanks and regards,
Shashi

 

From: Philip Race
Sent: Thursday, June 8, 2017 3:32 AM
To: Prasanta Sadhukhan [hidden email]
Cc: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

.. and please make sure all lines are <= 80 chars as per the coding standards.

-phil.

On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:

do_test() does not need to be under EDT as it invokes printer pagedialog and not swing components. Actually, createUI() needs to be under EDT which has not been done.

Also,

79         SwingUtilities.invokeAndWait(() -> {
  80             test.disposeUI();
  81         });
  82     }
should be called before you throw RuntimeException when test times out . 
There is no need of calling this after
75         if (test.testResult == false) {
  76             throw new RuntimeException("Test Failed.");
  77         }
as it has already been called in pass/fail actionlistener.
 
Also, put a sleep after T1.start() and do_test() otherwise since they are in separate thread, in mycase, pagedialog is displayed before test instructions dialog.
 
Regards
Prasanta

On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah wrote:

Hi All,

  I have altered the manual test template per the comments.

 

1.     Have moved the test instructions window under newly created thread.

2.     Have moved the print dialog(main test module) under EDT.

3.     Timer management shall be done on the main thread.

 

I have placed the updated Webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/

Please let me know if any comments on it.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Tuesday, June 6, 2017 11:52 AM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

As I told, pageDialog is modal so latch.await() will not be called if user does not close the page dialog or do any interaction. The actual test

59         PageFormat pageFormat = new PageFormat();
  60 
  61         createNewPrintPageSetup(pageFormat);
  62 
  63         setValuesForPrintPageSetup(pageFormat, 2);
  64 
  65         createNewPrintPageSetup(pageFormat);
  66 
  67         setValuesForPrintPageSetup(pageFormat, 3);
  68 
  69         createNewPrintPageSetup(pageFormat);


should be done in other thread.

Regards
Prasanta

On 6/6/2017 11:24 AM, Shashidhara Veerabhadraiah wrote:

The manual test template that I received from the team seems buggy and an older version it seems. I have modified the same per your inputs and now placed the updated Webrev at http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/.

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Monday, June 5, 2017 12:35 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

I guess there is one more problem in usage of CountDown latch. Have you seen this test fail with timeout even if you wait for 5 minutes as per your timeout period?

latch.await() needs to be wait on main thread while the test needs to be executed in another thread otherwise, pageDialog being modal the control will not come to latch.await()

Iguess you need to do this.

TestUI test = new TestUI(latch);
        Thread T1 = new Thread(test);
        T1.start();

class TestUI implements Runnable {
...
@Override
    public void run() {
        try {
            createUI();

Regards
Prasanta

On 6/2/2017 4:00 PM, Shashidhara Veerabhadraiah wrote:

Hi, I have fixed the comments below and updated the webrev @ http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/

 

Thanks and regards,

Shashi

 

From: Prasanta Sadhukhan
Sent: Friday, June 2, 2017 12:36 PM
To: Shashidhara Veerabhadraiah [hidden email]; [hidden email]
Cc: Philip Race [hidden email]
Subject: Re: [9]JDK-6949753:[TEST BUG]: java/awt/print/PageFormat/PDialogTest.java needs update by removing a infinite loop

 

Test fix look ok. Only thing is, you can call getPrinterJob() once and reutilise instead of calling 3 times and probably there is no need of creating a function createNewPrintPageSetup() for it (as it calls 1 method) but it is upto you.

Few comments:

Copyright should have "," after 2017.
I guess createUI() does not have any call that throws exception so no need to have try-catch block for createUI().
Also, there is no need to catch PrinterException and rethrow RuntimeException, so you can do away with that try-catch.
Also, you can call disposeUI() in passButton and failButton actionlistener instead of in main().  Also, there is no need to do setVisible(false) in disposeUI(), dispose() will take care of that.
You can throw RuntimeException when test timed out (instead of just println and later getting test fail exception) which is different from Test Failed RuntimeException.

Regards
Prasanta

On 6/1/2017 5:10 PM, Shashidhara Veerabhadraiah wrote:

Hi All,
Please review a fix for a test bug which contained an infinite loop to test the printer setup dialog's margin attributes retention without the manual step procedure.
 
The issue with PDialogTest.java which tests the printer setup dialog's margin attributes retention by having as infinite loop to keep popping up the dialog without a proper exit. The test does not cover the instruction steps necessary to properly test dialog's margin attributes retention.
 
The updated test file includes the standard manual test template along with test cases to cover the printer dialog's margin attributes retention feature.
 
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
 
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
 
Note : PrintDialog on Mac does not show page margins and hence this test does not run on Mac.
 
Thanks and regards,
Shashi

 

 

 

 

 

 

 

 

12
Loading...