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

Chris Plummer chris.plummer at oracle.com
Thu Aug 30 02:08:20 UTC 2018


Hi Alex,

Overall looks good. Just a couple minor comment suggestions below.

On 8/29/18 3:04 PM, Alex Menkov wrote:
> 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.
Can you capture this in the comment?
>
>>
>>    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.
Also capture this in the comment.
>
> > 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.
Ok.

thanks,

Chris
>
> --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