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

[Leonid Mesnik]

Yes, the protected should be enough. I've made all non constant fields and methods protected and remove comment.

The updated webrev (JDIBase only)
http://cr.openjdk.java.net/~lmesnik/8244133/webrev.01/ <http://cr.openjdk.java.net/~lmesnik/8244133/webrev.01/>

Leonid

> On May 1, 2020, at 4:05 PM, Chris Plummer <chris.plummer at oracle.com> wrote:
> 
> On 5/1/20 3:42 PM, Leonid Mesnik wrote:
>> Hi
>> 
>>> On May 1, 2020, at 1:12 PM, Chris Plummer <chris.plummer at oracle.com <mailto: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.
> Ok.
>>> 
>>> 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.
> But I'm just referring to the one copy in JDIBase.java. It seems getting the format and commenting of this entirely new class is something to do now, not at some later point when you cleanup other tests.
> 
> Also, the fields were not public before being moved to JDIBase, they were package private. So unless there is a reason for them to be public, I'm not sure why you would not be more restrictive as long as it does not impact the tests. Do the tests need for the fields to be public or else would need to be modified?
> 
> thanks,
> 
> Chris
>> 
>> 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/5b63b8f0/attachment-0001.htm>