RFR(S) 8211336 [TESTBUG] appcds tests with incorrect usage of -XX:+UseStringDeduplication

[David Holmes]

On 30/10/2018 12:23 PM, Ioi Lam wrote:
> On 10/29/18 6:23 PM, David Holmes wrote:
>> Hi Ioi,
>>
>> On 30/10/2018 7:32 AM, Ioi Lam wrote:
>>> http://cr.openjdk.java.net/~iklam/jdk12/8211336-cds-test-fix-for-string-dedup.v01/ 
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8211336
>>>
>>> Please review this small test fix:
>>>
>>> These are "shell" tests that need to pass certain options (such as
>>> -XX:+UseStringDeduplication) to child JVM processes that are launched 
>>> via
>>> jdk.test.lib.process.ProcessTools. The tests were using this:
>>>
>>>     * @run main/othervm -XX:+UseStringDeduplication ExerciseGC
>>>
>>> However, options passed this way are not passed by ProcessTools. The 
>>> fix is
>>> to change to the following
>>>
>>>     * @run driver ExerciseGC -XX:+UseStringDeduplication
>>>
>>> and let the main test program explicitly pass args[] (indirectly)
>>> to ProcessTools.
>>
>> That looks really weird to me. I would not expect to pass things this 
>> way. If the test is only intending to launch other VMs then the logic 
>> to add -XX:+UseStringDeduplication should be an explicit part of the 
>> test, directly in the main test logic, not incidentally passed through 
>> via the mechanism you suggest.
>>
> 
> 
> The 11 tests each originally had only one @run flag, like
> 
>         * @run driver ExerciseGC
> 
> Later, it was suggested (JDK-8185531) to add more flag combinations, 
> such as -XX:+UseStringDeduplication and -XX:-CompactStrings, to increase 
> test coverage. It's pretty common to do this by adding a new @run tag 
> with different flags, as done in JDK-8185531. The only problem is 
> JDK-8185531 doesn't work because jtreg doesn't pass the additional flags 
> to the child processes. Hence the fix in this RFR.

Understood. But tests that launch other VMs to do the real testing 
generally manage the flag coverage themselves. The way you have done it 
here just seems really bizarre and non-obvious i.e. you have to realize 
that the intended flags get passed via the args to main and there can't 
be any real args for the test. And its tricky to run outside jtreg as 
you have to manually.

I see two AOT tests that employ a similar scheme, but I don't like it.

> This way it's easy to add other VM flag combinations to the tests. You 
> don't need to modify the test sources identically 11 times just to add a 
> new flag combination to these 11 tests.

Umm how do you not have to modify the test source identically 11 times 
when you have to add the new arg to the "@run driver ..." command, or 
add a new @run driver line??

Thanks,
David

> Thanks
> - Ioi
> 
> 
> 
> 
> 
>> David
>> -----
>>
>>> (I also cleaned up some unnecessary @module tags)
>>>
>>> Test in progress with hs-tier1/2.
>>>
>>> Thanks
>>> - Ioi
>