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

[Igor Ignatyev]

thanks for your review David, pushed.

-- Igor

> On Apr 30, 2020, at 9:58 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> 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