Updated: Comment #26

Problem/Motivation

Follow-up of #2004408: Allow modules to alter the result of EntityListController::getOperations and #2020209: Random failures due to non uniqueness of randomString() and randomName() and #1988780: DrupalWebTestCase::buildXPathQuery() tries to handle backreferences in argument values

This issue originally was solving the issue fixed in #1988780: DrupalWebTestCase::buildXPathQuery() tries to handle backreferences in argument values however in testing we have discovered another set of circumstances where using random strings will cause assertLink to fail - if it contains 2 consecutive spaces see #2076929: EntityOperationsTest random fail for an example.

This issue is caused by the fact that we normalise spaces in assertLink()

$links = $this->xpath('//a[normalize-space(text())=:label]', array(':label' => $label));

Proposed resolution

Add the ability to validate random strings during generation and if invalid generate another.

Remaining tasks

Get reviews

User interface changes

N/a

API changes

  • $validator param added to \Drupal\Component\Utility\Random::string() it's optional.
  • Methods on \Drupal\Component\Utility\Random are no longer static which makes unit testing possible

Original report

See revisions - this has been extensively rewritten.

Files: 
CommentFileSizeAuthor
#36 random-fail-ouch-2032453-36.patch826 bytesBerdir
PASSED: [[SimpleTest]]: [MySQL] 59,067 pass(es).
[ View ]
#30 26-30-interdiff.txt2.95 KBalexpott
#30 2032453.randomstring-validator.30.patch14.46 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,331 pass(es).
[ View ]
#26 23-26-interdiff.txt981 bytesalexpott
#26 2032453.randomstring-validator.26.patch14.43 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,116 pass(es).
[ View ]
#23 21-23-interdiff.txt2.3 KBalexpott
#23 2032453.randomstring-validator.23.patch14.45 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,958 pass(es).
[ View ]
#21 2032453.randomstring-validator-will-fail.21.patch16.22 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 57,192 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#21 17-21-interdiff.txt3.62 KBalexpott
#21 2032453.randomstring-validator.21.patch17.16 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,963 pass(es).
[ View ]
#17 15-17-interdiff.txt445 bytesalexpott
#17 2032453.randomstring-validator.17.patch16.55 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,711 pass(es).
[ View ]
#15 13-15-interdiff.txt7.49 KBalexpott
#15 2032453.randomstring-validator.15.patch16.55 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#13 8-13-interdiff.txt5.46 KBalexpott
#13 2032453.randomstring-validator.13.patch12.21 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 56,429 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#11 randomstring_strreplace-2032453-11.patch3.49 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 56,776 pass(es).
[ View ]
#8 2032453.randomstring-validator.8.patch9.94 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,388 pass(es).
[ View ]
#8 4-8-interdiff.txt1.26 KBalexpott
#7 interdiff-5-will-fail.txt1.25 KBalexpott
#7 randomstring_strreplace-2032453-5.will-fail.patch2.99 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 56,685 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#5 randomstring_strreplace-2032453-5.patch3.15 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 57,014 pass(es).
[ View ]
#4 4-5-interdiff.txt1.64 KBalexpott
#4 2032453.randomstring-validator.5.patch10.2 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,455 pass(es).
[ View ]
#3 2032453.randomstring-validator.4.patch10.12 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 57,038 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
randomstring-simpletest.patch2.46 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] 56,705 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Comments

It also will fail if it contains two consecutive spaces...

Status:Needs review» Needs work

The last submitted patch, randomstring-simpletest.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.12 KB
FAILED: [[SimpleTest]]: [MySQL] 57,038 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here's a patch that adds the ability to validate a randomString

What is interested is that these tests proved we have issues with tests bleeding to each because they where using Drupal::config functions.

No interdiff because this approach is quite different... it uses PHPUnit to mock the abstract...

StatusFileSize
new10.2 KB
PASSED: [[SimpleTest]]: [MySQL] 56,455 pass(es).
[ View ]
new1.64 KB

Added test case from #2032565: Test failures due to special character (combinations) in random strings and improved test strings... you can never have too many pastes.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new3.15 KB
PASSED: [[SimpleTest]]: [MySQL] 57,014 pass(es).
[ View ]

Thinking about a slightly lower impact change, can we not just replace preg_replace with str_replace in the WebTestBase::buildXPathQuery method? It seems like we're only doing relatively simple string replacement so may get away with not using the regex.

