Ping: RFR: JDK-8209604: [TEST] rewrite com/sun/jdi shell tests to java version - step2

Alex Menkov alexey.menkov at oracle.com
Wed Aug 29 22:04:32 UTC 2018 

Hi Chris,

Updated fix:
http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev.02/

see replies inline.

On 08/28/2018 23:18, Chris Plummer wrote:
> Hi Daniil,
> 
> Overall the changes look good. A few cleanup suggestions are below:
> 
>    63         setBreakpoints(System.getProperty("test.src") + 
> "/CatchAllTest.java", 1);
> 
> This code is repeated a lot, and all current uses always use test.src. 
> Can the getProperty part just be moved into setBreakpoints?

Done.
Renamed the method name to ensure all calls are updated.
Had to update some other tests which use the method.
Also replaced calls of jdb.getJdbOutput()/jdb.getDebuggeeOutput() with 
JdbTest.getJdbOutput()/JdbTest.getDebuggeeOutput() as it's required for 
upcoming fixes to decrease noise in the future.

> 
>   280         final String invalidTypeException = "InvalidTypeException";
>   281
>   282 evalShouldContain("EvalArgsTarg.ffboolean(EvalArgsTarg.jjbyte)", 
> invalidTypeException);
>   283
>   284 evalShouldContain("EvalArgsTarg.ffintArray(EvalArgsTarg.jjint)", 
> invalidTypeException);
>   285
>   286 
> evalShouldContain("EvalArgsTarg.ffintArray(EvalArgsTarg.jjfloatArray)", 
> invalidTypeException);
>   287
>   288 evalShouldContain("EvalArgsTarg.ffjj2(EvalArgsTarg.myjj1)", 
> invalidTypeException);
>   289
>   290 evalShouldContain("EvalArgsTarg.ffjj2(EvalArgsTarg.myoranges)", 
> invalidTypeException);
> 
> I think the invalidTypeException variable is unnecessary and just causes 
> the reader to go find what it is actually set to. Also, line spacing is 
> not needed here.

The idea of the constant is to avoid typos in string constants (we need 
to ensure the string is correct only in one place, then compiler doesn't 
allow to mistype variable name).
Line spacing is dropped so variable definition is close to the calls.

> 
>    42  * The test makes sure that it is, at all, possible to invoke an 
> interface
>    43  * static method and that the static methods are not inherited by 
> extending
>    44  * interfaces.
> 
> I think the "at all" comment should be removed. No idea why it is there.

Done.

> 
>    40 /*
>    41  * This is another test of the same bug.  The bug occurs when trying
>    42  * to walk the stack of a deoptimized thread.  We can do this
>    43  * by running in -Xcomp mode and by doing a step which causes deopt,
>    44  * and then a 'where'.  This will cause not all the frames to be 
> shown.
>    45  */
> 
> I know you didn't change this comment, but what is "the same bug" in 
> reference too. Also, is says tested using -Xcomp, but the original shell 
> script version never seemed to do this. I guess it relied on -Xcomp 
> testing for the whole test run. You seem to have fixed this in the java 
> version so the debugee is always run with -Xcomp, which seems like the 
> right thing to do.

In the test header:
  * @test
  * @bug 4525714
  * @summary jtreg test PopAsynchronousTest fails in build 85 with -Xcomp
  * @comment converted from test/jdk/com/sun/jdi/DeoptimizeWalk.sh
I.e. the bug (4525714) is about failing PopAsynchronousTest test, but 
this test (DeoptimizeWalk) performs additional testing for the issue.

> 
>    39  * The bug is that, for example, if a String is passed
>    40  * as an arg to a func where an Object is expected,
>    41  * the above error occurs.  jdb doesnt notice that this is
>    42  * legal because String is an instance of Object.
> 
> Another unmodified comment that could use some cleaning up. What is "the 
> above error"? A better description of what is actually being tested 
> would be useful. Change "doesnt" to "doesn't".

This case is similar - look at the test header
  * @test
  * @bug 4663146
  * @summary Arguments match no method error
  * @comment converted from test/jdk/com/sun/jdi/EvalArgs.sh

So "the above error" is "Arguments match no method" error.

 > Change "doesnt" to "doesn't"
Done.

> 
> BTW, the navigation buttons at the bottom of your webrev don't seem to 
> work starting with the 3rd file. The URL gets messed up.

As far as I see the problem is renamed files - for some reason webrev 
doesn't correctly handle theirs URLs (the URLs contain both old and new 
names).
Sorry, have no idea how to fix it.

--alex

