RFR (S): 8026043: add regression test for JDK-8000831

[Erik Helin]

Filipp,

this looks good, thanks for the test!

Erik

On 2013-11-26 10:48, Filipp Zhinkin wrote:
> Eric,
>
> thanks for looking at it!
>
> I've fixed all issues that you've listed below.
> Here is updated webrev:
> http://cr.openjdk.java.net/~kshefov/8026043/webrev.03/
>
> Thanks,
> Filipp.
>
> On 11/25/2013 06:00 PM, Erik Helin wrote:
>> Filipp,
>>
>> On 2013-11-06 17:11, Filipp Zhinkin wrote:
>>> Hi,
>>>
>>> may I have a review on it?
>>
>> Overall, the test looks fine. Only three small comments:
>> - Could you @run main/samevm? I think that you can actually remove @run
>>   tag, since AFAIK the default action for jtreg is:
>>        @run main/samevm NameOfTest
>> - The method ProcessTools.createVerboseJavaProcessBuilder is not
>>   available yet (it is in one of your other changes), can you use the
>>   existing ProcessTools.createJavaProcessBuilder for now?
>> - Could you make the garbage variable public?
>>
>> Thanks for writing new tests!
>> Erik
>>
>>> Thanks,
>>> Filipp.
>>>
>>> On 10/22/2013 12:40 PM, Filipp Zhinkin wrote:
>>>> Hi,
>>>>
>>>> I've done following changes:
>>>> - @build tag removed;
>>>> - 'garbage' array is now static field;
>>>> - added heap-sizing flags to avoid failures with options passed via
>>>> 'test.java.opts'.
>>>>
>>>> Updated webrev: http://cr.openjdk.java.net/~kshefov/8026043/webrev.02/
>>>>
>>>> Thanks,
>>>> Filipp.
>>>>
>>>> On 10/18/2013 07:33 PM, Filipp Zhinkin wrote:
>>>>> Hi Erik,
>>>>>
>>>>> thank you for watching at it!
>>>>>
>>>>> On 10/18/2013 06:59 PM, Erik Helin wrote:
>>>>>> Hi Filipp,
>>>>>>
>>>>>> thanks for adding new tests, really appreciate it!
>>>>>>
>>>>>> I have a couple of comments/questions about this test:
>>>>>> - Do you really need a @build tag?
>>>>> No, I don't.
>>>>>> - Could you please provide a comment above each regex with an example
>>>>>>    that shows what the regex matches?
>>>>> Yes, sure.
>>>>>> - This test will only run with the default GC (since no GC is
>>>>>>    specified). What do you think about additional run targets testing
>>>>>>    various GC combinations?
>>>>> The idea was to pass all VM options including different GC's via
>>>>> test.java.opts property.
>>>>> It makes test more flexible and allow us to test not only some fixed
>>>>> subset of GC combinations.
>>>>>> - Have you tried running the test with -Xcomp? The reason I'm
>>>>>> asking is
>>>>>>    because I'm afraid that the compiler might be able to realize
>>>>>> that the
>>>>>>    GarbageProducer main method isn't doing anything.
>>>>> Yes, I've tried and that object allocation was not optimized out,
>>>>> but I agree with you, "garbage" should be static.
>>>>>>
>>>>>>    One thing that usually solves this is having garbage as a public
>>>>>>    static variable in the class.
>>>>>> - Are you using GarbageProducer since you want to trigger young
>>>>>> GCs? If
>>>>>>    so, why only tests young GCs? Would the test works just as well
>>>>>> using
>>>>>>    System.gc() to tests full GCs?
>>>>> In fact, test running with MaxHeapSize=5m, so both minor and full GC
>>>>> happens.
>>>>> But there is another issue then  - I forgot to set up other heap
>>>>> sizing options,
>>>>> so test will fail when someone start if with, for example, -Xms10m.
>>>>>
>>>>> I'll fix that.
>>>>>
>>>>> Thanks,
>>>>> Filipp.
>>>>>> Thanks,
>>>>>> Erik
>>>>>>
>>>>>> On 2013-10-14, Filipp Zhinkin wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I've refactored options parsing, here is updated webrev:
>>>>>>> http://cr.openjdk.java.net/~kshefov/8026043/webrev.01/
>>>>>>>
>>>>>>> Note that I'm using ProcessTools.createVerboseJavaProcessBuilder
>>>>>>> from 8026047
>>>>>>> to make sure that VM output will not be suppressed or redirected to
>>>>>>> file.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Filipp.
>>>>>>>
>>>>>>> On 10/11/2013 08:22 PM, Tao Mao wrote:
>>>>>>>> Ok, thank you for heads-up. Then, I'll await your webrev update.
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>> Tao
>>>>>>>>
>>>>>>>> On 10/11/13 9:11 AM, Filipp Zhinkin wrote:
>>>>>>>>> Hi Tao,
>>>>>>>>>
>>>>>>>>> Sorry for typo. You're right, the correct one is
>>>>>>>>> http://cr.openjdk.java.net/~kshefov/8026043/webrev.00/.
>>>>>>>>>
>>>>>>>>> And I'll refactor it in the same way as fix for 8026047.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Filipp.
>>>>>>>>>
>>>>>>>>> On 10/11/2013 08:11 PM, Tao Mao wrote:
>>>>>>>>>> Hi Filipp,
>>>>>>>>>>
>>>>>>>>>> FYI, it has a wrong webrev. I guess it should be
>>>>>>>>>> http://cr.openjdk.java.net/~kshefov/8026043/webrev.00/
>>>>>>>>>>
>>>>>>>>>> Thanks.
>>>>>>>>>> Tao
>>>>>>>>>>
>>>>>>>>>> On 10/8/13 8:34 AM, Filipp Zhinkin wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> I would like to get couple review on test, that cover issue
>>>>>>>>>>> fixed by 8000831 - Heap verification output
>>>>>>>>>>> incorrect/incomplete.
>>>>>>>>>>>
>>>>>>>>>>> Test verifies that options VerifyBeforeGC and
>>>>>>>>>>> VerifyAfterGCemit appropriate output if they are turned on
>>>>>>>>>>> and never
>>>>>>>>>>> emit corrupted output as it was before 8000831 fixed.
>>>>>>>>>>>
>>>>>>>>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8026043
>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~kshefov/8026047/webrev.00/
>>>>>>>>>>> Testing: manual on local host, automated on various platforms.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Filipp.
>>>>>
>>>>
>>>
>