RFR(S) : 8243432 : use reproducible random in :vmTestbase_vm_defmeth

[David Holmes]

On 1/05/2020 2:53 pm, Igor Ignatyev wrote:
> 
> 
>> On Apr 30, 2020, at 9:26 PM, David Holmes <david.holmes at oracle.com 
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>> On 1/05/2020 1:53 pm, Igor Ignatyev wrote:
>>>> On Apr 30, 2020, at 8:44 PM, David Holmes <david.holmes at oracle.com 
>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>
>>>> On 1/05/2020 1:38 pm, Igor Ignatyev wrote:
>>>>> Hi David,
>>>>> one reason is to remove redundant code, vmTestbase have several 
>>>>> classes which provide means to specify seed, and it might have made 
>>>>> sense in old test-harness where each testlist/testgroup might have 
>>>>> been seen as a separate product, it's not really a case anymore.
>>>>> another is to unify how we specify a seed, so it would be easier to 
>>>>> have a seed value "stable" for a build/CI run, which will help to 
>>>>> compare results of "randomness" tests from different runs w/o their 
>>>>> re-execution, right now if you have such test failing let's say on 
>>>>> windows-x64 and linux-x64, you can't tell if it's platform specific 
>>>>> or not until you rerun on one of the platforms w/ the seed from 
>>>>> another. and from that perspective, 8243432 and other subtasks of 
>>>>> 8241623 are just preparation step, yet w/ an extra value of their own.
>>>>
>>>> So can't we program the obtaining of the seed into the existing 
>>>> framework?
>>> that would mean we will still have multiple ways to set the seed and 
>>> we will need to update them all if some changes in the way we set up 
>>> seed is needed.
>>
>> I don't follow. Whatever means you intend to use to get the "one 
>> blessed seed" into the testing framework, you make available to the 
>> code that initializes the seed in the OptionsSupport code. I'd rather 
>> see that code hacked to hook into the test library than the current hack.
> 
> hm... I guess it's a matter of perspective but I look at the current 
> patch as rather a cleanup and a step on the way to further convergence 
> of vmTestbase w/ the rest of jtreg testbase than a hack. in any case, 
> now I think that I understand that you are really suggesting instead of 
> remove 'seed' from StressTest change StressTest:: startWorkers to use 
> Utils.SEED, right?
> 
> http://cr.openjdk.java.net/~iignatyev/8243432/webrev.01
> 
> sure I can do that, although I don't see how it's better than 00.

That's actually a lot simpler than what I had envisioned :)

It is better IMO opinion because you can still run the test standalone 
passing in any of the supported options.

Thanks,
David
-----

> 
>>
>>>> What if you just switch the default seed from 0 (random) to the 
>>>> mystical 42?
>>> that would make these test fully/more deterministic and can 
>>> potentially impair test coverage.
>>
>> Theoretically but it really will depend on exactly how these random 
>> numbers are being used in any given test. A good RNG should give you a 
>> uniform distribution across its range regardless of the initial seed 
>> value. And tests using randomness for coverage should have some means 
>> of ensuring they actually do get adequate cover.
>>
>> As I questioned in other email it is hard to know what defines a test 
>> that requires a truly random (on each run) seed, versus a fixed 
>> initial seed.
> true, but since this exercise isn't about making such decisions but 
> rather about documenting already done decisions (and some clean up), I 
> really don't think we should be spending time trying to guess what the 
> intent of tests' authors was. we can of course file a ticket to return 
> to this, but something tells me it would never happen ;)
> 
> -- Igor
>>
>> David
>>
>>> -- Igor
>>>>
>>>> David
>>>>
>>>>> Thanks,
>>>>> -- Igor
>>>>>
>>>>>> On Apr 30, 2020, at 7:48 PM, David Holmes <david.holmes at oracle.com 
>>>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>>>
>>>>>> Hi Igor,
>>>>>>
>>>>>> On 1/05/2020 3:23 am, Igor Ignatyev wrote:
>>>>>>> http://cr.openjdk.java.net/~iignatyev/8243432/webrev.00
>>>>>>>> 22 lines changed: 3 ins; 12 del; 7 mod;
>>>>>>> Hi all,
>>>>>>> could you please review this small patch?
>>>>>>> from JBS:
>>>>>>>> this subtask is to use j.t.l.Utils.getRandomInstance() as a 
>>>>>>>> random number generator, where applicable, in : 
>>>>>>>> vmTestbase_vm_defmeth test group and marking the tests which 
>>>>>>>> make use of "randomness" with a proper k/w.
>>>>>>
>>>>>> Sorry I don't like this change at all. The code already supports 
>>>>>> using a deterministic seed so why not just set it?
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> testing: : vmTestbase_vm_defmeth test group
>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8243432
>>>>>>> webrevs:
>>>>>>> - code changes: 
>>>>>>> http://cr.openjdk.java.net/~iignatyev//8243432/webrev.00.code
>>>>>>>> 16 lines changed: 1 ins; 12 del; 3 mod;
>>>>>>> - adding k/w: 
>>>>>>> http://cr.openjdk.java.net/~iignatyev//8243432/webrev.00.kw
>>>>>>>> 2 lines changed: 2 ins; 0 del; 0 mod;
>>>>>>> - full: http://cr.openjdk.java.net/~iignatyev//8243432/webrev.00
>>>>>>>> 22 lines changed: 3 ins; 12 del; 7 mod;
>>>>>>> Thanks,
>>>>>>> -- Igor
>