We rely on chance to ensure that drupalCreateRole and drupalCreateUser do not fail if called multiple times in the same test. This is a bad idea.

CommentFileSizeAuthor
#41 38-41-interdiff.txt1.64 KBalexpott
#41 2020209.randomuniques.41.patch13.69 KBalexpott
#38 37-38-interdiff.txt2.61 KBalexpott
#38 2020209.randomuniques.38.patch13.59 KBalexpott
#37 36-37-interdiff.txt766 bytesalexpott
#37 2020209.randomuniques.37.patch13.1 KBalexpott
#36 33-36-interdiff.txt706 bytesalexpott
#36 2020209.randomuniques.36.patch13.09 KBalexpott
#34 2020209.randomuniques.34-will-fail.patch1.76 KBalexpott
#33 2020209.randomuniques.33.patch12.67 KBalexpott
#33 29-33-interdiff.txt7.46 KBalexpott
#29 29-30-interdiff.txt1.07 KBalexpott
#29 2020209.randomuniques.29.patch11.75 KBalexpott
#28 26-28-interdiff.txt1.25 KBalexpott
#28 2020209.randomuniques.28.patch11.56 KBalexpott
#26 24-26-interdiff.txt3.08 KBalexpott
#26 2020209.randomuniques.26.patch11.55 KBalexpott
#24 2020209.randomuniques.24.patch11.78 KBalexpott
#22 looks-better-now.patch756 bytesGábor Hojtsy
#15 12-15-interdiff.txt1.13 KBalexpott
#15 2020209.randomuniques.15.patch3.24 KBalexpott
#12 10-12-interdiff.txt4.23 KBalexpott
#12 2020209.randomuniques.12.patch4.68 KBalexpott
#10 7-10-interdiff.txt531 bytesalexpott
#10 2020209.randomuniques.10.patch3.51 KBalexpott
#7 2020209.randomuniques.7.patch3.51 KBYesCT
#7 interdiff-6-7.txt1.13 KBYesCT
#6 4-6-interdiff.txt1.66 KBalexpott
#6 2020209.randomuniques.6.patch3.51 KBalexpott
#4 2-4-interdiff.txt790 bytesalexpott
#4 2020209.randomuniques.4.patch3.15 KBalexpott
#2 2020209.randomuniques.2.patch3.13 KBalexpott
#1 2020209.randomuniques.1.patch3.55 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
3.55 KB

And here's a patch...

alexpott’s picture

