RFR 8196324: Update FilterMatch & FilterNoMatch to use TestScaffold instead of JDIScaffold

[Paru Somashekar]

Hi Serguei,

Thanks very much for the review and I have made all the changes 
incorporating your feedback.

New Webrev at :http://cr.openjdk.java.net/~psomashe/8196324/webrev.01/ 
<http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev.01/>

thanks,
Paru.

On 2/13/18, 2:02 PM, serguei.spitsyn at oracle.com wrote:
> Hi Paru,
>
> It looks good in general but I'd like to ask for some cleanup.
>
> I tell just about the first test (FilterMatch) but the other one needs 
> the same.
>
>   132     // This gets called if all filters match.
>   133     public void stepCompleted(StepEvent event) {
>   134         listenCalled = true;
>   135         System.out.println("listen: line#=" + event.location().lineNumber()
>   136                 + " event=" + event);
>   137         // disable the step and then run to completion
>   138         StepRequest str= (StepRequest)event.request();
>   139         str.disable();
>   140         eventSet.resume();
>   141     }
>   I'd suggest to replace "listen:" above with "Agent: StepEvent:" to make
>  it more consistent with similar tracing in the breakpointReached() below.
>
>   143     public void breakpointReached(BreakpointEvent event) {
>   144         ThreadReference thread = ((BreakpointEvent) event).thread();
>   145         String locStr = "" + ((BreakpointEvent) event).location();
>   146         System.out.println("Agent: BreakpointEvent #" +
>   147                 " at " + locStr + " in thread: " + thread);
>   148         // The bkpt was hit; disable it.
>   149         request.disable();
>   150     }
>   The casts to BreakpointEvent at lines 144, 145 are not needed
>   as the "event" is already of this type.
>   Unneeded sign '#' at the line 146.
>   Also, I'm suggesting to disable the "request" the same way
>   as it is done in the stepCompleted():
>            BreakpointRequest bpr= (BreakpointRequest)event.request();
>            bpr.disable();
>
>    It will help to make the "request" local in the runTests().
> Unneeded empty lines: 52, 54:
>    51     EventSet eventSet = null;
>    52
>    53     static boolean listenCalled;
>    54
>    55     BreakpointRequest request;
>
> The listenCalled needs to be renamed to stepCompleted.
> There is no big need for it to be static as it is similar to the eventSet.
> It is better to initialize it with false.
> Then this line can be removed:
>   114         listenCalled = false;
>
> I'd suggest to rename the variables "request" and "request1"
> to "bpRequest" and "stepRequest" and make the 'bpRequest' to be local
> in the runTests() as it is the only place where it has to be used.
>
>
> Thanks,
> Serguei
>
>
> On 2/12/18 11:25, Paru Somashekar wrote:
>> Hi,
>>
>> Please review fix for JDK-8196324
>>
>> The includes the following changes:
>>
>> * Use startToMain method of TestScaffold that automatically calls 
>> connect() and waitForVMStart()
>> * Since TestScaffold extends TargetAdapter, it now overrides event 
>> callbacks directly in the test class rather than implement an 
>> anonymous inner class.
>> * Breakpoint handling is done in breakpointReached callback instead 
>> of waitForRequestedEvent and subsequently removing it.
>>
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8196324
>> Review : http://cr.openjdk.java.net/~psomashe/8196324/webrev/ 
>> <http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev/> 
>> <http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev/>
>>
>> thanks,
>> Paru.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180213/12024157/attachment-0001.html>