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?
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.
thanks,
Chris
On 4/29/20 4:45 PM, Leonid Mesnik wrote:
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?
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.
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.
