Make randomString always return a > to avoid random test fails

Makes sense to me.

In the current function (found in core/modules/simpletest/src/TestBase.php), an ampersand is returned in randomString generated strings where the requested length is greater than 2. Will the greater than ('>') need to be added to all strings or just to those greater than 2 (similar to the ampersand)?

Here's a patch for this. Adding a greater than ('>') into the pseudo-random string generated by randomString in core/modules/simpletest/src/TestBase.php. (This is my first patch, so I probably did not follow all the protocols. Tell me what I did wrong and I'm happy to adjust things).

What if a test really needs a random string? I would feel better if the function were renamed pseudorandomTestString() or similar in order to convey there is a difference.

+1 for this change in general for the same reasons as in the original issue. However, we need test coverage.

I also wonder if we should randomize the positions of these two special characters more, rather than sticking them together always? To @cilefen's point.

Writing tests.

It looks like this already gets tested?

\Drupal\Tests\simpletest\Unit\TestBaseTest::testRandomString

public function testRandomString($length) {
    $mock_test_base = $this->getMockForAbstractClass('Drupal\simpletest\TestBase');
    $string = $mock_test_base->randomString($length);
    $this->assertEquals($length, strlen($string));
    // randomString() should always include an ampersand ('&') if $length is
    // greater than 2.
    if ($length > 2) {
      $this->assertContains('&', $string);
    }
  }

However, as @xjm suggested, I wrapped ”almost-random" string with str_shuffle() making it more random :)

Patch from #4 and adding a test for Greater Than (">") on strings longer than 2 characters.

The patch looks pretty great!!

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -115,10 +115,11 @@ public function testRandomString($length) {
+    // randomString() should always include an ampersand ('&')  and a greater than ('>')
+    // if $length is greater than 2.

:( < 80 chars

Whoops. Good catch on the super long line!

I tweaked that text to be less than 80 chars per line.

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -115,10 +115,11 @@ public function testRandomString($length) {
+    // randomString() should always include an ampersand ('&')  and a ¶

trailing whitespace :( Feel free to fix that now or let the committer deal with it

1. +++ b/core/modules/simpletest/src/TestBase.php
   @@ -1424,9 +1424,9 @@ protected function settingsSet($name, $value) {
   @@ -1445,9 +1445,10 @@ public function randomString($length = 8) {
   
   @@ -1445,9 +1445,10 @@ public function randomString($length = 8) {
   -    // Remove 1 from the length to account for the ampersand character.
   -    $string = $this->getRandomGenerator()->string($length - 1, TRUE, array($this, 'randomStringValidate'));
   ...
   +    $string = $this->getRandomGenerator()->string($length - 2, TRUE, array($this, 'randomStringValidate'));
   +    return substr_replace($string, '>&', $replacement_pos, 0);
   

   i'm concerned about the fact the we're severely limiting the number of possibilities for 3 letter random strings. Random strings are guaranteed to be be unique. Maybe we should increase this form 2 to 3. There is only one use of $this->randomString(3) in the code base in Drupal\views\Tests\GlossaryTest - I don't see any problems with removing the 3 from the method call there.

2. +++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
   @@ -115,10 +115,11 @@ public function testRandomString($length) {
   +    // randomString() should always include an ampersand ('&')  and a ¶
   

   errant space on the end of the line

3. BrowserTestBase::randomString needs updating too. Infact it'd be great to move randomString, randomMachineName and getRandomGenerator into a trait for BrowserTestBase and TestBase to share. We should do this in a separate issue before we make this change.

@alexpott: To clarify your first point in #19... Are you asking for the insertion of '>&' to happen on generated strings that are greater than 3 characters in length (so when $length is 1, 2, or 3, > and & aren't inserted)?

Adding a related issue which adds such a trait: #2304461: KernelTestBaseTNG™

i'm concerned about the fact the we're severely limiting the number of possibilities for 3 letter random strings. Random strings are guaranteed to be be unique. Maybe we should increase this form 2 to 3. There is only one use of $this->randomString(3) in the code base in Drupal\views\Tests\GlossaryTest - I don't see any problems with removing the 3 from the method call there.

I think that is covered by the first three lines of \Drupal\simpletest\TestBase::randomString:

public function randomString($length = 8) {
    if ($length < 3) {
      return $this->getRandomGenerator()->string($length, TRUE, array($this, 'randomStringValidate'));
    }
//...
}

#19.2: Fixed
#19.3: I suggest to create separate issue if there's even an related issue (as @dawehner pointed out in #21). The original report about avoiding random test fails moves forward having such traits or not.

public function randomString($length = 8) {
    if ($length < 3) {

Now we're adding both & and > I think we need to raise this to 4.

bumped $length to 4 instead of 3.

changed status.

We need the full patch.

the full patch + interdiff

+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -115,10 +115,11 @@ public function testRandomString($length) {
     if ($length > 2) {

We need to change this test from ”greater than 2" to "greater than 3" and the comments too..

created a patch as per iMiksu's comments

updated comment

first try -> http://lastgif.com/gifs/1022.gif

Committed/pushed to 8.0.x, thanks!