Re: RFR JDK-8209109: [TEST] rewrite com/sun/jdi shell tests to java version - step1

2018-08-09 Thread serguei.spit...@oracle.com 

Hi Alex,

I'm Okay with this keyword removal.
Thank you for checking.

Thanks,
Serguei


On 8/9/18 14:59, Alex Menkov wrote:

Hi Serguei,

On 08/07/2018 20:53, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good to me - good start!
The only question I have is that the following line is dropped for 
both tests:

@key intermittent

Just wanted to double check if the SQE team is still using it
to mark the intermittently failing tests.


SQE tells that they do not use it.
The keyword was added to intermittently failing tests
(looks like jdk9 has too many such tests, so the goal way to simplify 
failure analysis).


Also one of the goals of shell to java conversion is to resolve tests 
instability, so it looks logical to remove the keyword as part of the 
conversion fix.


--alex



Thanks,
Serguei



On 8/7/18 17:09, Alex Menkov wrote:

Hi all,

Please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8209109
webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/webrev/

This is a first step in shell to java conversion of com/sun/jdi tests
The fix introduces base classes to work with jdb and converts couple 
tests.


--alex

Re: RFR JDK-8209109: [TEST] rewrite com/sun/jdi shell tests to java version - step1

2018-08-09 Thread Alex Menkov 

Hi Jc,

Thank you for the review.
Initial implementation had only a single wait while outputHandler get 
some new data.
But handling jdb output (actually detection of the point when jdb 
completes command execution and waits for user input) is quite tricky.
So after researching existing code which works with jdb I decided to add 
extra delays to minimize probability of false detection.


--alex

On 08/09/2018 11:46, JC Beyler wrote:

Hi Alexey,

I'm evidently not a reviewer but the webrev looks great to me. Since the 
new classes are to be used for other tests, I was looking a bit more 
into them. I saw a small nit on the waitForMsg method:


In 
http://cr.openjdk.java.net/~amenkov/sh2java/webrev/test/jdk/com/sun/jdi/lib/jdb/Jdb.java.html 
:

  174             try {
  175                 Thread.sleep(sleepTime);
  176             } catch (InterruptedException e) {
  177                 // ignore
  178             }

Seems like that try/catch could be removed entirely. The next block 
says: if the outputHandler is empty, wait on it for a notify. So instead 
of doing a :


   - Thread wait
   - Check on outputHandler and wait on it

You would only check on the outputHandler and wake up when something new 
came in. That might however provoke a lot of wake/notifies, on the other 
hand, right now you do:


- Thread wait
- Check if there is something and wake up at the first write
- If that first write does not have the message
- Thread wait

So, then the question becomes, is it really not simpler to "just" have 
the thread wait?


But apart from that (and even with it), it looks great to me :)

Thanks for doing this!
Jc





On Tue, Aug 7, 2018 at 5:09 PM Alex Menkov > wrote:


Hi all,

Please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8209109
webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/webrev/


This is a first step in shell to java conversion of com/sun/jdi tests
The fix introduces base classes to work with jdb and converts couple
tests.

--alex



--

Thanks,
Jc

Re: RFR JDK-8209109: [TEST] rewrite com/sun/jdi shell tests to java version - step1

2018-08-09 Thread Alex Menkov 

Hi Serguei,

On 08/07/2018 20:53, serguei.spit...@oracle.com wrote:

Hi Alex,

It looks good to me - good start!
The only question I have is that the following line is dropped for both 
tests:

@key intermittent

Just wanted to double check if the SQE team is still using it
to mark the intermittently failing tests.


SQE tells that they do not use it.
The keyword was added to intermittently failing tests
(looks like jdk9 has too many such tests, so the goal way to simplify 
failure analysis).


Also one of the goals of shell to java conversion is to resolve tests 
instability, so it looks logical to remove the keyword as part of the 
conversion fix.


--alex



Thanks,
Serguei



On 8/7/18 17:09, Alex Menkov wrote:

Hi all,

Please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8209109
webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/webrev/

This is a first step in shell to java conversion of com/sun/jdi tests
The fix introduces base classes to work with jdb and converts couple 
tests.


--alex

Re: RFR JDK-8209109: [TEST] rewrite com/sun/jdi shell tests to java version - step1

2018-08-07 Thread serguei.spit...@oracle.com 

  
  
Hi Alex,
  
  It looks good to me - good start!
  The only question I have is that the following line is dropped for
  both tests:
    @key intermittent

Just wanted to double check if the SQE team is still using it
to mark the intermittently
failing tests.

Thanks,
Serguei



  On 8/7/18 17:09, Alex Menkov wrote:

Hi all,
  
  
  Please review a fix for
  
  https://bugs.openjdk.java.net/browse/JDK-8209109
  
  webrev:
  
  http://cr.openjdk.java.net/~amenkov/sh2java/webrev/
  
  
  This is a first step in shell to java conversion of com/sun/jdi
  tests
  
  The fix introduces base classes to work with jdb and converts
  couple tests.
  
  
  --alex