> 
> thanks,
> 
> Chris
> 
> On 8/28/18 5:06 PM, serguei.spitsyn at oracle.com wrote:
>> Sorry, forgot to add Chris - fixed now.
>> Still it'd be good to get a confirmation from Chris.
>>
>> Thanks,
>> Serguei
>>
>> On 8/28/18 16:38, Alex Menkov wrote:
>>> Looks like you sent the message only to me :)
>>> Anyway I've got 2nd review from Jc.
>>>
>>> --alex
>>>
>>> On 08/28/2018 16:20, serguei.spitsyn at oracle.com wrote:
>>>> Hi Chris,
>>>>
>>>> I guess, Alex is waiting for your final thumbs up.
>>>> Alex, please, fix me if it is wrong.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 8/28/18 14:03, Alex Menkov wrote:
>>>>> Need one more reviewer for the fix.
>>>>>
>>>>> --alex
>>>>>
>>>>> On 08/22/2018 10:18, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Alex,
>>>>>>
>>>>>> This looks good.
>>>>>> Thank you for the update!
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 8/21/18 17:10, Alex Menkov wrote:
>>>>>>> Hi guys,
>>>>>>>
>>>>>>> Updated webrev:
>>>>>>> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev.01/
>>>>>>>
>>>>>>> changes (vs initial fix):
>>>>>>> - Eval*.java tests are updated, common methods are moved into 
>>>>>>> JdbTest;
>>>>>>> - $classname substitution is dropped in EvalArgs.java;
>>>>>>> - comment are reintroduced;
>>>>>>> - line separator logic was dropped from JdbCommand (it's not 
>>>>>>> needed as JdbCommand contains static factory methods to create 
>>>>>>> commands).
>>>>>>>
>>>>>>> Update of String.split to use "\\R" is done in other fix:
>>>>>>> JDK-8209605: com/sun/jdi/BreakpointWithFullGC.java fails with ZGC
>>>>>>>
>>>>>>> --alex
>>>>>>>
>>>>>>> On 08/20/2018 19:12, David Holmes wrote:
>>>>>>>> Picking up on one point ...
>>>>>>>>
>>>>>>>> On 21/08/2018 9:40 AM, serguei.spitsyn at oracle.com wrote:
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>> It looks good in general.
>>>>>>>>> Some minor comments below.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/lib/jdb/JdbCommand.java.frames.html 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>   132     private static final String ls = 
>>>>>>>>> System.getProperty("line.separator");
>>>>>>>>>
>>>>>>>>>    Minor: Replace variable name "ls" with "LINE_SEP" (in upper 
>>>>>>>>> case as it is a static final).
>>>>>>>>
>>>>>>>> There have been bugs in the past where tests use the wrong 
>>>>>>>> line-separator because there is confusion as to which output 
>>>>>>>> comes via the OS (and so has platform line-separators) and which 
>>>>>>>> comes more directly (e.g. via socket) and so has the language 
>>>>>>>> line-separator. Are we sure we will always be dealing with the 
>>>>>>>> platform line-separator here?
>>>>>>>>
>>>>>>>> For String.split use of the regex \\R for the line-separators 
>>>>>>>> seems the best solution.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArgs.java.frames.html 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>    Just wanted to double-check that this check:
>>>>>>>>> 262 jdbFailIfPresent "Arguments match no method"
>>>>>>>>>
>>>>>>>>>    works in each call of the verifyNoArgumentsMatchMethod()
>>>>>>>>>    instead of just once as it was originally.
>>>>>>>>>
>>>>>>>>> 214 private void verifyNoArgumentsMatchMethod(String expr) {
>>>>>>>>> 215 List<String> reply = 
>>>>>>>>> jdb.command(JdbCommand.eval(expr.replace("$classname", 
>>>>>>>>> DEBUGGEE_CLASS)));
>>>>>>>>> 216 new 
>>>>>>>>> OutputAnalyzer(reply.stream().collect(Collectors.joining(lineSeparator))) 
>>>>>>>>>
>>>>>>>>> 217 .shouldNotContain("Arguments match no method");
>>>>>>>>> 218 }
>>>>>>>>>
>>>>>>>>>   Minor: It feels like the method name need a tweak.
>>>>>>>>>          Something like: verifyNoArgumentsMatchNoMethod
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 292 // These overload calls should fail because ther Minor: A 
>>>>>>>>> suggestion is to fix a typo at the end of comment.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArraysAsList.java.frames.html 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>    The comment was not converted:
>>>>>>>>>
>>>>>>>>> 31 # The test checks if evaluation of the expression 
>>>>>>>>> java.util.Arrays.asList(null, "a")
>>>>>>>>> 32 # works normally and does not throw an 
>>>>>>>>> IllegalArgumentException.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalInterfaceStatic.java.frames.html 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>    The comment was not converted:
>>>>>>>>>
>>>>>>>>> 33 # The test exercises the ability to invoke static methods on 
>>>>>>>>> interfaces.
>>>>>>>>> 34 # Static interface methods are a new feature added in JDK8.
>>>>>>>>> 35 #
>>>>>>>>> 36 # The test makes sure that it is, at all, possible to invoke 
>>>>>>>>> an interface
>>>>>>>>> 37 # static method and that the static methods are not 
>>>>>>>>> inherited by extending
>>>>>>>>> 38 # interfaces.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Serguei
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8/16/18 14:13, Alex Menkov wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> Please review next chunk of shell->java test conversion.
>>>>>>>>>> jira: https://bugs.openjdk.java.net/browse/JDK-8209604
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/
>>>>>>>>>>
>>>>>>>>>> The fix contains some changes in library classes:
>>>>>>>>>> Jdb.java - timeouts are updated (as per Dan note in one of 
>>>>>>>>>> previous review, timeouts should respect timeout factor, 
>>>>>>>>>> Utils.adjustTimeout implements the functionality);
>>>>>>>>>> JdbCommand.java - new jdb commands (required by tests) are added.
>>>>>>>>>>
>>>>>>>>>> --alex
>>>>>>>>>
>>>>>>
>>>>
>>
> 
>

More information about the serviceability-dev mailing list