Test using Random() should be seeded

[sdmaclea]

There are many tests using unseeded Random number generators. When failures happen it is difficult to debug/repro.

Here are examples which are currently causing intermittent failures in arm64...

tests/src/JIT/Methodical/MDArray/basics/classarr.cs
tests/src/JIT/Methodical/MDArray/basics/doublearr.cs
tests/src/JIT/Methodical/MDArray/basics/jaggedarr.cs
tests/src/JIT/Methodical/MDArray/basics/stringarr.cs
tests/src/JIT/Methodical/MDArray/basics/structarr.cs

Many tests are using seeded random number generators using code in tests/src/Common/CoreCLRTestLibrary/TestFramework.cs.

It would be nice to use simialr code in place of all tests using Random() without a seed.
category:correctness
theme:testing
skill-level:beginner
cost:small

[sdmaclea]

@jkotas Please mention correct people

[jkotas]

cc @dotnet/jit-contrib

[stefannikolei]

        if (Environment.GetEnvironmentVariable("CORECLR_SEED") != null)
        {
            try
            {
                seed = int.Parse(Environment.GetEnvironmentVariable("CORECLR_SEED"));
            }
            catch (FormatException) { seed = rand.Next(); }
        }
        else
        {
            seed = rand.Next();
        }

You suggest to use this code on all tests which use Random?

[sdmaclea]

@reaction1989 that code plus a fixed default seed and the code to print the seed into the trace. Fixed default seed may want to be unique per test.

[stefannikolei]

i would tackle this. If someone wanna assign me

[pgavlin]

@reaction1989: I don't think we can formally assign you since you're not affiliated with any of the required GitHub organizations, but feel free to take this on in any case. Thanks!

[danmoseley]

@reaction1989 still interested?

[tannergooding]

@sdmaclea, @reaction1989. I don't believe a fixed seed is the right way to go. That causes you to lose the benefits of using RNG (which is testing arbitrary inputs behave correctly).

The issue here is that:

1. There is no way to tell the test the seed to use
2. The tests do not tell you the seed that was used

If we had a way of doing both, then it should resolve all the issues while keeping the benefits.

[sdmaclea]

The typical usage in other tests is to use a fixed seed (i.e. the date) for CI and print the result. The seed may be from the command line. It is certainly acceptable to randomly generate a seed, print it, then use it as the seed. So that the failures can be reproduce. However, that means the logs for all failures must be available. I have not always been able to find the logs.

[tannergooding]

The typical usage in other tests is to use a fixed seed (i.e. the date)

This is probably the disconnect. I wouldn't consider "the date" a fixed seed, as it could vary from run to run 😄 (I would probably say the seed comes from a fixed source instead)

However, that means the logs for all failures must be available. I have not always been able to find the logs.

Right, I think this is the real problem.

Because the logs don't tell you the seed used, there is currently no chance for reproducability.

[sdmaclea]

In the date cases, the date was the date the seed was chosen, not the current date.

[sdmaclea]

For instance

tests/src/Common/CoreCLRTestLibrary/TestFramework.cs:            seed = 20010415;

[tannergooding]

@sdmaclea, I don't see how that is useful. It means that you don't get any of the benefits that using random inputs would normally provide (one of which is testing values that the dev didn't consider "special" or needing explicit tests).

At that point, why not just pick random inputs and hard code them.

[sdmaclea]

Take a look at the file for the whole context. It allows environment variable to override the seed. It prints the seed used in the logs.... CI is not for exhaustive checking typically, it is generally for general coverage.

If you want more exhaustive testing you can run the test with more seeds.... But that is not needed in general CI.

[tannergooding]

If you want more exhaustive testing you can run the test with more seeds.... But that is not needed in general CI.

The point is that it is no more work (the same number of tests get run), but it provides "greater" coverage (as it tests more inputs over the course of multiple runs).

The fact that there are tests without fixed seeds that are failing is "proof" that there is something wrong somewhere (some random input is causing a failure). If you change that to be a fixed seed, the inputs will never change and the tests will no longer "randomly" fail and you will miss out on the fact that something is wrong (either in the test code or in source code).

That is why using a fixed source seed (such as DateTime.Now) combined with printing said seed to the output (for reproducability) is a better solution.