Problem/Motivation

TipPluginTextTest is using the \Drupal\Component\Utility\Random::word() function to generate strings and expecting uniqueness - however that method does not guarantee that.

See https://www.drupal.org/pift-ci-job/962303

Proposed resolution

Use randomManchineName instead as that guarantees uniqueness.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#7 2972531-7.patch1.05 KBalexpott
#4 2972531-4-test-drive.patch2.09 KBAnonymous (not verified)
#4 2972531-4.patch2.25 KBAnonymous (not verified)
#4 2972531-4-data-provider-test-only.patch1.11 KBAnonymous (not verified)
#2 2972531-2.patch834 bytesalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
Issue tags: +Random test failure
StatusFileSize
new 2972531-2.patch834 bytes

Status: Needs review » Needs work

The last submitted patch, 2: 2972531-2.patch, failed testing. View results

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new 2972531-4-data-provider-test-only.patch1.11 KB
new 2972531-4.patch2.25 KB
new 2972531-4-test-drive.patch2.09 KB

--- Expected
+++ Actual
@@ @@
-'kgm8UeiZ'
+'kgm8ueiz'

Hah, it is surprising that $this->randomMachineName() returns string with upper cases too.

Also it is suprising that static names not works with dataprovider. Maybe I just do not know this mechanism, I used to think that the one iteration should not affect the other, but some kind of caching is obviously happening. Sure, it is out of scope.

Anonymous’s picture

Anonymous (not verified) commented 
+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -53,18 +53,18 @@ protected function setUp() {
-    return $this->getRandomGenerator()->name($length, TRUE);
+    return strtolower($this->getRandomGenerator()->name($length, TRUE));

Now we lose the guarantee that this is a unique name :(
Because Random::name() is case sensitive when checking for uniqueness.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -53,18 +53,18 @@ protected function setUp() {
-   * Generates a unique random string containing letters and numbers.
+   * Generates a unique random string containing lower letters and numbers.
    *
    * @param int $length
-   *   Length of random string to generate.
+   *   Length of random machine name to generate.
    *
    * @return string
-   *   Randomly generated unique string.
+   *   Randomly generated unique machine name.
    *
    * @see \Drupal\Component\Utility\Random::name()
    */
   public function randomMachineName($length = 8) {
-    return $this->getRandomGenerator()->name($length, TRUE);
+    return strtolower($this->getRandomGenerator()->name($length, TRUE));

Machine names can have uppercase. So I'm not sure about that. The only reason I used the machine name version is because of the unique guarantee.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2972531-7.patch1.05 KB

The reason we have this test is because of an incorrect static that was removed in #2918643: TipPluginText's ariaId is not unique.. So hardcoding the strings is fine. Also we should actually run this test in a separate process because of the static in \Drupal\Component\Utility\Html::getUniqueId() - using run-tests.sh every test is run in a separate process but if you use the vanilla PHPUnit runner you have no such guarantee.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community

Machine names can have uppercase.

Wow, I did not know that! Last time I experienced this effect when I realized that we can create field without "field_". Thank you for the wisdom, @alexpott!

#7 looks very solid!

The last submitted patch, 4: 2972531-4-data-provider-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Hmm... I'm wrong. By default a machine name can only contain lowercase. 'replace_pattern' => '[^a-z0-9_]+', from \Drupal\Core\Render\Element\MachineName::processMachineName(). @vaplas the change in #4 is definitely worth pursing in a different issue as I think the random machine name generated should conform to our default regex.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed df75450932 to 8.6.x and 59f1eae853 to 8.5.x. Thanks!

Committed myself because random fails are annoying and this is test only code.

  • alexpott committed df75450 on 8.6.x 
    Issue #2972531 by vaplas, alexpott: Random fail in TipPluginTextTest

  • alexpott committed 59f1eae on 8.5.x 
    Issue #2972531 by vaplas, alexpott: Random fail in TipPluginTextTest

(...
Anonymous’s picture

Anonymous (not verified) commented

Done #2972573: randomMachineName() should conform to processMachineName() pattern

Status: Fixed » Closed (fixed)

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