RFR JDK-8211292: [TEST] convert com/sun/jdi/DeferredStepTest.sh test
JC Beyler
jcbeyler at google.com
Sat Oct 6 01:19:15 UTC 2018
Hi Alex,
Thanks also for the update; looks great to me!
Jc
On Fri, Oct 5, 2018 at 5:00 PM <serguei.spitsyn at oracle.com> wrote:
> Hi Alex,
>
> It looks good to me.
> Thank you for the update.
>
> Thanks,
> Serguei
>
> On 10/5/18 4:53 PM, Alex Menkov wrote:
> > ok, this is updated webrev:
> >
> http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev.03/
> >
> > --alex
> >
> > On 10/05/2018 12:37, serguei.spitsyn at oracle.com wrote:
> >> In general, like the suggestion from Jc with the correction for
> >> lastLine to be a local.
> >> But leave it up to Alex to decide what is better as changes would
> >> require another round of testing.
> >>
> >> Thanks,
> >> Serguei
> >>
> >> On 10/5/18 12:10 PM, JC Beyler wrote:
> >>> You're right for the single threaded part; I misread that part and
> >>> thought it would be multi-threaded as well. And fair enough for the
> >>> keeping it then as a do..while(false); it just took me a while to
> >>> figure out what was being done. You could put the data.lastLine in a
> >>> local variable and update it at the start of the method (only using
> >>> the local version for the rest of the method); then everything would
> >>> be in there. But, I'll still say it is a more a question of style :)
> >>>
> >>> LGTM,
> >>> Jc
> >>>
> >>> On Fri, Oct 5, 2018 at 12:01 PM Alex Menkov
> >>> <alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>> wrote:
> >>>
> >>> Hi Jc,
> >>>
> >>>
> >>> On 10/05/2018 10:34, JC Beyler wrote:
> >>> > Hi Alex,
> >>> >
> >>> > One question and a comment on this:
> >>> > - I thought HashMap was not thread safe so I think you need to
> >>> > synchronize the access to the map threadData
> >>>
> >>> The map is accessed from a single thread (main test thread which
> >>> sends
> >>> jdb commands and handles jdb replies), so synchronization is not
> >>> required.
> >>>
> >>> >
> >>> > - I think your test code could be simplified if you moved it
> >>> into a
> >>> > helper method (not tested but just for example):
> >>>
> >>> I suppose you don't like do/break/while(false)?
> >>> To me it's quite standard method to avoid multi-level if/then/else.
> >>> In your suggestion I don't like that processNewData() method
> >>> handles
> >>> minLine/maxLine, but doesn't handle lastLine (i.e. it doesn't do
> >>> all
> >>> processing). But if "data.lastLine = lineNum" is moved into the
> >>> method,
> >>> we need something like do/break/while(false) in the method.
> >>>
> >>> --alex
> >>>
> >>> >
> >>> > + private void next() {
> >>> > + List<String> reply = jdb.command(JdbCommand.next());
> >>> > + /*
> >>> > + * Each "next" produces something like ("Breakpoint
> >>> hit" line
> >>> > only if the line has BP)
> >>> > + * Step completed:
> >>> > + * Breakpoint hit: "thread=jj2",
> >>> > DeferredStepTestTarg$jj2.run(), line=74 bci=12
> >>> > + * 74 ++count2;
> >>> > // @2 breakpoint
> >>> > + * <empty line>
> >>> > + * jj2[1]
> >>> > + */
> >>> > + // detect thread from the last line
> >>> > + String lastLine = reply.get(reply.size() - 1);
> >>> > + String threadName = parse(threadRegexp, lastLine);
> >>> > + String wholeReply =
> >>> > reply.stream().collect(Collectors.joining(Utils.NEW_LINE));
> >>> > + int lineNum = Integer.parseInt(parse(lineRegexp,
> >>> wholeReply));
> >>> > +
> >>> > + System.out.println("got: thread=" + threadName + ",
> >>> line=" +
> >>> > lineNum);
> >>> > +
> >>> > + ThreadData data = threadData.get(threadName);
> >>> > + if (data == null) {
> >>> > + data = new ThreadData();
> >>> > + threadData.put(threadName, data);
> >>> > + }
> >>> > + processNewData(data, threadName, lineNum);
> >>> > + data.lastLine = lineNum;
> >>> > + }
> >>> > +
> >>> > + private void processNewData(ThreadData data, String
> >>> threadName, int
> >>> > lineNum) {
> >>> > + if (data.lastLine < 0) {
> >>> > + // the 1st stop in the thread
> >>> > + return;
> >>> > + }
> >>> > +
> >>> > + if (lineNum == data.lastLine + 1) {
> >>> > + // expected.
> >>> > + return;
> >>> > + }
> >>> > +
> >>> > + if (lineNum < data.lastLine) {
> >>> > + // looks like step to the beginning of the cycle
> >>> > + if (data.minLine > 0) {
> >>> > + // minLine and maxLine are not set - verify
> >>> > + Asserts.assertEquals(lineNum, data.minLine,
> >>> threadName +
> >>> > " - minLine");
> >>> > + Asserts.assertEquals(data.lastLine, data.maxLine,
> >>> > threadName + " - maxLine");
> >>> > + } else {
> >>> > + // set minLine/maxLine
> >>> > + data.minLine = lineNum;
> >>> > + data.maxLine = data.lastLine;
> >>> > + }
> >>> > + return;
> >>> > + }
> >>> > +
> >>> > + throw new RuntimeException(threadName + " (line " +
> >>> lineNum +
> >>> > ") - unexpected."
> >>> > + + " lastLine=" + data.lastLine + ", minLine=" +
> >>> > data.minLine + ", maxLine=" + data.maxLine);
> >>> > + }
> >>> >
> >>> > Thanks,
> >>> > Jc
> >>> >
> >>> >
> >>> >
> >>> > On Thu, Oct 4, 2018 at 6:31 PM <serguei.spitsyn at oracle.com
> >>> <mailto:serguei.spitsyn at oracle.com>
> >>> > <mailto:serguei.spitsyn at oracle.com
> >>> <mailto:serguei.spitsyn at oracle.com>>> wrote:
> >>> >
> >>> > Hi Alex,
> >>> >
> >>> > It looks good to me.
> >>> > Could you, please, also remove the line? :
> >>> >
> >>> > 156 //
> >>> >
> >>> > No need in new webrev.
> >>> >
> >>> > Thanks,
> >>> > Serguei
> >>> >
> >>> >
> >>> > On 10/4/18 4:11 PM, Alex Menkov wrote:
> >>> > > Hi Serguei,
> >>> > >
> >>> > > Updated webrev:
> >>> > >
> >>> >
> >>>
> http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev.02/
> >>> <
> http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev.02/
> >
> >>> >
> >>> <
> http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev.02/
> >
> >>> > >
> >>> > > Fixed all issues except
> >>> > > 140 // the 1st stop in the thread
> >>> > > 141 break;
> >>> > > In this case the comment is an explanation why we reach
> >>> the
> >>> > block, not
> >>> > > an explanation for the "break" statement.
> >>> > >
> >>> > > --alex
> >>> > >
> >>> > > On 10/04/2018 13:56, serguei.spitsyn at oracle.com
> >>> <mailto:serguei.spitsyn at oracle.com>
> >>> > <mailto:serguei.spitsyn at oracle.com
> >>> <mailto:serguei.spitsyn at oracle.com>> wrote:
> >>> > >> Hi Alex,
> >>> > >>
> >>> > >> Several minor suggestions.
> >>> > >>
> >>> > >> 77 new Thread(aRP, "jj1").start();
> >>> > >> 78 new Thread(asRP, "jj2").start(); What mean aRP and
> >>> asRP? In
> >>> > fact,
> >>> > >> it is confusing. Can they be renamed to something like
> >>> obj1 and
> >>> > obj2.
> >>> > >>
> >>> > >> 79 // new Thread(aRP, "jj3").start();
> >>> > >> 80 // new Thread(asRP, "jj4").start();
> >>> > >>
> >>> > >> These lines can be removed.
> >>> > >>
> >>> > >> 94 // line of the last stop
> >>> > >> 95 int lastLine = -1;
> >>> > >> 96 // min line (-1 means "not known yet")
> >>> > >> 97 int minLine = -1;
> >>> > >> 98 // max line (-1 means "not known yet")
> >>> > >> 99 int maxLine = -1; ... 140 // the 1st stop in the
> >>> thread
> >>> > >> 141 break;
> >>> > >>
> >>> > >> I'd suggest the refactor above as below:
> >>> > >>
> >>> > >> int lastLine = -1; // line of the last stop
> >>> > >> int minLine = -1; // min line (-1 means "not known yet")
> >>> > >> int maxLine = -1;// max line (-1 means "not known yet")
> >>> > >> ...
> >>> > >> break; // the 1st stop in the thread
> >>> > >>
> >>> > >> 116 private void next() {
> >>> > >> 117 List<String> reply = jdb.command(JdbCommand.next());
> >>> > >> 118 /* each "next" produces something like ("Breakpoint
> >>> hit" line
> >>> > >> only if the line has BP)
> >>> > >> 119 Step completed:
> >>> > >> 120 Breakpoint hit: "thread=jj2",
> >>> DeferredStepTestTarg$jj2.run(),
> >>> > >> line=74 bci=12
> >>> > >> 121 74 ++count2; // @2 breakpoint
> >>> > >> 122 <empty line>
> >>> > >> 123 jj2[1]
> >>> > >> 124 */ It would better to have it in this style: 118 /*
> >>> * Each
> >>> > "next"
> >>> > >> produces something like ("Breakpoint hit" line only if
> >>> the line
> >>> > has BP).
> >>> > >> 119 * Step completed:
> >>> > >> 120 * Breakpoint hit: "thread=jj2",
> >>> DeferredStepTestTarg$jj2.run(),
> >>> > >> line=74 bci=12
> >>> > >> 121 * 74 ++count2; // @2 breakpoint
> >>> > >> 122 * <empty line>
> >>> > >> 123 * jj2[1]
> >>> > >> 124 */
> >>> > >>
> >>> > >>
> >>> > >> Otherwise, it looks Okay to me.
> >>> > >>
> >>> > >>
> >>> > >> Thanks,
> >>> > >> Serguei
> >>> > >>
> >>> > >> On 10/3/18 5:49 PM, Alex Menkov wrote:
> >>> > >>> Hi all,
> >>> > >>>
> >>> > >>> please review a fix for
> >>> > >>> https://bugs.openjdk.java.net/browse/JDK-8211292
> >>> > >>> webrev:
> >>> > >>>
> >>> >
> >>> http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev/
> >>> <
> http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev/>
> >>> >
> >>> <
> http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev/>
> >>> > >>>
> >>> > >>> The fix converts manual shell test to automatic java
> >>> (as Java
> >>> > allows
> >>> > >>> to parse jdb replies much easier).
> >>> > >>> This is the last sub-task for the "shell to java
> >>> conversion" task,
> >>> > >>> so the fix also removes shared shell scripts.
> >>> > >>>
> >>> > >>> --alex
> >>> > >>
> >>> >
> >>> >
> >>> >
> >>> > --
> >>> >
> >>> > Thanks,
> >>> > Jc
> >>>
> >>>
> >>>
> >>> --
> >>>
> >>> Thanks,
> >>> Jc
> >>
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181005/600c2e19/attachment-0001.html>
More information about the serviceability-dev
mailing list