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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

mpdonadio’s picture

A 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.

dawehner’s picture

I 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.

borisson_’s picture

@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.

klausi’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
9.2 KB

Cool, looks like we have general agreement on this.

First patch what I have in mind.

Status: Needs review » Needs work

The last submitted patch, 5: random-remove-tests-2571183-5.patch, failed testing.

The last submitted patch, 5: random-remove-tests-2571183-5.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
9.52 KB
1.14 KB

Oh yeah, we need to generate different feed titles each time.

Status: Needs review » Needs work

The last submitted patch, 8: random-remove-tests-2571183-8.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review

Tests are passing.

klausi’s picture

And also replaced a couple of randomString() calls to also show that conversion.

dawehner’s picture

+++ b/core/modules/simpletest/src/RandomGeneratorTrait.php
@@ -11,6 +11,10 @@
+ * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.

@@ -37,6 +41,11 @@
+   * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.

@@ -64,6 +73,11 @@ public function randomString($length = 8) {
+   * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.

@@ -93,6 +107,11 @@ public function randomStringValidate($string) {
+   * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.

Nitpick alarm!!:

~/w/d8 (8.0.x) $ ag "@deprecated in Drupal 8.0.x" | wc -l
      43
~/w/d8 (8.0.x) $ ag "@deprecated in Drupal 8.x" | wc -l
       5
klausi’s picture

"@deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0." is used even more, choosing that.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +rc eligible

Thank you

alexpott’s picture

I 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

klausi’s picture

@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.

alexpott’s picture

@klausi nope it found a real bug see #1655422-11: Random test failures in SearchCommentTest

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs framework manager review, +Needs subsystem maintainer review

Yeah, 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.

xjm’s picture

Issue tags: -rc eligible
klausi’s picture

Hm, 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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

When 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.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

claudiu.cristea’s picture

Yet 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".

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Version: 8.4.x-dev » 8.5.x-dev
Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: -Needs subsystem maintainer review

I'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.

alexpott’s picture

I don't think #24 or #17 or #15 have been adequately answered.

alexpott’s picture

To 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.

joachim’s picture

> 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.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

To 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.

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.

lokapujya’s picture

+1 to #30.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jonathanshaw’s picture

So 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.

Krzysztof Domański’s picture

IMO 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?

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Component: simpletest.module » phpunit

Simpletest module is obsolete, moving to testing component, phpunit.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.