Blah my .htaccess file crept in :(

Berdir’s picture

Status: Needs review » Needs work
+++ b/.htaccessundefined
@@ -107,7 +107,7 @@ DirectoryIndex index.php index.html index.htm
-  # RewriteBase /
+  RewriteBase /

:)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -1179,6 +1179,27 @@ public static function randomString($length = 8) {
   /**
+   * Generates a unique random string containing letters and numbers.

@@ -1209,6 +1230,27 @@ public static function randomName($length = 8) {
+   * Generates a unique random string containing letters and numbers.

Same description for both?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -1179,6 +1179,27 @@ public static function randomString($length = 8) {
+   * @param $length
+   *   Length of random name to generate.

Missing type

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.15 KB
790 bytes

Thanks!

YesCT’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -1179,6 +1179,27 @@ public static function randomString($length = 8) {
+    } while (isset($names[$string]));

should this be
isset($strings... ?
copy and paste typo?

alexpott’s picture

Thanks @YesCT and @chx pointed out no need for these functions to be static too...

YesCT’s picture

some more copy and paste fix.

also,
https://drupal.org/node/1354#classes

If you use a namespace in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash).

Status: Needs review » Needs work

The last submitted patch, 2020209.randomuniques.7.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

#7: 2020209.randomuniques.7.patch queued for re-testing.

alexpott’s picture

Yet another c&p error from me :( spotted by @YesCT

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks great. I think it would have been equally fine to have this feature in the existing methods (as a base feature), but additional methods sound equally fine. This highlights the specifics of the behaviour definitely.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.68 KB
4.23 KB

I agree with #11...

alexpott’s picture

... as a result now is the time to make randomName and randomString non static...

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -487,7 +487,7 @@ protected function drupalCreateUser(array $permissions = array(), $name = NULL)
-    $edit['name']   = !empty($name) ? $name : $this->randomName();
+    $edit['name']   = !empty($name) ? $name : $this->randomNameUnique();
     $edit['mail']   = $edit['name'] . '@example.com';
     $edit['pass']   = user_password();
     $edit['status'] = 1;
@@ -527,13 +527,13 @@ protected function drupalCreateUser(array $permissions = array(), $name = NULL)

@@ -527,13 +527,13 @@ protected function drupalCreateUser(array $permissions = array(), $name = NULL)
   protected function drupalCreateRole(array $permissions, $rid = NULL, $name = NULL, $weight = NULL) {
     // Generate a random, lowercase machine name if none was passed.
     if (!isset($rid)) {
-      $rid = strtolower($this->randomName(8));
+      $rid = strtolower($this->randomNameUnique(8));
     }
     // Generate a random label.
     if (!isset($name)) {
       // In the role UI role names are trimmed and random string can start or
       // end with a space.
-      $name = trim($this->randomString(8));
+      $name = trim($this->randomStringUnique(8));

The *Unique methods don't exist anymore.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.24 KB
1.13 KB

Doh!

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks for putting it into the original implementation I think it will not hurt to have this unique at all times. I don't think people expect randomname to return sometimes the same values as before randomly :)

effulgentsia’s picture

This looks great to me as well. RTBC+1. @alexpott: since that's now two people who've reviewed it, I think it's okay for you to commit it, despite it being your patch.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 880d568 and pushed to 8.x. Thanks!

tstoeckler’s picture

Can someone explain how the $this->randomStrings and $this->randomNames are being set? It's not obvious to me from the patch.

Gábor Hojtsy’s picture

Wups, looks like it does not remember the old strings in fact.... They will always be an empty array....

Gábor Hojtsy’s picture

Status: Fixed » Needs work
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
756 bytes

Looks better now? :)

Berdir’s picture

Status: Needs review » Needs work

\Drupal\Tests\UnitTestCase has it's own implementation of randomName() that we should fix too.

alexpott’s picture

Status: Needs work » Needs review
FileSize
11.78 KB

So I've refactored randomName, randomString and randomObject into a new random component and added tests for the uniqueness of random names and strings. Additionally I have added a fail safe to ensure we don't end up in an endless loop.

I have ran the new phpUnit test over 1000 times locally to ensure this test does not introduce new random fails.

xjm’s picture

Looks great other than some stupid docs nitpicks.

+++ b/core/lib/Drupal/Component/Utility/Random.phpundefined
@@ -0,0 +1,132 @@
+ * Utility class for creating random data.

Defines a.... https://drupal.org/node/1354#classes

+++ b/core/lib/Drupal/Component/Utility/Random.phpundefined
@@ -0,0 +1,132 @@
+   * Do not use this method when special characters are not possible (e.g., in
+   * machine or file names that have already been validated); instead, use
+   * \Drupal\Component\Utility\Random::name().
...
+   * Do not use this method when testing unvalidated user input. Instead, use
+   * \Drupal\Component\Utility\Random::string().

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -1168,62 +1155,31 @@ protected function settingsSet($name, $value) {
-   * Do not use this method when special characters are not possible (e.g., in
-   * machine or file names that have already been validated); instead, use
-   * Drupal\simpletest\TestBase::randomName().
...
-   * Do not use this method when testing unvalidated user input. Instead, use
-   * Drupal\simpletest\TestBase::randomString().

+++ b/core/tests/Drupal/Tests/UnitTestCase.phpundefined
@@ -33,16 +35,7 @@ public static function getInfo() {
-   * Do not use this method when testing unvalidated user input. Instead, use
-   * Drupal\simpletest\TestBase::randomString().

Let's keep these lines in TestBase.

+++ b/core/tests/Drupal/Tests/Component/Utility/RandomTest.phpundefined
@@ -0,0 +1,83 @@
+ * Tests random data generating.

generation.

+++ b/core/tests/Drupal/Tests/Component/Utility/RandomTest.phpundefined
@@ -0,0 +1,83 @@
+    // There are less than 100 possibilities so an exception should occur to
...
+    // There are less than 100 possibilities so an exception should occur to

s/less/fewer/ :)

alexpott’s picture

Thanks @xjm

Berdir’s picture

+++ b/core/lib/Drupal/Component/Utility/Random.phpundefined
@@ -0,0 +1,132 @@
+      $values = array_merge(range(65, 90), range(97, 122), range(48, 57));
+      $max = count($values) - 1;

This doesn't need to be inside the loop, it's static.

alexpott’s picture

Good point...

alexpott’s picture

Ok missed the assertion messages :)

Status: Needs review » Needs work

The last submitted patch, 2020209.randomuniques.29.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#29: 2020209.randomuniques.29.patch queued for re-testing.

Berdir’s picture

+++ b/core/lib/Drupal/Component/Utility/Random.phpundefined
@@ -0,0 +1,129 @@
+ * Defines a utility class for creating random data.
+ */
+class Random {
...
+   * The maximum number of times name() and string() can loop.

We don't actually document anywhere that it *does* loop and does ensure unique random strings per request.

I think we should a) do that and b) provide a possibility to disable the uniqueness check, especially because there's no way to reset it and we explicitly want to make this available to non-test code.

