Code Review: Better handling of abnormal exits from JUnitShell remoteweb test

[Eric Ayers]

To recap from another thread:

- When running a JUnit test using -remoteweb, there are error conditions where RunStyleRemoteWeb calls System.exit(1)
- This is not a problem when you run a test standalone, but when running as a junit ant task, these errors get reported somewhat cryptically as:

testcase: testGetBuildInfo took 0.002 sec
	Caused an ERROR
Forked Java VM exited abnormally. Please note the time in the report does not reflect the time until the VM exit.

junit.framework.AssertionFailedError: Forked Java VM exited abnormally. Please note the time in the report does not reflect the time until the VM exit.
	at java.lang.Thread.run(Thread.java:595)

Scott suggested we replace the System.exit(1) with throwing an exception.  This change does that, plus adds a shutdown hook that will send a message to BrowserManagerServer to kill any browsers that are still running at the end of the test.

M      user/src/com/google/gwt/junit/RunStyleRemoteWeb.java


--
Eric Z. Ayers - GWT Team - Atlanta, GA USA
http://code.google.com/webtoolkit/

Don't miss Google's biggest developer event of the year
http://code.google.com/events/io

[Scott Blum]

- Needs sort (member types go at top)

- ShutdownCb.run() must be synchronized
- I wouldn't try to log an error on shutdown since TreeLogger is not generally thread-safe.  I would just ignore the error or possibly System.err.println it.

- The failure mode user experience as it sits right now is pretty bad, because we end up spamming the console for every test in the whole suite.  To get the right user experience, we need some small changes to JUnitShell (attached)

Otherwise LGTM.

[Eric Ayers]

On Wed, Apr 30, 2008 at 12:29 PM, Scott Blum <sco...@google.com> wrote:

- Needs sort (member types go at top)

Done.
 

- ShutdownCb.run() must be synchronized

Done. (At first I wasn't sure this was necessary, but I read that shutdown hooks run concurrently.)

- I wouldn't try to log an error on shutdown since TreeLogger is not generally thread-safe.  I would just ignore the error or possibly System.err.println it.

Done (printed the error for now)
 

- The failure mode user experience as it sits right now is pretty bad, because we end up spamming the console for every test in the whole suite.  To get the right user experience, we need some small changes to JUnitShell (attached)

LG. 
  

Otherwise LGTM.

Committed both patches as r2616