It passes my sample test in the original post, curious what it does to all the other tests.

#5 yes but... this does not cope with all the issues that #4 does... we also have issues with consecutive spaces and also starting and ending with a space as the xpath normalises spaces.

And I'm pretty sure there are other combinations we have not come up with yet which will fail for different random reasons. That's why I think an implementation based on #4 is the way to go. Since it will be easy just to add the additional validations to the callback.

StatusFileSize
new2.99 KB
FAILED: [[SimpleTest]]: [MySQL] 56,685 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
new1.25 KB

Just to prove #6 copied values from #4 into the test from #5

StatusFileSize
new1.26 KB
new9.94 KB
PASSED: [[SimpleTest]]: [MySQL] 56,388 pass(es).
[ View ]

I've cleaned up an unnecessary abstraction that was in #4...

Thinking about this some more... I think the change from preg_replace<code> to <code>str_replace that #5 makes is the right thing to do and will mean that the validator callback can be simpler as we won't have to avoid strings that look like regex back references.

I think I agree - I get the same test failures locally on the double whitespace and leading/trailing whitespace issues with the patch from #7, but the tests that I've been looking at (i.e. using drupalCreateRole) do a trim() of randomString anyway with comments:

// In the role UI role names are trimmed and random string can start or
// end with a space.
$name = trim($this->randomString(8));

We could patch randomString further to not generate spaces in the string, so
chr(mt_rand(32, 126))
becomes
chr(mt_rand(33, 126))

StatusFileSize
new3.49 KB
PASSED: [[SimpleTest]]: [MySQL] 56,776 pass(es).
[ View ]

This patch adds a trim on the rolename which createRole() does on the randomly generated string, eliminates chr(32) from randomString generation and removes the double whitespace tests as they would no longer be possible.

We should not remove the space character from random strings... also trimming the rolename will fix other places where we can be caught out by this.

I did not mean that #5 was the entire fix... we need to combine #5 and #8...

StatusFileSize
new12.21 KB
FAILED: [[SimpleTest]]: [MySQL] 56,429 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new5.46 KB

So here is the combination of #5 and #8

We protected against:

  • random strings starting and ending with spaces
  • random strings containing two consecutive spaces

Regex back references are no longer a since switching to str_replace() - nice catch @tsphethean

I've added test coverage for assertLink - unfortunately PHPUnit testing this method proved overly complicated since it is protected - see testing your privates :)

Status:Needs review» Needs work

The last submitted patch, 2032453.randomstring-validator.13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new16.55 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new7.49 KB

Okay so making the random class static prevents it's reuse and makes it impossible to unit test as each test changes the starting conditions :(

Patch attached fixes that...

Status:Needs review» Needs work

The last submitted patch, 2032453.randomstring-validator.15.patch, failed testing.

StatusFileSize
new16.55 KB
PASSED: [[SimpleTest]]: [MySQL] 56,711 pass(es).
[ View ]
new445 bytes

Doh!

Status:Needs work» Needs review

Status:Needs review» Needs work

