Should srand be disabled by default in Test2::V#? (Was: The -no_rand option is undocumented)

[Corion]

As Test2::V0 runs all tests with the same srand() value, there is tempfile contention as File::Temp expects a different srand() value for every test . This happens once 1000+ tempfiles have been created.

Only after source-diving, the -no_srand import option is found for Test2::V0 . Please document that option.

[eserte]

I wonder why -srand is the default, as it's use is surprising and obviously breaking things.

[exodist]

It is the default because it solves a problem that has been expressed to me many times, and has largely been a problem at multiple workplaces. the problem is reproducible. As soon as you use randomness in a test you introduce the possibility that most random values pass, but occasionally some randomness causes a bug you cannot reproduce unless you happen upon the same initial values for the RNG.

My preferred solution would be "Don't do that" I feel that randomness in a test (at least on things you are intentionally testing) is asking for trouble and poor design. If you "don't do that" then srand has no impact on the tests at all (though, yes, side effects on things like File::Temp.

But since pretty much every place I have ever worked, and a good selection of cpan modules do not side with me, and embrace randomness in tests I was forced to introduce a tool that makes it possible to reproduce failures caused by "random" data. But I also needed to preserve the behavior many desired of catching edge cases exposed by the changes in random data.

The compromise was srand, it uses the date as a seed so that on any given day the test behavior is fully reproducible. You can also specify a seed to reproduce the results from any day. However from day to day the randomness changes so that you still catch the edge cases exposed by changes in "random" data.

The default is to have srand on so that by default you can fully reproduce failure conditions. If srand was off there would be no way to know/guarantee the RNG starting condition when trying to reproduce failures. It is default because having it on makes some forms of debugging possible that would be impossible if it was off. You can turn it off, but if you do then you are responsible for breaking reproducibility.

[Corion]

I think this errs on the wrong side of the reproducibility issue, simply because its effect on File::Temp. This implicit srand makes sure that File::Temp always encounters conflicting filenames and will churn a lot to find unique filenames in parallel test execution.

I think more test files use File::Temp than use randomness, and I also think -no_srand should be the default behaviour.

[eserte]

Yes, randomness in tests, or difficult reproducibility is a problem. However, the overwhelming majority of such problems is not caused by srand()/rand(), but:

• race conditions (for example if parallelism is in the game)
• timing problems
• hash randomization (yes, randomization here, but one which is not controlled by srand())
• failing tests only on peculiar dates, times or timezones
• transient network problems
• dependency problems (optional dependencies, different versions)

and probably more.

Yes, it's possible that test failures can happen because rand() is deliberately used. If a test case uses random input data, then just make sure that the used input data is printed on failures. If the randomness happens within the code, well, then the user can always call srand() himself.

I also think the danger of unwanted side effects is larger than the benefits, and the default should be changed.

[exodist]

Unfortunately this is not so easily changed. This default has been in place for many years. I have also worked at 3 companies in that time, and 2 of them have code that depends on this behaviors, changing the default will break their expectations, and cause serious issues for them. I would not be surprised if cpan modules that use this also have ill effects from what I would consider a "breaking change".

There are however options, and they are not exclusive, we can do a combination

1. Make Test2::V0, when srand is enabled (still the default) use File::Temp to create a $TMPDIR/[RAND] dir BEFORE calling rand(), then modify $ENV{TMPDIR} to point at the new directory. This way every new test will have a clean directory without conflicts from other tests.
2. We can release a Test2::V1 with the default changed.
3. We can add environment variables to control the behavior that cpan testers can set
4. We can toggle the default based on the presence of existing cpantesters variables that should be present

Personally I like idea 1 best. It has the following benefits:

• Declutters $TMPDIR as a single test, no matter how many temp files it creates, will only have 1 item dropped into the temp dir
• We can have it also add a file into the tempdir saying what directory and test filename created the temp dir so that we can report failures to clean up temp files to test authors (Which honestly is the real problem here, if tests properly cleaned up after themselves we would not have these conflicts)

Even number 1 could be considered a breaking change unfortunately, people could be depending on $TMPDIR not being different between tests for things like resource lock files.

So I think we need a combination of 1 and 2 for sure, cause whatever we do it will be a breaking change, and I promised in the module docs not to make breaking changes without releasing a new Test2::V# module, leaving the old behavior intact in the previous one.

I am a little hesitant on the third and fourth because it means creating divergent behavior between cpantesters and non-cpantesters. That said as long as it is clearly documented it is probably acceptable.

[Corion]

I think option 1) is a non-starter as it just shuffles the problem around and requires changes to an otherwise unrelated module just for placating the change that Test2 brings.

Option 2) with Test2::V1 would be the simple and most straightforward approach. This breaks nobodys code and since Test2 adoption is fairly low still, people will gravitate naturally towards the new version.

The other two options are non-starters as you already said.

[exodist]

@Corion what do you mean when you say "requires changes to an otherwise unrelated module", which module? My proposal did not have any changes to anything that is not in the Test2::Suite dist.

[Corion]

Oh, sorry! I misread your option 1) - I thought you meant changing File::Temp. Creating a tempdir and setting up the environment isn't a great approach but yes, that would make option 1) workable. I still prefer option 2), as it would be a much cleaner cut without side effects.

[Leont]

Make Test2::V0, when srand is enabled (still the default) use File::Temp to create a $TMPDIR/[RAND] dir BEFORE calling rand(), then modify $ENV{TMPDIR} to point at the new directory. This way every new test will have a clean directory without conflicts from other tests.

That wouldn't help anyone who explicitly sets the DIR (this is sometimes necessary, for example because TMPDIR might not allow executable files). But it would go a long way helping most people.

We can release a Test2::V1 with the default changed.

If there is a V1 then we should absolutely change the default. Useful but surprising behavior should be opt-in. That said, I'm sure that after X years of experience there are a few more such optimizations to be made so lets inventorize those first before making a new module.

[exodist]

@Leont agreed on inventorying changes to make. I will add a label for github issues that should be addressed by ::V1, I will also re-open this ticket and add it to that label.

[exodist]

oops, intended to also say I am not going to rush a ::V1 out the door. Unless there is a compelling reason to do it sooner, I suspect the toolchain summit (~April of 2024?) would be a good time to aggregate ::V1 changes and make a release.

[Corion]

Yes, IMO there is no need for an immediate rush, and it makes sense to collect more things for V1 than just this.

[haarg]

I think changing this for a V1 is the only option that makes sense. Adding more unexpected changes to the test environment, like changing TMPDIR, is likely to just add confusion.

[eserte]

Another idea for an interim time: change the V0 documentation so that it's clear that

use Test2::V0 -no_srand => 1;

is the preferred way to use the module.

[exodist]

Another idea for an interim time: change the V0 documentation so that it's clear that

use Test2::V0 -no_srand => 1;

is the preferred way to use the module.

I will probably word it a bit differently, but I will for sure make it clear at the top of the docs that it is less likely to cause certain issues.