Fwd: Re: Fwd: Re: RFR: 8009808 TEST-BUG : test case is using bash style tests. Default shell for jtreg is bourne. thus failure

[Bengt Rutisson]

Including the correct email list.

Bengt


-------- Original Message --------
Subject: 	Re: Fwd: Re: RFR: 8009808 TEST-BUG : test case is using bash 
style tests. Default shell for jtreg is bourne. thus failure
Date: 	Tue, 09 Apr 2013 14:40:09 +0200
From: 	Bengt Rutisson <bengt.rutisson at oracle.com>
To: 	Mikael Gerdin <mikael.gerdin at oracle.com>
CC: 	hotspot_gc_staff_ww_grp <hotspot_gc_staff_ww_grp at oracle.com>




Leonid,

I think the test looks good.

One minor nit: I think the the @summary comment could maybe be a bit 
different. It says:

@summary test new added flags for gc log rotation

In a while these flags won't be newly added. I think it is enough to say:

@summary test flags for gc log rotation

Thanks,
Bengt


On 4/9/13 1:45 PM, Mikael Gerdin wrote:
> Can one of our Reviewers please look at this, it's a good cleanup of a 
> messy test for gc log rotation.
>
> /m
>
> -------- Original Message --------
> Subject: Re: RFR: 8009808 TEST-BUG : test case is using bash style 
> tests. Default shell for jtreg is bourne. thus failure
> Date: Mon, 08 Apr 2013 18:23:10 +0400
> From: Leonid Mesnik <leonid.mesnik at oracle.com>
> CC: hotspot-gc-dev at openjdk.java.net <hotspot-gc-dev at openjdk.java.net>
>
> I still need approval from reviewers for this fix.
>
> Could anyone take a look on it?
>
> Leonid
> On 04/04/2013 03:57 PM, Mikael Gerdin wrote:
>> Leonid,
>>
>> On 2013-04-04 12:34, Leonid Mesnik wrote:
>>> Mikael
>>>
>>> Here is updated webrev:
>>> http://cr.openjdk.java.net/~mgerdin/lmesnik/log_rot_test/webrev.2
>>>
>>> Streams were redirected, toArray() was updated.
>>
>> Looks good to me.
>>
>> /Mikael
>>
>>>
>>> Leonid
>>> On 04/02/2013 01:57 PM, Mikael Gerdin wrote:
>>>> Leonid,
>>>>
>>>> On 2013-03-29 10:38, Leonid Mesnik wrote:
>>>>> Hi
>>>>>
>>>>> Here is new version of test. (pass jprt)
>>>>> http://cr.openjdk.java.net/~mgerdin/lmesnik/log_rot_test/webrev.1/
>>>>> <http://cr.openjdk.java.net/%7Emgerdin/lmesnik/log_rot_test/webrev.1/> 
>>>>>
>>>>
>>>> Overall it looks good. The test is nicely specified and in a
>>>> controlled environment.
>>>>
>>>> Can you please make the test save the stdout/stderr of the launched
>>>> process and print it out in case of failure?
>>>> If the launch fails with some exit code we won't know its cause. I
>>>> suggest something like:
>>>> pb.redirectErrorStream(true)
>>>> pb.redirectOutput("output.log")
>>>>
>>>> I also have one small comment in:
>>>> args.toArray(externalVMopts)
>>>>
>>>> Even though it may be correct to pass exernalVMopts to toArray(T[]) it
>>>> looks a bit confusing.
>>>> Can you please change it to either
>>>> "toArray(new String[0])" or
>>>> "toArray(new String[args.size()])"
>>>>
>>>> /Mikael
>>>>
>>>>>
>>>>> See my comment inline.
>>>>> On 03/26/2013 05:30 PM, Mikael Gerdin wrote:
>>>>>> Leonid,
>>>>>>
>>>>>> On 2013-03-22 08:06, Leonid Mesnik wrote:
>>>>>>> Could anyone please review this small test fix.
>>>>>>>
>>>>>>> Leonid
>>>>>>> On 03/20/2013 04:16 PM, Leonid Mesnik wrote:
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> Could you please review fix for  8009808 TEST-BUG : test case is
>>>>>>>> using
>>>>>>>> bash style tests. Default shell for jtreg is bourne. thus failure.
>>>>>>>>
>>>>>>>> I've completely rewritten test in java without major changes it
>>>>>>>> test
>>>>>>>> logic.
>>>>>>>> I remove CMS so test could be run when CMS is not supported. 
>>>>>>>> Also I
>>>>>>>> changed max memory to 128M to reduce memory load and increase
>>>>>>>> number
>>>>>>>> of GC log entries.
>>>>>>
>>>>>> Do you think it would be useful to run this test with different GCs?
>>>>>> In that case you can pick up the test.vm.opts and test.java.opts
>>>>>> system properties and add them to the java command line you launch.
>>>>> I've added test.java.opts properties. They are used during testing to
>>>>> set additional GC.
>>>>>>
>>>>>>>>
>>>>>>>> Here is the link:
>>>>>>>> http://cr.openjdk.java.net/~mgerdin/lmesnik/log_rot_test/webrev.0/
>>>>>>>> <http://cr.openjdk.java.net/%7Emgerdin/lmesnik/log_rot_test/webrev.0/> 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>> Are you sure about this:
>>>>>> static final File currentDirectory = new
>>>>>> File(System.getProperty("user.dir"));
>>>>>>
>>>>>> isn't user.dir the home directory? Current directory should be
>>>>>> just "."
>>>>>> Something like new File(".").getAbsoluteFile() should give you the
>>>>>> current work dir.
>>>>> System.getProperty("user.dir") is current directory. However I 
>>>>> changed
>>>>> it to "." to make it more evident.
>>>>>>
>>>>>> What is the relation between "numberOfFiles" and "minutesToRun"??
>>>>>> How do you know that running for 3 minutes will cause a log 
>>>>>> rotation?
>>>>> I've updated test to invoke fullGC and estimate lower bound of needed
>>>>> invocation. Now test check that exactly 3 logs are generated.
>>>>>
>>>>> Leonid
>>>>>>
>>>>>> I know that you've just adapted the existing test but it seems
>>>>>> strange
>>>>>> to me.
>>>>>>
>>>>>> /Mikael
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130409/3d473046/attachment.htm>