On Tue 18 Jun 2013 12:30:07 PM CEST, Jaroslav Bachorik wrote: > On Tue 18 Jun 2013 12:25:35 PM CEST, Daniel Fuchs wrote: >> Hi Jaroslav, >> >>> I've added the tests for the proper behaviour when the >>> "com.sun.jmx.mbeans.allowNonPublic" system property is set to true. >>> >>> http://cr.openjdk.java.net/~jbachorik/8010285/webrev.05/ >> >> Thanks for adding the test. I haven't looked at the source again, >> I trust nothing changed there. Some issues in the tests: >> >> MBeanFallbackTest.java: >> >> 57 if (failures > 0) { >> 58 System.exit(1); >> 59 } >> >> I think >> throw new Exception("TEST FAILURES: " + failures); >> would be more appropriate. > > I saw both of them used across the regtests and was not really sure > which is the recommended way of signalling the test failure ... > >> >> Also I see that you have an @run main line - I believe it >> should be @run main/othervm since the test sets some >> system properties. > > Currently all the testst in javax/management are run as main/othervm. > But it won't hurt to make the intention of running the test in a > separate JVM visually clear. > > The same applies to the rest of the new tests. > >> >> >> MXBeanFallbackTest.java: >> >> 30 * @author Eamonn McManus > > Yep :/
http://cr.openjdk.java.net/~jbachorik/8010285/webrev.06 Unified the way the test failure is reported, removed the forgotten debug code and copy/paste errors. I haven't changed "@run main" to "@run main/othervm" since the tests in javax/management are run in othervm anyway. Also, there is more JMX tests which eg. start they own MBean server and they don't declare "@run main/othervm". So I'd prefer staying consistent. -JB- > > -JB- > >> >> Is that a copy paste issue? >> >> 47 System.in.read(); >> >> Left over from debugging session - right ;-) ? >> >> I see that you have an @run main line there too - I >> believe it should be @run main/othervm since this >> test also sets some system properties. >> >>
