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.

Files: 
CommentFileSizeAuthor
#41 38-41-interdiff.txt1.64 KBalexpott
#41 2020209.randomuniques.41.patch13.69 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 56,496 pass(es).
[ View ]
#38 37-38-interdiff.txt2.61 KBalexpott
#38 2020209.randomuniques.38.patch13.59 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,096 pass(es).
[ View ]
#37 36-37-interdiff.txt766 bytesalexpott
#37 2020209.randomuniques.37.patch13.1 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#36 33-36-interdiff.txt706 bytesalexpott
#36 2020209.randomuniques.36.patch13.09 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#34 2020209.randomuniques.34-will-fail.patch1.76 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#33 2020209.randomuniques.33.patch12.67 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 57,984 pass(es).
[ View ]
#33 29-33-interdiff.txt7.46 KBalexpott
#29 29-30-interdiff.txt1.07 KBalexpott
#29 2020209.randomuniques.29.patch11.75 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 57,983 pass(es).
[ View ]
#28 26-28-interdiff.txt1.25 KBalexpott
#28 2020209.randomuniques.28.patch11.56 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 58,045 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#26 24-26-interdiff.txt3.08 KBalexpott
#26 2020209.randomuniques.26.patch11.55 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#24 2020209.randomuniques.24.patch11.78 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#22 looks-better-now.patch756 bytesGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#15 12-15-interdiff.txt1.13 KBalexpott
#15 2020209.randomuniques.15.patch3.24 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,542 pass(es).
[ View ]
#12 10-12-interdiff.txt4.23 KBalexpott
#12 2020209.randomuniques.12.patch4.68 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#10 7-10-interdiff.txt531 bytesalexpott
#10 2020209.randomuniques.10.patch3.51 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,207 pass(es).
[ View ]
#7 2020209.randomuniques.7.patch3.51 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 58,297 pass(es).
[ View ]
#7 interdiff-6-7.txt1.13 KBYesCT
#6 4-6-interdiff.txt1.66 KBalexpott
#6 2020209.randomuniques.6.patch3.51 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,007 pass(es).
[ View ]
#4 2-4-interdiff.txt790 bytesalexpott
#4 2020209.randomuniques.4.patch3.15 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 57,972 pass(es).
[ View ]
#2 2020209.randomuniques.2.patch3.13 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#1 2020209.randomuniques.1.patch3.55 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new3.55 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

And here's a patch...

StatusFileSize
new3.13 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Blah my .htaccess file crept in :(

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

Status:Needs work» Needs review
StatusFileSize
new3.15 KB
PASSED: [[SimpleTest]]: [MySQL] 57,972 pass(es).
[ View ]
new790 bytes

Thanks!

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

StatusFileSize
new3.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,007 pass(es).
[ View ]
new1.66 KB

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

StatusFileSize
new1.13 KB
new3.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,297 pass(es).
[ View ]

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.

Status:Needs work» Needs review

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

StatusFileSize
new3.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,207 pass(es).
[ View ]
new531 bytes

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

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.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new4.68 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new4.23 KB

I agree with #11...

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

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.

Status:Needs work» Needs review
StatusFileSize
new3.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,542 pass(es).
[ View ]
new1.13 KB

Doh!

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

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.

Status:Reviewed & tested by the community» Fixed

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

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

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

Status:Fixed» Needs work

Status:Needs work» Needs review
StatusFileSize
new756 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Looks better now? :)

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new11.78 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

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

StatusFileSize
new11.55 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new3.08 KB

Thanks @xjm

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

StatusFileSize
new11.56 KB
FAILED: [[SimpleTest]]: [MySQL] 58,045 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.25 KB

Good point...

StatusFileSize
new11.75 KB
PASSED: [[SimpleTest]]: [MySQL] 57,983 pass(es).
[ View ]
new1.07 KB

Ok missed the assertion messages :)

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

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

StatusFileSize
new7.46 KB
new12.67 KB
PASSED: [[SimpleTest]]: [MySQL] 57,984 pass(es).
[ View ]

Thanks for the review @berdir...

StatusFileSize
new1.76 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

So lets prove we have the problem...

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.

Priority:Normal» Major
Status:Needs work» Needs review
StatusFileSize
new13.09 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new706 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.

StatusFileSize
new13.1 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new766 bytes

StatusFileSize
new13.59 KB
PASSED: [[SimpleTest]]: [MySQL] 58,096 pass(es).
[ View ]
new2.61 KB

Improving docs...

nitpick alert:

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

need visibility

+++ 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" ?

StatusFileSize
new13.69 KB
PASSED: [[SimpleTest]]: [MySQL] 56,496 pass(es).
[ View ]
new1.64 KB

Thanks for the reviews :)

Status:Needs review» Reviewed & tested by the community

thanks!

Status:Reviewed & tested by the community» Fixed

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

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.