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?
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.
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.
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.
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".
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.
thanks,
Chris
On 8/28/18 5:06 PM, serguei.spit...@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.spit...@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.spit...@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.spit...@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
