RFR JDK-8211292: [TEST] convert com/sun/jdi/DeferredStepTest.sh test
Alex Menkov
alexey.menkov at oracle.com
Fri Oct 5 23:53:11 UTC 2018
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