> On Jun 8, 2017, at 4:13 AM, Ujwal Vangapally <[email protected]> > wrote: > > Thanks for the Review Mandy, Igor, Harsha. > > below is the link for new webrev incorporating review comments. > > webrev: > http://cr.openjdk.java.net/~uvangapally/webrev/2017/8178508/webrev.01/ > <http://cr.openjdk.java.net/%7Euvangapally/webrev/2017/8178508/webrev.00/>
As you originally had, using @run main/othervm -Xmx3000M LargeHeapThresholdTest is much simpler. I think it’s better to say @requires to run only only 64-bit platforms. Can you move the multi-line @summary to the last tag. Mandy > changes in new webrev other than those mentioned in review comments: > > using -Xmx3000M option on unsupported machines(like window 32bit machine > containing just 2GB user space) creates unnecessary failures in previous > implementation, so used ProcessTools.executeTestJava() to do a sample run of > "java -Xmx3000M -version" and took decision depending on it's exit value > whether to use -Xmx3000M or not in actual execution . > > removed '@requires os.maxmemory >3G' as we really don't require more than > 3GB physical memory to verify this test. > > Suggestions: > > will it makes more sense to add > @requires (os.family != "windows") | (os.simpleArch != "i586") > as windows 32 bit just provides 2GB for user space. > > no problem with current test as it just prints > "Ability to use big heap thresholds has NOT been verified" > on win 32bit. > > Thanks, > Ujwal. > > > On 6/5/2017 10:29 PM, Mandy Chung wrote: >> >>> On Jun 5, 2017, at 9:48 AM, Mandy Chung <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> >>>>> >>>>> On 5/31/2017 10:32 AM, Ujwal Vangapally wrote: >>>>>> webrev : >>>>>> http://cr.openjdk.java.net/~uvangapally/webrev/2017/8178508/webrev.00/ >>>>>> <http://cr.openjdk.java.net/%7Euvangapally/webrev/2017/8178508/webrev.00/> >>> >>> The test should be in test/java/lang/management/MemoryPoolMXBean since >>> it’s a test for that API. I also suggest to rename the test to >>> LargeHeapThresholdTest (or something like that). >> >> 41 * @run main/othervm -Xmx3000M -ea MX2G >> 59 assert i.getUsageThreshold() == TWO_G : "Usage >> threshold for" >> The test should elimiate its dependency on -ea; otherwise the test may not >> fail when it runs standalone without -ea. You can eplace the >> assert with an if-statement to check the condition and throw a >> RuntimeException instead. >> >> Mandy >
