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