[SUREFIRE-2055] Always show random seed

[delanym]

Always show the random seed when using the random runOrder. This is so tests that fail because of their runOrder can be replayed without necessarily knowing the build command.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
• In any other case, please file an Apache Individual Contributor License Agreement.

[slawekjaranowski]

There is IT test: RunOrderIT

Maybe assertions can be added to verify it.

[slawekjaranowski]

@delanym

• please check build result
• PR template contains important things - should be preserved

[delanym]

@slawekjaranowski how do I get the PR template back? What's wrong with my IT test?

[slawekjaranowski]

you cen edit PR description and copy template from https://github.com/apache/maven-surefire/blob/master/.github/pull_request_template.md

[slawekjaranowski]

build approval ... we will see the result

[slawekjaranowski]

please consider to split into two test, eg:

• testRandomJUnit4PrintSeedWithGivenSeed
• testRandomJUnit4PrintSeedWithNoGivenSeed

[Tibor17]

Hi @delanym . Nice to meet you after the release Vote!
I know that it is a matter of taste but these names of classes and methods determine the length of folders on the file system in our CI. Using WithSeed and WithoutSeed is shorter.
Now, I have realized that @delanym is using two executions which might be utilized in some way.
@delanym what wat your point? You wanted to compare the XML reports and the order of tests? Something else? If you want to make sure that the seed is set, such situation can be verified with the old IT where a debug message can be printed in the logs and the IT can check that the message was printed with seed != NULL.

[slawekjaranowski]

I understand that new tests verify if expected message is printed in standard info level nevermind if seed is given or not.
Message should be printed always for random orders.

ok, name of test method can be shorter 😄 , but in this case is only 40 chars it is acceptable

[Tibor17]

@delanym
We are doing these things twice. The seed is set to System.nanoTime() if it was NULL, see DefaultRunOrderCalculator.
This means that the only work here should be different, we should change the condition in the IF statement but nothing more. WDYT? Pls comment on this!

        Long runOrderRandomSeed = runOrderParameters.getRunOrderRandomSeed();
        if ( runOrderRandomSeed == null )
        {
            runOrderRandomSeed = System.nanoTime();
            runOrderParameters.setRunOrderRandomSeed( runOrderRandomSeed );
        }
        this.random = new Random( runOrderRandomSeed );

[Tibor17]

@slawekjaranowski Do you remember all the years we solved together with Robert and Infra team because the Windows paths were limited on the file system? They are still limitted however the limit is longer but it still exists! Also the infra team prolonged the limit of CMD line in bash but it still exists.

[Tibor17]

@delanym I made RunOrderParameters immutable because the contributor did not implement it right and the setter was not necessary. Pls rebase your branch on origin/master. You should exclude the fallback value System.nanoTime() in the DefaultRunOrderCalculator constructor and use it only in MOJO. It does not make sense to do one thing twice with the seed.
Finally this.random = new Random( runOrderRandomSeed == null ? System.nanoTime() : runOrderRandomSeed ); would become random = runOrderRandomSeed == null ? null : new Random( runOrderRandomSeed );.

[delanym]

Have I done that right?

[Tibor17]

Personally I would expect some warning that the seed is missing.
If random, then yes, we should use random and prevent from any cohesion between the neighbouring runs.

[Tibor17]

Let's see the implementation, because the impl has much better opportunities to use real random number if this parameter is NULL.

[slawekjaranowski]

runOrderRandomSeed is never NULLm if is not provided it will be set to nanoTime.

Currently is printed only when is not given in configuration, change introduce to print always - also if is given by configuration.

[Tibor17]

@slawekjaranowski runOrder can be other than random and so DefaultRunOrderCalculator may obtain null seed.
@delanym see my comment #505 (comment)

[slawekjaranowski]

Why change in this way ...?
When rundom will be null we will have NPE from Collections.shuffle( testClasses, random );

[slawekjaranowski]

IMHO here runOrderParameters.getRunOrderRandomSeed(); never be null

[slawekjaranowski]

lgtm

[slawekjaranowski]

@Tibor17 any more remarks ... I'm going to merge it.