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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

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

Status: Needs review » Needs work

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.12 KB

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

alexpott’s picture

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.

alexpott’s picture

Issue summary: View changes

Updated issue summary.

tsphethean’s picture

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.

alexpott’s picture

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

alexpott’s picture

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

alexpott’s picture

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

alexpott’s picture

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.

tsphethean’s picture

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

tsphethean’s picture

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.

alexpott’s picture

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

alexpott’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
16.55 KB
7.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.

alexpott’s picture

Berdir’s picture

Status: Needs work » Needs review
Berdir’s picture

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? ;)

...

Damien Tournoud’s picture

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

alexpott’s picture

alexpott’s picture

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

alexpott’s picture

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.

catch’s picture

Priority: Normal » Critical
Berdir’s picture

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

alexpott’s picture

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.

alexpott’s picture

Issue summary: View changes

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

alexpott’s picture

Tagging

alexpott’s picture

Issue summary: View changes

Updated

Berdir’s picture

  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 :)

catch’s picture

Issue tags: +Approved API change

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

alexpott’s picture

Addressed point 3 from #28

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Title: WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to fail it's xpath match » Change 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.

catch’s picture

tstoeckler’s picture

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.

andypost’s picture

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

Berdir’s picture

Title: Change notice: WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to fail » Random 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
FileSize
826 bytes

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.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Makes sense - nice catch

catch’s picture

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.

xjm’s picture

Title: Random fail: WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to fail » Change 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.

xjm’s picture

Issue summary: View changes

Updated issue summary.

mr.baileys’s picture

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

  $string = Random::string(8);

After

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

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;
  }
}
mr.baileys’s picture

Issue summary: View changes
Status: Active » Needs review

Setting to needs review for the change record.

alexpott’s picture

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

mr.baileys’s picture

Title: Change notice: WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to fail » WebTestBase::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.