You asked for it ;)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -1169,7 +1176,38 @@ protected function settingsSet($name, $value) {
+   * @see \Drupal\Component\Utility\Random::name()

shouldn't this reference the string method?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/WebTestBaseTest.phpundefined
@@ -0,0 +1,51 @@
+   * Tests the behaviour of Drupal\simpletest\WebTestBase::assertLink().

\Drupal\...

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/WebTestBaseTest.phpundefined
@@ -0,0 +1,51 @@
+   */
+  public function testAsertLink() {

typo (Asert)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/WebTestBaseTest.phpundefined
@@ -0,0 +1,51 @@
+    foreach ($link_texts as $link) {
+      $result = $this->assertLink($link);
+      $this->assertTrue($result, 'The assertLink method found link.');

While adding tests anyway, should we also add a test to make sure it doesn't match a link that's not there, and maybe a string that's not a link?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -1877,7 +1877,7 @@ protected function buildXPathQuery($xpath, array $args = array()) {
-      $xpath = preg_replace('/' . preg_quote($placeholder) . '\b/', $value, $xpath);
+      $xpath = str_replace($placeholder, $value, $xpath);

The tricky part about the preg_replace() is the word boundary (\b), we are removing this here.

I've no idea if it is necessary, but I assume there was a reason why we put it here. I'm quite sure that this was written by @DamZ, so we should probably ask him.

+++ b/core/modules/simpletest/tests/Drupal/simpletest/Tests/TestBaseTest.phpundefined
@@ -0,0 +1,71 @@
+   * @var object

In CacheCollectorTest, I documented this as "\PHPUnit_Framework_MockObject_MockObject", this gives you autocomplete on mocking methods. There was some discussion about it, but it got in and I think it's quite useful.

+++ b/core/tests/Drupal/Tests/Component/Utility/RandomTest.phpundefined
@@ -18,6 +18,15 @@
+   * @see \Drupal\Tests\Component\Utility\RandomTest::_RandomStringValidate

missing () ?

+++ b/core/tests/Drupal/Tests/UnitTestCase.phpundefined
@@ -43,13 +50,27 @@ public static function getInfo() {
-   * @see \Drupal\Component\Utility::string()
+   * @see \Drupal\Component\Utility\Random::name()

That's what we put in after all these reviews? ;)

...

<?php
@@ -1877,7 +1877,7 @@ protected function buildXPathQuery($xpath, array $args = array()) {
        
// Return the string.
        
$value = count($parts) > 1 ? 'concat(' . implode(', \'"\', ', $parts) . ')' : $parts[0];
       }
-     
$xpath = preg_replace('/' . preg_quote($placeholder) . '\b/', $value, $xpath);
+     
$xpath = str_replace($placeholder, $value, $xpath);
     }
     return
$xpath;
   }
?>

Nack. It is not ok to remove the boundary check, as it makes :placeholder1 match for :placeholder11. Of course, the boundary check is not foolproof either, but it is better then nothing.

The easy fix for this issue described here is preg_replace_callback() with a lambda.

StatusFileSize
new17.16 KB
PASSED: [[SimpleTest]]: [MySQL] 56,963 pass(es).
[ View ]
new3.62 KB
new16.22 KB
FAILED: [[SimpleTest]]: [MySQL] 57,192 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Thanks for the reviews!

Status:Needs work» Needs review

Pressed save too soon... The will fail patch in #21 proves that using preg_replace_callback() will fix our issue whilst preserving the boundary check. Thanks @damz

StatusFileSize
new14.45 KB
PASSED: [[SimpleTest]]: [MySQL] 56,958 pass(es).
[ View ]
new2.3 KB

So I've just discovered and committed #1988780: DrupalWebTestCase::buildXPathQuery() tries to handle backreferences in argument values so this issue will just handle the addition of the random string validation callback so that strings generated by it don't contain 2 spaces in a row or start or end with a space.

Priority:Normal» Critical

I thought we had fixed this one, haven't seen this fail since the referenced issue above was committed.

StatusFileSize
new14.43 KB
PASSED: [[SimpleTest]]: [MySQL] 58,116 pass(es).
[ View ]
new981 bytes

This patch now prevents a much much rarer error - when there are two consecutive spaces inside a random string! Which is exactly what's occurred in #2076929: EntityOperationsTest random fail

This patch makes the random not a public static as this make it extremely difficult class to unit test and there does not seem to be any reason for it being so.

Reviewed and noticed a something wrong in the comments.

Issue summary:View changes

Merging issue summary from https://drupal.org/node/2032565

Tagging

Issue summary:View changes

Updated

  1. +++ b/core/lib/Drupal/Component/Utility/Random.php
    @@ -48,15 +48,20 @@ class Random {
    +  public function string($length = 8, $unique = FALSE, $validator = NULL) {

    In case someone is wondering, type hint on callable is only possible with PHP 5.4+

  2. +++ b/core/modules/simpletest/tests/Drupal/simpletest/Tests/TestBaseTest.php
    @@ -0,0 +1,71 @@
    +class TestBaseTest extends UnitTestCase {

    Testing Simpletest with PHPUnit. Interesting... ;)

  3. +++ b/core/modules/simpletest/tests/Drupal/simpletest/Tests/TestBaseTest.php
    @@ -0,0 +1,71 @@
    +   * @param string $str
    ...
    +  public function testRandomStringValidate($str, $expected) {

    I think we don't shorten variable names, so this should be $string.

Can you approve your own API change? Or do you need someone else to do that? I can't, but other than the above nitpick, can't find anything wrong with it, have reviewed the same code a few times already :)

Issue tags:+Approved API change

I think he can but so can I on API changes.

StatusFileSize
new14.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,331 pass(es).
[ View ]
new2.95 KB

Addressed point 3 from #28

Status:Needs review» Reviewed & tested by the community

Ok, let's do this. I have nothing to complain about anymore.

Title:WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to fail it's xpath matchChange notice: WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to fail
Category:bug» task
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed/pushed to 8.x.

Needs a change notice.

Not setting back to needs work, but I'm not sure that changing Random::$strings and Random::$names to no longer be static was a worthwhile API change. I agree on making the methods non-static but I don't see any reason why the variables were changed. Before you could rely on the fact that Random would generate a string (if wanted) that had not been generated during the entire request. This is no longer the case. Now you have to make sure that you're dealing with the exact same object. I'm not aware of any concrete implications but I also don't see this being discussed anywhere or the original issue with Random referenced, which contains a lot more discussion about this topic.

I can confirm that I got a few false failures in the test RandomTest::testRandomStringValidator()
for example https://qa.drupal.org/pifr/test/625203

Title:Change notice: WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to failRandom fail: WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to fail
Category:task» bug
Priority:Major» Critical
Status:Active» Needs review
StatusFileSize
new826 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,067 pass(es).
[ View ]

Confirmed, this does fail randomly, always with the same test assertion:

$ ./vendor/bin/phpunit --repeat 1000 tests/Drupal/Tests/Component/Utility/RandomTest.php
...
Time: 7 seconds, Memory: 4.50Mb
There were 13 failures:
1) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that ')' is not equal to <string:)>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
2) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that 'N' is not equal to <string:N>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
3) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that 'r' is not equal to <string:r>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
4) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that '5' is not equal to <string:5>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
5) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that '@' is not equal to <string:@>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
6) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that 'p' is not equal to <string:p>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
7) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that '?' is not equal to <string:?>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
8) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that 'G' is not equal to <string:G>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
9) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that 'i' is not equal to <string:i>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
10) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that 'Y' is not equal to <string:Y>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
11) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that '}' is not equal to <string:}>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
12) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that '!' is not equal to <string:!>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
13) Drupal\Tests\Component\Utility\RandomTest::testRandomStringValidator
Failed asserting that '%' is not equal to <string:%>.
/home/berdir/Projekte/d8/core/tests/Drupal/Tests/Component/Utility/RandomTest.php:156
FAILURES!
Tests: 8000, Assertions: 69000, Failures: 13.

