RFR: 8244133: Refactor nsk/jdi tests to reduce code duplication in settingBreakpoint communication
Leonid Mesnik
leonid.mesnik at oracle.com
Mon May 4 18:43:53 UTC 2020
Thank you for review and feedback.
Leonid
On 5/1/20 7:45 PM, serguei.spitsyn at oracle.com wrote:
> Hi Leonid,
>
> I'd suggest to align parameters and remove two commented lines:
> 90 protected final BreakpointRequest settingBreakpoint(ThreadReference thread,
> 91 ReferenceType testedClass,
> 92 String methodName,
> 93 String bpLine,
> 94 String property)
> . . .
> 141 protected final void getEventSet() throws JDITestRuntimeException {
> 142 try {
> 143 // jdiHelper.log2(" eventSet = eventQueue.remove(waitTime);");
> 144 eventSet = eventQueue.remove(waitTime);
> 145 if (eventSet == null) {
> 146 throw new JDITestRuntimeException("** TIMEOUT while waiting for event **");
> 147 }
> 148 // jdiHelper.log2(" eventIterator = eventSet.eventIterator;");
>
> Otherwise, looks good.
> No need in new webrev if you fix it.
>
> Thanks,
> Serguei
>
>
> On 5/1/20 16:51, Leonid Mesnik wrote:
>> 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/
>>
>> Leonid
>>
>>> On May 1, 2020, at 4:05 PM, Chris Plummer <chris.plummer at oracle.com
>>> <mailto: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/
>>>>>> bug: 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/20200504/cf53e282/attachment-0001.htm>
More information about the serviceability-dev
mailing list