8289400: Improve com/sun/jdi/TestScaffold error reporting #10127

plummercj · 2022-09-01T19:09:25Z

com/sun/jdi tests report errors by calling TestScaffold.failure(msg), which prints the failure message and sets the testFailed flag. At some later point the failure is detected and an exception is thrown. The end result is the exception has just has a vanilla message that says something like "TestXXX failed", and the backtrace is not indicative of where the failure occurred. If you have tools that search logs looking for exceptions to determine the reason for the failure, you likely won't find any. Here's an example:

[2ms] run args: [SuspendAfterDeathTarg]
[514ms] FAILED: got Breakpoint event before ThreadDeath event.
java.lang.Exception: SuspendAfterDeath: failed
at SuspendAfterDeath.runTests(SuspendAfterDeath.java:110)
at TestScaffold.startTests(TestScaffold.java:432)
at SuspendAfterDeath.main(SuspendAfterDeath.java:47)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
at java.base/java.lang.reflect.Method.invoke(Method.java:578)
at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:1589)

So the reason for the failure is clear (the "FAILED" message), but its stack trace is missing and the reason is not included in the exception message that is printed much later on. This PR adds printing of the stack trace at the time of the failure.

[2ms] run args: [SuspendAfterDeathTarg]
[514ms] FAILED: got Breakpoint event before ThreadDeath event.
        at TestScaffold.failure(TestScaffold.java:455)
        at SuspendAfterDeath.breakpointReached(SuspendAfterDeath.java:64)
        at TestScaffold$EventHandler.notifyEvent(TestScaffold.java:194)
        at TestScaffold$EventHandler.run(TestScaffold.java:278)
        at java.base/java.lang.Thread.run(Thread.java:1589) 
java.lang.Exception: SuspendAfterDeath: failed
at SuspendAfterDeath.runTests(SuspendAfterDeath.java:110)
at TestScaffold.startTests(TestScaffold.java:432)
at SuspendAfterDeath.main(SuspendAfterDeath.java:47)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
at java.base/java.lang.reflect.Method.invoke(Method.java:578)
at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:1589)

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8289400: Improve com/sun/jdi/TestScaffold error reporting

Reviewers

