RFR JDK-8210243: [TEST] rewrite com/sun/jdi shell tests to java version - step3
Alex Menkov
alexey.menkov at oracle.com
Fri Aug 31 17:49:49 UTC 2018
Hi Jc,
On 08/31/2018 10:25, JC Beyler wrote:
> Hi Alexey,
>
> Apart from Serguei's comment above, I only saw two nits:
>
> A) The shorthand
>
> http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/lib/jdb/JdbTest.java.udiff.html
> <http://cr.openjdk.java.net/%7Eamenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/lib/jdb/JdbTest.java.udiff.html>
>
> 1) typo shortland -> shorthand
> 2) could we just have a method instead of the shorthand? ie:
> instead of:
> + protected final String debuggeeClass; // shortland for
> launchOptions.debuggeeClass
>
> we have:
> + protected String debuggeeClass() { return
> launchOptions.debuggeeClass; }
>
> Somehow, even as a final, duplication of data seems weird to me. (On the
> other hand, it seems you only use launchOptions.debuggeeClass in two
> spots in the code and once you used the shorthand and once you didn't.
> So perhaps no shorthand at all makes sense?)
Yes, agree - there is no sense to have the field.
>
> B) Not testing the command that works?
>
> http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/NotAField.java.udiff.html
> <http://cr.openjdk.java.net/%7Eamenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/NotAField.java.udiff.html>
> -> I'm surprised we are testing a command that should work and then a
> command that should not but only are checking the failing command's
> output. I know this is a direct translation of the test but it seems
> weird to not be testing also the hashCode() call, no?
The original issue (https://bugs.openjdk.java.net/browse/JDK-4467887) is
about "more graceful message when user tries to evaluate a method such
as length() or toString() as a field".
So actually this "correct" command is not needed (I suppose the command
is executed just because test example in the bug contains it for comparison)
I'll do a test run and then upload new webrev.
--alex
>
> Thanks,
> Jc
>
>
>
> On Thu, Aug 30, 2018 at 11:13 PM serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com>> wrote:
>
> Hi Alex,
>
> It looks good in general but not sure I understand all the changes.
> Could you, please, tell a little bit more about the refactoring in
> the Jdb.java and JdbTest.java?
> I understand that you moved some code from Jdb.java to JdbTest.java.
> But it is hard to track all of these move details.
> What was the main refactoring logic?
>
> Some comments on this update:
>
> http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/NullLocalVariable.java.frames.html
> <http://cr.openjdk.java.net/%7Eamenkov/sh2java/step3/webrev.01/test/jdk/com/sun/jdi/NullLocalVariable.java.frames.html>
>
> 30 * @library /test/lib
> 31 * @compile -g JdbExprTest.java
> 32 * @run main/othervm JdbExprTest
>
> Why the class 'JdbExprTest' is used above?
> Should it be the 'NullLocalVariable' instead?
>
> Thanks,
> Serguei
>
>
> On 8/30/18 16:27, Alex Menkov wrote:
>> Hi all,
>>
>> Please review a fix for
>> https://bugs.openjdk.java.net/browse/JDK-8210243
>> webrev:
>> http://cr.openjdk.java.net/~amenkov/sh2java/step3/webrev.01/
>> <http://cr.openjdk.java.net/%7Eamenkov/sh2java/step3/webrev.01/>
>>
>> The fix converts the following tests:
>> - test/jdk/com/sun/jdi/JdbArgTest.sh
>> - test/jdk/com/sun/jdi/JdbLockTest.sh
>> - test/jdk/com/sun/jdi/JdbMissStep.sh
>> - test/jdk/com/sun/jdi/JdbVarargsTest.sh
>> - test/jdk/com/sun/jdi/MixedSuspendTest.sh
>> - test/jdk/com/sun/jdi/NotAField.sh
>> - test/jdk/com/sun/jdi/NullLocalVariable.sh
>>
>> JdbArgTest requires to run only jdb (without debuggee), so Jdb
>> class was decoupled - it now contains only jdb logic, debuggee
>> logic was moved to JdbTest.
>>
>> --alex
>
>
>
> --
>
> Thanks,
> Jc
More information about the serviceability-dev
mailing list