RFR JDK-8195703: BasicJDWPConnectionTest.java: 'App exited unexpectedly with 2'

[JC Beyler]

Hi Alex,

Looks great to me, thanks for cleaning it up!
Jc

On Thu, Oct 11, 2018 at 11:02 AM Alex Menkov <alexey.menkov at oracle.com>
wrote:

> Hi Jc,
>
> updated webrev:
> http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.02/
>
> > <
> http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html
> >
> >>> -> Why not make it javadoc like the other methods of the same file
> >>> (so @return instead of returns and a second * at the start of the
> >>     comment)?
>
> done.
>
>
> >>> b) Nit: Is there a reason we are complicating the code here:
> >>>
> >>>           try {
> >>>               LingeredApp a = LingeredApp.startApp(cmd);
> >>> +
> >>> + // startApp is expected to fail, but if not, terminate the app
> >>> + try {
> >>> + a.stopApp();
> >>> + } catch (IOException e) {
> >>> + // print and let the test fail
> >>> + System.err.println("LingeredApp.stopApp failed");
> >>> + e.printStackTrace();
> >>> + }
> >>>           } catch (IOException ex) {
> >>>               System.err.println(testName + ": caught expected
> >>     IOException");
> >>>               System.err.println(testName + " PASSED");
> >>>               return;
> >>>           }
> >>>
> >>> Why not just put it below? We could either put a outside the try
> >>     and then move that code out; or perhaps move it into a separate
> >>     method to let
> >>>
> >>> the reader concentrate on the test at hand and let the "stopping
> >>     of the app"  happen somewhere else?
>
> This is to improve code cleanup on error (the test expects that
> LingeredApp.startApp(cmd) fails, but if the test fails, the app remains
> running).
> I moved cleanup outside of the try (and removed try/catch around
> a.stopApp() - the test throws Exception anyway)
>
> --alex
>


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181011/00cc369f/attachment.html>