Alex Menkov (@alexmenkov - Reviewer)
Leonid Mesnik (@lmesnik - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10127/head:pull/10127
$ git checkout pull/10127

Update a local copy of the PR:
$ git checkout pull/10127
$ git pull https://git.openjdk.org/jdk pull/10127/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10127

View PR using the GUI difftool:
$ git pr show -t 10127

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10127.diff

bridgekeeper · 2022-09-01T19:11:30Z

👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-09-01T19:15:04Z

@plummercj The following label will be automatically applied to this pull request:

serviceability

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.

mlbridge · 2022-09-01T19:17:42Z

Webrevs

01: Full - Incremental (49cbaf6a)
00: Full (86e42e53)

alexmenkov · 2022-09-01T20:15:04Z

test/jdk/com/sun/jdi/TestScaffold.java

@@ -453,6 +453,9 @@ protected void traceln(String str) {

    protected void failure(String str) {
        println(str);
+        StackTraceElement[] trace = Thread.currentThread().getStackTrace();
+        for (StackTraceElement traceElement : trace)


Please add curly braces around the cycle body

Ok. This was a copy-n-paste from Throwable.printStackTrace(), and I didn't noticed the omission when copying.

openjdk · 2022-09-01T21:48:26Z

@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:

8289400: Improve com/sun/jdi/TestScaffold error reporting

Reviewed-by: amenkov, lmesnik

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 20 new commits pushed to the master branch:

77e21c5: 8290529: C2: assert(BoolTest(btest).is_canonical()) failure
fcc0cf9: 8292375: Convert ProtectionDomainCacheTable to ResourceHashtable
6fc58b8: 8293207: Add assert to JVM_ReferenceRefersTo to clarify its API
ce06a3b: 8293023: Change CardTable::is_in_young signature
26cac08: 8293209: Parallel: Remove unused variables in PSParallelCompact::invoke
46523b8: 8293219: Microsoft toolchain selection picks 32-bit tools over 64-bit
bc5ffc8: 8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub
98ce45f: 8292981: Unify and restructure integer printing format specifiers
3ac91b0: 8293003: Review running time of Warn5 regression test
e0168a0: 8288012: AArch64: unnecessary macro expansion in stubGenerator_aarch64
... and 10 more: https://git.openjdk.org/jdk/compare/07616de00c3e1c305852fcc44df8dadafd0dbf3f...master

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 master branch, type /integrate in a new comment.

AlanBateman · 2022-09-02T05:40:38Z

test/jdk/com/sun/jdi/TestScaffold.java

+        StackTraceElement[] trace = Thread.currentThread().getStackTrace();
+        for (StackTraceElement traceElement : trace) {
+            System.err.println("\tat " + traceElement);
+        }


Thread.dumpStack() prints to System.errr.

But it does so by creating an Exception and calling printStackTrace() on it. I didn't want that because it includes the Exception itself in the output, not just the stack trace.

plummercj · 2022-09-02T16:05:42Z

Thanks Alex and Leonid!

/integrate

openjdk · 2022-09-02T16:08:27Z

Going to push as commit da99e3e.
Since your change was applied there have been 20 commits pushed to the master branch:

77e21c5: 8290529: C2: assert(BoolTest(btest).is_canonical()) failure
fcc0cf9: 8292375: Convert ProtectionDomainCacheTable to ResourceHashtable
6fc58b8: 8293207: Add assert to JVM_ReferenceRefersTo to clarify its API
ce06a3b: 8293023: Change CardTable::is_in_young signature
26cac08: 8293209: Parallel: Remove unused variables in PSParallelCompact::invoke
46523b8: 8293219: Microsoft toolchain selection picks 32-bit tools over 64-bit
bc5ffc8: 8293100: RISC-V: Need to save and restore callee-saved FloatRegisters in StubGenerator::generate_call_stub
98ce45f: 8292981: Unify and restructure integer printing format specifiers
3ac91b0: 8293003: Review running time of Warn5 regression test
e0168a0: 8288012: AArch64: unnecessary macro expansion in stubGenerator_aarch64
... and 10 more: https://git.openjdk.org/jdk/compare/07616de00c3e1c305852fcc44df8dadafd0dbf3f...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-02T16:08:49Z

@plummercj Pushed as commit da99e3e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Includes stack trace when there is a failure.

86e42e5

openjdk bot changed the title ~~8289400~~ 8289400: Improve com/sun/jdi/TestScaffold error reporting Sep 1, 2022

openjdk bot added the rfr Pull request is ready for review label Sep 1, 2022

openjdk bot added the serviceability serviceability-dev@openjdk.org label Sep 1, 2022

alexmenkov reviewed Sep 1, 2022

View reviewed changes

Add curly braces.

49cbaf6

alexmenkov approved these changes Sep 1, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 1, 2022

AlanBateman reviewed Sep 2, 2022

View reviewed changes

lmesnik approved these changes Sep 2, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 2, 2022

openjdk bot closed this Sep 2, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 2, 2022

plummercj deleted the 8289400_testscaffold_error_reporting branch October 11, 2022 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8289400: Improve com/sun/jdi/TestScaffold error reporting #10127

8289400: Improve com/sun/jdi/TestScaffold error reporting #10127

plummercj commented Sep 1, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 1, 2022

openjdk bot commented Sep 1, 2022

mlbridge bot commented Sep 1, 2022 •

edited

Loading

alexmenkov Sep 1, 2022

plummercj Sep 1, 2022

openjdk bot commented Sep 1, 2022 •

edited

Loading

AlanBateman Sep 2, 2022

plummercj Sep 2, 2022

plummercj commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

8289400: Improve com/sun/jdi/TestScaffold error reporting #10127

8289400: Improve com/sun/jdi/TestScaffold error reporting #10127

Conversation

plummercj commented Sep 1, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 1, 2022

openjdk bot commented Sep 1, 2022

mlbridge bot commented Sep 1, 2022 • edited Loading

Webrevs

alexmenkov Sep 1, 2022

Choose a reason for hiding this comment

plummercj Sep 1, 2022

Choose a reason for hiding this comment

openjdk bot commented Sep 1, 2022 • edited Loading

AlanBateman Sep 2, 2022

Choose a reason for hiding this comment

plummercj Sep 2, 2022

Choose a reason for hiding this comment

plummercj commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

openjdk bot commented Sep 2, 2022

plummercj commented Sep 1, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 1, 2022 •

edited

Loading

openjdk bot commented Sep 1, 2022 •

edited

Loading