RFR: JDK-8209604: [TEST] rewrite com/sun/jdi shell tests to java version - step2
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Aug 22 17:18:23 UTC 2018
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