Problem/Motivation
Whenever RandomGeneratorTrait is used in a test case we are risking random test failures. Test runs should be consistent: the values used for testing must always be the same, because we always want the same results when we don't change any code. Random values introduce uncertainty, inconsistency and complexity into automated test cases and we should avoid that.
Countless developer hours have been wasted hunting down random test failures in Drupal core, so we must limit randomness in test case where possible.
Proposed resolution
Mark RandomGeneratorTrait and all methods on it as @deprecated. Convert usage of those random methods in tests to hard-coded string values to have the desired predictability of tests. If special characters must be tested then an additional test case must be added with such characters explicitly instead of implicit testing with the random methods.
Remaining tasks
Write and review initial patch deprecating the random*() functions + some example conversions.
User interface changes
none.
API changes
none.
Data model changes
none.
Comment | File | Size | Author |
---|---|---|---|
#13 | interdiff.txt | 1.95 KB | klausi |
#13 | random-remove-tests-257118313.patch | 12.82 KB | klausi |
Comments
Comment #2
mpdonadioA similar problem to the methods on RandomGeneratorTrait are direct usages of rand() and relatives in tests. NumberFieldTest is a particularly bad case of this, especially the ::testNumberFormatter() method. Simulating additional coverage via Monte Carlo values is a ticking time bomb.
Comment #3
dawehnerI agree the randomness makes it actually quite hard to debug at the end ... it makes hard to understand the setup method and more in general moves us away from the idea of fixtures.
On the other hand we need to ensure thinking about the edge cases as this is kinda what the randomness can care about. Having special characters etc. in there is than kinda fundamental.
Comment #4
borisson_@dawehner, we can have the same special characters we care about in the new string that'll be used as a replacement for the random generator.
When we meet edge cases, I think it's better to have a special test with those specific characters in it to make sure those specific failures don't show up again.
Comment #5
klausiCool, looks like we have general agreement on this.
First patch what I have in mind.
Comment #8
klausiOh yeah, we need to generate different feed titles each time.
Comment #10
klausiTests are passing.
Comment #11
klausiAnd also replaced a couple of randomString() calls to also show that conversion.
Comment #12
dawehnerNitpick alarm!!:
Comment #13
klausi"@deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0." is used even more, choosing that.
Comment #14
dawehnerThank you
Comment #15
alexpottI think we need to be careful here. Random text does generate random fails BUT sometimes it helps discover bugs that we would not have found. For example #1655422: Random test failures in SearchCommentTest
Comment #16
klausi@alexpott: not sure if the issue that you linked is a good example. Only bugs in test code were fixed?
As guy_schneerson said in that issue: "We just had this issue on our first core patch and was a confusing experience, not saying it is a major issue but should definitely be fixed ASAP".
Random test fails disrupt the whole issue queue. This can cumulate to quite a bit of work for contributors debugging and figuring out what is going on. I argue that it is not worth it to have monte carlo testing coverage if we interfere with the work of many contributors. The amount of work for contributors far outweighs the small number of bugs we find with random testing.
Comment #17
alexpott@klausi nope it found a real bug see #1655422-11: Random test failures in SearchCommentTest
Comment #18
xjmYeah, I'm not sure about this either. It's true that our random generators are often misused or overused, and that random failures cause loss of productivity. I think all of us on this issue have spent scores of hours debugging them and trying to make our test suite more stable and reliable. But this patch feels like addressing the the symptom instead of the cause -- or cutting off your hand because you have arthritis.
Moving back to NR for more discussion. Also tagging for framework manager and subsystem maintainer review since this actually has pretty big implications.
Comment #19
xjmComment #20
klausiHm, why do you think this only addresses the symptom? Sure, we have many causes for random test fails (testbot disk is full, multiple test runs interfering with each other etc.), but using random test data is also a significant source of random test fails. Random test data is a real cause.
Sometimes we might be lucky and catch a bug with our random test data, but even then we spend huge amounts of time figuring out what is even going on because the test fail is hard to reproduce.
Comment #22
catchWhen we originally added the random string functions it was before we always ran tests in clean environments - idea was to guarantee uniquely named entities.
While there have been a couple of cases where they've found real bugs, we could likely achieve the same with providers - i.e. have a fixed set of strings meeting various criteria and run tests with each. Then we'd get the fails without the randomness.
Not sure whether me and Alex Pott agree on this one, but we've both reviewed/commented so removing the tag.
Comment #24
claudiu.cristeaYet another bug spotted by our random string generator #2808085: Pipe char in locators break Mink and Symfony element search. The bug was revealed in #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits when test failed only when the random string returned a pipe char ('|'). Probably no one would have been used such character in a test. I think that the randomness is good in tests. I would close this with "won't fix".
Comment #27
xjmI've started recommending non-random string names in most tests. Agreed also on making more use of data providers, although there is a factor that @pfrenssen mentioned where for browser and especially JS tests increase test run time significantly when we use providers (since each provider entry is a new install). But servers are still cheaper than people. Plus, having actually meaningful names in fixtures makes debugging easier than
mklcadsh
or whatever.Untagging subsystem review since the window to provide feedback has come and gone as well; it gets escalated to framework managers and they have signed off.
Let's update the docs in the patch to point people at how to use data providers as well for things that are part of what's being tested. We also obviously need to skip this deprecation at first since it's hugely disruptive for core, but I still think we should add the
@trigger_error()
always in the issue.Comment #28
alexpottI don't think #24 or #17 or #15 have been adequately answered.
Comment #29
alexpottTo expand in #28. The problem with the provider pattern is that it is expectS developers to come up with all edge cases. I know that I'm not able to come up with all possible cases especially for string input into a free text field.
randomString()
explicitly adds an&
and a>
into all random strings to ensure that output is correctly escaped. By recommending to not use randomString() we are reducing security coverage and overall test coverage in favour of not having to work out why a random error occurs and as #24, #17 and #15 point out this often reveals real bugs that would not have been caught by providers because the developer was not thinking that way when they wrote the test.Comment #30
joachim CreditAttribution: joachim as a volunteer commented> The problem with the provider pattern is that it is expectS developers to come up with all edge cases. I know that I'm not able to come up with all possible cases especially for string input into a free text field.
To help with that, we could add methods that provide fixed sample strings that contain the edge cases.
That would catch failures immediately, rather than hoping an edge case will get thrown up by the random generator.
Comment #32
borisson_I think that can be fixed by instead of using
randomMachineName
we can create a new method that always returns the same machine name, with|>&
and other special characters in it. I'm not sure if that will resolve the issues here.Comment #33
lokapujya+1 to #30.
Comment #38
jonathanshawSo we want the additional test coverage that randomisation gives, but we want intermittent failures to be easier to debug.
Here's an idea:
Php's mt_srand() can be used to set the random seed before a test run. If we set it at the start of a test run, and logged the seed, then we'd be able to reproduce the exact circumstances of any "random" failure, without sacrificing significant test coverage.
There's no reason that randomisation has to equate to unreproducibility.
Comment #39
Krzysztof DomańskiIMO random() disturbs and helps with a similar frequency. Also random strings are not the most common cause of random test failure. See #2829040: [meta] Known intermittent, random, and environment-specific test failures. The most troublesome are JS random test failure. They occur frequently and are difficult to eliminate.
There are tests where random strings are unnecessary but deprecating random() doesn't solve the problem, it just hides it.
+1 to #30. Can someone add a follow-up?
Comment #42
quietone CreditAttribution: quietone as a volunteer commentedSimpletest module is obsolete, moving to testing component, phpunit.