Re: RFR 8193150: Create a jtreg version of the test from JDK-8187143

[Chris Plummer]

Hi Paru, Thanks for writing this test. It will make figuring out JDK-8187143 a lot easier. Overall the test looks good. My main concern is the lack of comments. It makes it hard for the reader to understand the flow of the test and to understand some of the less obvious actions being performed. That is especially true for this test, which is doing some really bizarre stuff. Some of this you cover in our RFR summary below, but that info really needs to be in the test itself, along with additional comments. For example, what does pauseAtDebugger() do? It looks to me like it sets a breakpoint on the _javascript_ debugger that has a class name that ends with ScriptRuntime and the method name is DEBUGGER(). But I only figured that out after staring at the code for a while, and recalling a conversation we had a few weeks ago. It's also not described why this is being done. Here's another example:  126         while (!vmDisconnected) {  127             try {  128                 Thread.sleep(100);  129             } catch (InterruptedException ee) {  130             }  131         } I seem to also recall us discussing the need for this, but can no longer recall the reason Another example is findScriptFrame(). What is the significance of the frame whose class name starts with jdk.nashorn.internal.scripts.Script$? I think I understand (it's the generated java method for the _javascript_ you setup in ScriptDebuggee.doit()), but I can only figure this out based on earlier conversations we had and your RFR comments below. I'd expect the uninformed reader to spend a long time coming the same conclusion. The following are just a few minor things I noticed: Copyright should only have 2018.   57         } catch (Exception npe) { Probably best to call it "ex" instead of "npe".   85         NashornPopFrameTest bbcT = new NashornPopFrameTest(args); It's unclear to me where the name "bbcT" comes from.  134         if (failReason != null) {  135             failure(failReason);  136         } You have two classes that declare "String failReason" which makes it a bit confusing to track which one the reader is dealing with. Also, the NashornPopFrameTest version is initialized to non-null, so doesn't that make the test always fail when the above code is executed? Is there a reason why ScriptDebuggee doesn't just put everything in main() and not have a doit() method? thanks, Chris On 2/7/18 12:25 PM, serguei.spit...@oracle.com wrote: Hi Paru, It looks good. Thank you a lot for taking care about this! Could we get at least one more review from the Serviceability team on this new test? Thanks, Serguei On 2/2/18 09:35, Paru Somashekar wrote: Hi, Please review the fix for JDK-8193150. The fix introduces a new jtreg test, NashornPopFrameTest. It is based on the original test from JDK-8187143 that was provided by the customer. Bug : https://bugs.openjdk.java.net/browse/JDK-8193150 Webrev : http://cr.openjdk.java.net/~psomashe/8193150/webrev/ Here is a brief description of what the test does :- * The debuggee,  creates and uses a Nashorn engine to evaluate a simple script. * The debugger  tries to set a breakpoint in Nashorn’s internal DEBUGGER method. * When the breakpoint is reached, it looks for stack frame whose method's declaring type name starts with (nashorn dynamically generated classes) ”jdk.nashorn.internal.scripts.Script$”. * It then pops stack frames using the ThreadReference.popFrames() call, up to and including the above stackframe. * The execution of the debuggee application is resumed after the needed frames have been popped. This test is included in the ProblemList as it fails under some circumstances (bug JDK-8187143). Is always passes with the -Xint flag however always fails with -Xcomp. It fails intermittently with the -Xmixed (default). thanks, Paru.