8292250: Create test for co-located JDI MethodEntry, Step, and Breakpoint events #9840
Conversation
|
👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into |
|
Here is output from a test run that might be useful when reviewing the test: Got main thread: instance of java.lang.Thread(name='main', id=1) EventSet for test case #0: event set, policy:2, count:1 = {BreakpointEvent@CLEDebugee:75 in thread main} EventSet for test case #1: event set, policy:2, count:5 = {MethodEntryEvent@java.lang.ClassLoader:521 in thread main, MethodEntryEvent@jdk.internal.loader.ClassLoaders$AppClassLoader:180 in thread main, MethodEntryEvent@java.lang.System:497 in thread main, MethodEntryEvent@java.lang.System:205 in thread main, MethodExitEvent@java.lang.System:205 in thread main} EventSet for test case #1: event set, policy:2, count:1 = {StepEvent@CLEDebugee:76 in thread main} EventSet for test case #1: event set, policy:2, count:1 = {BreakpointEvent@CLEDebugee:78 in thread main} EventSet for test case #2: event set, policy:2, count:5 = {MethodEntryEvent@java.lang.ClassLoader:521 in thread main, MethodEntryEvent@jdk.internal.loader.ClassLoaders$AppClassLoader:180 in thread main, MethodEntryEvent@java.lang.System:497 in thread main, MethodEntryEvent@java.lang.System:205 in thread main, MethodExitEvent@java.lang.System:205 in thread main} EventSet for test case #2: event set, policy:2, count:1 = {StepEvent@t2:44 in thread main} EventSet for test case #2: event set, policy:2, count:1 = {BreakpointEvent@CLEDebugee:63 in thread main} EventSet for test case #3: event set, policy:2, count:3 = {MethodEntryEvent@CLEDebugee:87 in thread main, StepEvent@CLEDebugee:87 in thread main, BreakpointEvent@CLEDebugee:87 in thread main} EventSet for test case #3: event set, policy:2, count:1 = {BreakpointEvent@CLEDebugee:64 in thread main} EventSet for test case #4: event set, policy:2, count:2 = {MethodEntryEvent@CLEDebugee:95 in thread main, BreakpointEvent@CLEDebugee:95 in thread main} EventSet for test case #4: event set, policy:2, count:1 = {BreakpointEvent@CLEDebugee:65 in thread main} EventSet for test case #5: event set, policy:2, count:2 = {StepEvent@CLEDebugee:102 in thread main, BreakpointEvent@CLEDebugee:102 in thread main} EventSet for test case #5: event set, policy:2, count:1 = {BreakpointEvent@CLEDebugee:66 in thread main} EventSet for test case #6: event set, policy:2, count:2 = {MethodEntryEvent@CLEDebugee:109 in thread main, StepEvent@CLEDebugee:109 in thread main} EventSet for test case #6: event set, policy:2, count:2 = {VMDeathEvent, VMDeathEvent} EventSet for test case #6: event set, policy:0, count:1 = {VMDisconnectEvent} |
openjdk
bot
changed the title
|
@plummercj The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
Ping! |
test/jdk/com/sun/jdi/CLETest.java
Outdated
| String method; | ||
| String signature; | ||
| int lineNumber; |
test/jdk/com/sun/jdi/CLETest.java
Outdated
| List frames = thread.frames(); | ||
| Iterator iter = frames.iterator(); |
There was a problem hiding this comment.
List, Iterator and type cast is not needed2 lines below
| if (isColocated(set, true, true, true)) { | ||
| testcaseFailed = false; | ||
| } | ||
| } |
| if (isColocated(set, true, false, true)) { | ||
| testcaseFailed = false; | ||
| } | ||
| } |
| if (isColocated(set, false, true, true)) { | ||
| testcaseFailed = false; | ||
| } | ||
| } |
| if (isColocated(set, true, true, false)) { | ||
| testcaseFailed = false; | ||
| } | ||
| } |
| try { | ||
| // Setup all breakpoints | ||
| for (MethodBreakpointData bpData : breakpoints) { | ||
| Location loc = findMethodLocation(targetClass, bpData.method, |
There was a problem hiding this comment.
Specify breakpoints by line number in a method is better than just line number, but did you think about mark them with some tag and parse source file to get line numbers? (like in test/jdk/com/sun/jdi/lib/jdb/JdbTest.java, parseBreakpoints method)
There was a problem hiding this comment.
It looks like the tests in com/sun/jdi are split between those that extend JdbTest and those that extend TestScaffold. I'm not too sure of the rationale for using one over the other, but I ended up with TestScaffold because of the test I initially used as a template (BreakpointTest.java). parseBreakpoints() is only supported by JdbTest, so I would need to convert the test to extend it instead. I have no idea how disruptive this might be to the test.
I think the breakpoints I have are fairly safe from code movement and modifications. It would require edits within the methods with the breakpoints, all of which are very simple and well commented, and there really is no reason to ever edit them in the first place. Also there are 8 other tests that are sensitive to line numbers (search for THIS TEST IS LINE NUMBER SENSITIVE), and they are at much greater risk (adding or removing a line anywhere before the breakpoint will break the test).
There was a problem hiding this comment.
AFAIR JdbTest is a base class to test jdb functionality, I didn't mean to extend it. But breakpoint parsing code is quite simple, I thought about add similar method to TestScaffold instead of findMethodLocation.
As I wrote your way with findMethodLocation is much better than just line numbers (used in "THIS TEST IS LINE NUMBER SENSITIVE" tests), I'm okay with it.
There was a problem hiding this comment.
Ok. Thanks for the review!
|
@plummercj This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 162 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
I had to undo the last change that checked if the breakpoint was the expected one. There were two issues. The first was that the breakpoints are not in order, so it always threw an exception on the first breakpoint. However, the test still passed. That was because of a pre-existing bug. I declared |
plummercj
requested review from
kevinjwalls and
alexmenkov
and removed request for
kevinjwalls and
alexmenkov
|
Thanks! |
|
/integrate |
|
Going to push as commit d83faea.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@plummercj Pushed as commit d83faea. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
We currently have no tests for co-located MethodEntry, Step, and Breakpoint events. We should make sure they are being properly co-located as described in the JDI spec, and also do special test cases for JDK-8292217.
https://docs.oracle.com/en/java/javase/17/docs/api/jdk.jdi/com/sun/jdi/event/EventSet.html
And sorry in advance that the logic is a bit hard to follow in this test due to having multiple test cases, and dealing with the async nature of JDI testing. All I can say is that is used to be a lot worse before I did multiple passes to improve it.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9840/head:pull/9840$ git checkout pull/9840Update a local copy of the PR:
$ git checkout pull/9840$ git pull https://git.openjdk.org/jdk pull/9840/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9840View PR using the GUI difftool:
$ git pr show -t 9840Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9840.diff