RFR JDK-8211292: [TEST] convert com/sun/jdi/DeferredStepTest.sh test
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Sat Oct 6 00:00:21 UTC 2018
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
>>
More information about the serviceability-dev
mailing list