Maybe that should even be the default and only the test methods that call out to this should enforce uniqueness?

+++ b/core/lib/Drupal/Component/Utility/Random.phpundefined
@@ -0,0 +1,129 @@
+      $max = count($values) - 1;

$max also doesn't change :)

alexpott’s picture

Thanks for the review @berdir...

alexpott’s picture

So lets prove we have the problem...

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +PHPUnit
+++ b/core/lib/Drupal/Component/Utility/Random.phpundefined
@@ -0,0 +1,139 @@
+  public static function object($size = 4) {
+    $object = new \stdClass();
+    for ($i = 0; $i < $size; $i++) {
+      $random_key = static::name();
+      $random_value = static::string();
+      $object->{$random_key} = $random_value;
+    }
+    return $object;

you will never know that maybe something changes stuff here and it potentially break. Let's ensure the size of the object in a test.

alexpott’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
13.09 KB
706 bytes

@dawehner here is a test for you...

I test 0 and 1 because different amounts of code will fire in Random::object depending on this value. So we have complete execution path coverage.

Based on the result of #34 marking this as major as random testbot failures greatly impact our velocity.

alexpott’s picture

alexpott’s picture

Improving docs...

ParisLiakos’s picture

nitpick alert:

+++ b/core/tests/Drupal/Tests/Component/Utility/RandomTest.phpundefined
@@ -0,0 +1,131 @@
+  function testRandomStringUniqueness() {
...
+  function testRandomNamesUniqueness() {

need visibility

Berdir’s picture

+++ b/core/lib/Drupal/Component/Utility/Random.phpundefined
@@ -0,0 +1,139 @@
+   * @param bool $unique
+   *   Ensure that the random string returned is unique.
...
+   * @param bool $unique
+   *   Ensure that the random name returned is unique.

nitpick alert^2.

Should start with (optional), maybe something like "If uniqueness of returned strings should be enforced, defaults to TRUE" ?

alexpott’s picture

Thanks for the reviews :)

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks!

catch’s picture

Status: Reviewed & tested by the community » Fixed

That looks better. Committed/pushed to 8.x, thanks!

Berdir’s picture

More random fun, here is the random problem that @alexpott and I discovered in Dublin: #2032565: Test failures due to special character (combinations) in random strings.

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