See attached patch. With that, not getting any failures with --repeat 10000.

Status:Needs review» Reviewed & tested by the community

Makes sense - nice catch

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Title:Random fail: WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to failChange notice: WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to fail
Category:bug» task
Priority:Critical» Major
Status:Closed (fixed)» Active

The change notice still hasn't been written.

Issue summary:View changes

Updated issue summary.

My first attempt at writing a change record:

  • Should the new getRandomGenerator-function mentioned as part of the change record?

Change record

Description

  • Methods on \Drupal\Component\Utility\Random are no longer static which makes unit testing possible.
  • An optional $validator parameter has been added added to \Drupal\Component\Utility\Random::string(). The parameter takes a callable that returns TRUE if the generated string is valid, FALSE otherwise. In case of an invalid string, Random::string() will attempt to generate a new random string until the string is valid or MAXIMUM_TRIES is reached.

Before

<?php
  $string
= Random::string(8);
?>

After

<?php
  $random_generator
= new Random();
 
$string = $random_generator->string(8);
?>

The $validator callable can be used to discard randomly generated strings that are invalid. The example below ensures that randomly generated strings do not start with a digit.

<?php
class RandomExample {
  public function
generateString() {
   
$random_generator = new Random();
    return
$random_generator->string(8, TRUE, array($this, 'randomStringValidate'));
  }
 
/**
   * Callback for random string validation.
   */
 
public function randomStringValidate($string) {
   
// Reject random strings that start with a digit.
   
if (preg_match('/^\d/', $string)) {
      return
FALSE;
    }
    return
TRUE;
  }
}
?>

Issue summary:View changes
Status:Active» Needs review

Setting to needs review for the change record.

#41 looks great. Once you've created a https://drupal.org/node/add/changenotice - then you can just mark this issue as fixed. Thanks!

Title:Change notice: WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to failWebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to fail
Category:Task» Bug report
Status:Needs review» Fixed
Issue tags:-Needs change record

Change notice added: https://drupal.org/node/2175701

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.