ViewExecutableTest::testPropertyMethods() generates a random string that is then used to test that the ViewExecutable::getUrl() method returns a URL correctly.

When the randomly generated string begins with "%/", ends with "/%", or contains "/%/" then getUrl() treats this like a placeholder and replaces it with an asterisk. Although this seems like a random enough occurrence, I've encountered it twice in one day.

CommentFileSizeAuthor
#10 interdiff.txt574 bytespbull
#10 testPropertyMethods_random_url-2409339-10.patch979 bytespbull
#6 interdiff.txt1.24 KBpbull
#6 testPropertyMethods_random_url-2409339-6.patch1.13 KBpbull
#1 testPropertyMethods_random_url-2409339-1.patch634 bytespbull

Comments

pbull’s picture

pbull commented
Assigned: pbull » Unassigned
Status: Active » Needs review
StatusFileSize
new testPropertyMethods_random_url-2409339-1.patch634 bytes

Replace "%" with "*" to prevent randomly generated URLs from containing placeholders.

pbull’s picture

pbull commented
Title: ViewExecutableTest::testPropertyMethods fails when randomly generater URL contains placeholders » ViewExecutableTest::testPropertyMethods fails when randomly generated URL contains placeholders
dawehner’s picture

dawehner
Primary language German
commented
Priority: Minor » Critical
Issue tags: +Random test failure

Random test failures are critical.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work 
+++ b/core/modules/views/src/Tests/ViewExecutableTest.php
@@ -321,6 +321,8 @@ public function testPropertyMethods() {
     $url = $this->randomString();
+    // Prevent URL placeholders from being added.
+    $url = preg_replace('/(\/|^)%(\/|$)/', '${1}*${2}', $url);

The randomness has no part of the test. We could just use a hard coded string

anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
commented
Issue tags: +VDC
pbull’s picture

pbull commented
Status: Needs work » Needs review
StatusFileSize
new testPropertyMethods_random_url-2409339-6.patch1.13 KB
new interdiff.txt1.24 KB

The attached patch hard-codes the paths and view arguments.

(Alternately, the randomness could be retained with URL-safe strings by using TestBase::randomMachineName() in the place of TestBase::randomString() but I don't think this is necessary.)

Status: Needs review » Needs work

The last submitted patch, 6: testPropertyMethods_random_url-2409339-6.patch, failed testing.

adci_contributor queued 6: testPropertyMethods_random_url-2409339-6.patch for re-testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review

Thank you for your work on that!

+++ b/core/modules/views/src/Tests/ViewExecutableTest.php
@@ -320,19 +320,19 @@ public function testPropertyMethods() {
     // Test the title methods.
-    $title = $this->randomString();
+    $title = 'FooBar';
     $view->setTitle($title);

Is the random string for the title a problem?

pbull’s picture

pbull commented
StatusFileSize
new testPropertyMethods_random_url-2409339-10.patch979 bytes
new interdiff.txt574 bytes

@dawehner no, randomness in the titles doesn't appear to be an issue. Updated patch attached.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Great!

This looks fine for me.

alexpott’s picture

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

Committed 312aa7c and pushed to 8.0.x. Thanks!

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes.

  • alexpott committed 312aa7c on 8.0.x 
    Issue #2409339 by pbull: ViewExecutableTest::testPropertyMethods fails...

Status: Fixed » Closed (fixed)

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