RFR: 8244133: Refactor nsk/jdi tests to reduce code duplication in settingBreakpoint communication

[Leonid Mesnik]

Hi

> On May 1, 2020, at 1:12 PM, Chris Plummer <chris.plummer at oracle.com> wrote:
> 
> Hi Leonid,
> 
> Overall looks good.
> 
> How did you verify that all settingBreakpoint() implementations were the same? How did you find that some of the breakpointForCommunication() implementations were different?


Unfortunately I didn't find a good way to check that all these method are exactly the same. I just go through diff without lines common for all methods and verify that nothing unexpected was removed.
Using this I found the different  breakpointForCommunication() and settingBreakpoint() implementation. I found 2  variants of   settingBreakpoint(). The one which set breakpointLocation (or location) and one which don't set. 


Additionally I grepped new files to check that lines like ' static final int PASSED' and other known patterns are removed.
> 
> A few minor things in JDIBase:
> 
>  108             throws JDITestRuntimeException {
> 
> I think it would look better with the { on a new line.
> 
>   80     // used by tests with breakpoint communication
>   81     public EventRequestManager eventRManager;
>   82     public EventQueue eventQueue;
>   83     public EventSet eventSet;
>   84     public EventIterator eventIterator;
>   85 
>   86     // additional fields initialized during breakpoint communication
>   87     public Location breakpLocation = null;
>   88     public BreakpointEvent bpEvent;
> 
> Do these fields need to be public and not just protected? Public fields in java are not the norm.
> 
>   91     /*
>   92      * private BreakpointRequest settingBreakpoint(ThreadReference, ReferenceType,
>   93      *                                             String, String, String)
>   94      *
>   95      * It sets up a breakpoint at given line number within a given method in a given class
>   96      * for a given thread.
>   97      *
>   98      * Return value: BreakpointRequest object  in case of success
>   99      *
>  100      * JDITestRuntimeException  in case of an Exception thrown within the method
>  101      */
>  123             int n =
>  124                     ((IntegerValue) testedClass.getValue(testedClass.fieldByName(bpLine))).value();
> 
> I'm not sure why the comment includes the prototype, but in any case it is out of date. I suggest just removing it.
> 
> Also, there are double-spaces in lines 98 and 100.
> 
I could make fields protected and remove comments. But I want to review base class and  might be make more refactoring.  Also there are a couple of tests with strange formatting, so I am going to add them separately.
I just don't want to put all this in single change. So diff contains mostly removal of same lines and it is easier to check it.

Leonid
> thanks,
> 
> Chris
> 
> On 4/29/20 4:45 PM, Leonid Mesnik wrote:
>> Hi
>> 
>> Could you please review following fix which remove code duplication by moving methods BreakpointRequest settingBreakpoint, getEventSet, breakpointForCommunication and log1,2,3 in base class.
>> 
>> The fix is huge but pretty straight forward, I tried to keep changes as small as possible so most tests just changed to extends JDIBase and corresponding methods and fields were deleted. 
>> 
>> Some tests used slightly different 'breakpointForCommunication()' implementation so they override it now. 
>> 
>> Some tests contain change like this:
>> -                     eventRequest1 = eventRManager.createBreakpointRequest(location);
>> +                     eventRequest1 = eventRManager.createBreakpointRequest(breakpLocation);
>> it is because they had 'location' and not 'breakpLocation' which is set by settingBreakpoint(...). So just merged location and breakpLocation.
>> 
>> All tests passed, so the main goal is to ensure that changes don't introduce false positive results ignoring exit codes and statuses.
>> 
>> I quickly verified that updated files don't contain declarations of variable like PASSED, testExitCode and other.
>> 
>> The JDIBase and all tests could be improved, also there are still a lot of tests which could use it as a base as well as other base classes for idi debuggers. However I  want to keep this fix as simple as possible.
>> 
>> werbev: http://cr.openjdk.java.net/~lmesnik/8244133/webrev.00/ <http://cr.openjdk.java.net/~lmesnik/8244133/webrev.00/>
>> bug: https://bugs.openjdk.java.net/browse/JDK-8244133 <https://bugs.openjdk.java.net/browse/JDK-8244133>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200501/697f3db8/attachment-0001.htm>