Fixed Random test failures with PathPatternBulkUpdateTestCase

[klonos]

This is part of the #1478 meta.

The 2 failures that we are usually getting are:

• "Generated 20 URL aliases." found path_pattern.test:929
• "Generated 1 URL alias." found path_pattern.test:943

I've had enough with this 😅 ...so I took a look at the code, and have noticed some inconsistencies in it. Perhaps addressing these inconsistencies will also fix these failures.

[klonos]

PR up for review: https://github.com/backdrop/backdrop/pull/3483

I believe that the main culprit was this: https://github.com/backdrop/backdrop/blob/1.x/core/modules/path/tests/path_pattern.test#L942

    // Create three tags terms.
    $this->terms = array();
    for ($i = 1; $i <= 3; $i++) {
      $term = entity_create('taxonomy_term', array(
        'name' => $this->randomName(),
        'vocabulary' => 'tags',
        'langcode' => LANGUAGE_NONE,
      ));
      taxonomy_term_save($term);
      $this->terms[$term->tid] = $term;
      // Test that terms have proper aliases.
      $this->assertEntityAlias('term', $term, 'tags/' . strtolower($term->name));
    }
    // Create three bulk_test terms.
    $this->terms = array();
    for ($i = 1; $i <= 3; $i++) {
      $term = entity_create('taxonomy_term', array(
        'name' => $this->randomName(),
        'vocabulary' => 'bulk_test',
        'langcode' => LANGUAGE_NONE,
      ));
      taxonomy_term_save($term);
      $this->terms[$term->tid] = $term;
      // Test that terms have proper aliases.
      $this->assertEntityAlias('term', $term, 'bulk-test/' . strtolower($term->name));
    }

Notice how there is a second $this->terms = array(); after creating the first set of 3 terms for the Tags vocabulary? This means that the initial array is reset/emptied, so the terms contained in $this->terms end up being 3 instead of 6. So later in the test, when we check:

    // Bulk create aliases.
    $edit = array(
      'update[node][base]' => TRUE,
      'update[taxonomy_term][base]' => TRUE,
      'update[user][base]' => TRUE,
      'operations[operation]' => 'generate',
    );
    $this->backdropPost('admin/config/urls/path/bulk-update', $edit, t('Execute'));
    $this->assertText('Generated 20 URL aliases.'); // 12 nodes + 6 terms + 2 users

...it won't actually be 6 term aliases as expected/intended - the total will be 17 instead of 20 new aliases generated.

Then the test goes on to add a new node:

    // Add a new node.
    $new_node = $this->backdropCreateNode(array('path' => array('alias' => '', 'auto' => FALSE)));
    $this->nodes[$new_node->nid] = $new_node;

...and then bulk-updates all aliases:

    // Run the update again which should only run against the new node.
    $this->backdropPost('admin/config/urls/path/bulk-update', $edit, t('Execute'));
    $this->assertText('Generated 1 URL alias.'); // 1 node + 0 users

It expects only a single alias to be generated (for the newly-created node), but the 3 terms from the tags vocabulary don't have any alias, so there will be another 3 aliases generated - not 1.

[klonos]

...the PR to fix the random failures could be as simple as removing that extra $this->terms = array();, but there are other things wrong with that test.

1. This test uses protected $profile = 'standard';, which by default has a path pattern defined for the post content type. So this bit of code is not needed:

   // Add a path pattern on the post content type.
   $edit = array(
     'path_pattern' => 'post/[node:title]',
   );
   $this->backdropPost('admin/structure/types/manage/post/configure', $edit, t('Save content type'));

   ...it seems to be a remnant of the past, so I've removed it.

2. This bit doesn't check that the user aliases are also deleted:

   // Clear out all aliases.
   $this->deleteAllAliases();
   // Test that aliases were deleted.
   foreach ($this->nodes as $node) {
     $this->assertNoEntityAliasExists('node', $node);
   }
   foreach ($this->terms as $term) {
     $this->assertNoEntityAliasExists('term', $term);
   }

   ...I've added that check in the PR.

3. This bit only checks that node aliases and the alias for the test user have been created:

   // Check that aliases have actually been created.
   foreach ($this->nodes as $node) {
     $this->assertEntityAliasExists('node', $node);
   }
   $this->assertEntityAliasExists('user', $this->admin_user);

   ...I've added a check for terms + included user1, to make sure that an alias has been created for it as well.

4. This bit resets the path pattern for posts, and then updates aliases for both posts as well as pages:

   // Test resetting aliases. Change the path pattern on the post content type.
   $edit = array(
     'path_pattern' => 'changed-post/[node:title]',
   );
   $this->backdropPost('admin/structure/types/manage/post/configure', $edit, t('Save content type'));
   $edit = array(
     'update[node][page]' => TRUE,
     'update[node][post]' => TRUE,
     'operations[operation]' => 'update',
   );
   $this->backdropPost('admin/config/urls/path/bulk-update', $edit, t('Execute'));

   ...and straight after that, it expects the page aliases to not be there, which is wrong (because the standard profile has path patterns defined for pages, as well as a fallback pattern for any content type):

   foreach ($this->nodes as $node) {
     if ($node->type == 'page') {
       $this->assertNoEntityAliasExists('node', $node);
     }
     if ($node->type == 'post') {
       $this->assertEntityAlias('node', $node, 'changed-post/' . strtolower($node->title));
     }
   }

   ...the 5 post aliases should be updated, but the 5 page aliases should be left untouched.

5. This bit deletes post and tag aliases:

   // Test deletion of individual types by deleting only post and tags aliases.
   $edit = array(
     'update[node][post]' => TRUE,
     'update[taxonomy_term][tags]' => TRUE,
     'operations[operation]' => 'delete',
   );
   $this->backdropPost('admin/config/urls/path/bulk-update', $edit, t('Execute'));
   $this->backdropPost(NULL, array(), t('Delete URL aliases'));

   ...and straight after that, it expects ALL node aliases to be gone, which is wrong:

   // Check that all node aliases are again deleted.
   foreach ($this->nodes as $node) {
     $this->assertNoEntityAliasExists('node', $node);
   }
   // Check only tags aliases were deleted.
   foreach ($this->terms as $term) {
     if ($term->vocabulary == 'tags') {
       $this->assertNoEntityAliasExists('taxonomy_term', $term);
     }
     if ($term->vocabulary == 'bulk_test') {
       $this->assertEntityAliasExists('taxonomy_term', $term);
     }
   }

[indigoxela]

It seems, this issue has stalled. No update since January, and the PR isn't ready yet. It has failing tests. Also, there's both, label "milestone candidate" and a milestone set, which is contradictory. Removing milestone.

[indigoxela]

@klonos besides your code oddness findings... do you have steps to reproduce the problem locally?

[quicksketch]

I rebased @klonos's PR and pushed up a new one at https://github.com/backdrop/backdrop/pull/3733 (I closed the previous PR).

One of the changes was incorrect.

...and straight after that, it expects ALL node aliases to be gone, which is wrong

All node aliases actually should have been removed at this point, since only post aliases were regenerated. I removed the failing checks but left the code comments in place to help clarify the situation.

To also increase the robustness of the tests slightly I added a few lines at the beginning of the test to delete all default nodes (the sample page and post) that come with the standard profile, so this test won't break in the future if we add more sample content.

[quicksketch]

I still have a few random failures happening. I begin to wonder if there is something specific to Batch API that causes random test failures. This PathPatternBulkUpdateTestCase, the BatchProcessingTestCase, ConfigurationUITest, and InstallerBrowserAdministrationTestCase all use Batch API and all intermittently fail.

I tried combing batch.inc for any missed patches from Drupal, but seems we're all up-to-date with the exception of a different implementation of https://www.drupal.org/i/3084945. I tried pulling over that approach to see if it would help, but it seems unrelated to our particular problems.

[indigoxela]

So cool, many thanks @quicksketch for digging into this! It seems, you found the right approach (which I don't understand yet).

Time to pop the corks? :champagne:

[klonos]

🎉 🍾 🎉 🍾 🎉 🍾 🎉 🍾 🎉 🍾 🎉 🍾 🎉 🍾 🎉 🍾 🎉 🍾 🎉 🍾 YAY!!!

I rebased @klonos's PR...

I had actually rebased a few hrs earlier:

...and as you can see, I had started to try a series of things. Here's what I had typed here before heading for a nap:

@indigoxela I've rebased, and the GHA tests started to look better (still not passing) after I fixed another 2 flaws in the test logic:

1. We were not accounting for the 1 post node and 1 page node that come by default OOTB with the standard installation profile (which is what the test is using). The test was expecting and testing for 5 pages + 5 posts, whereas there would actually be 6 pages + 6 posts.
2. At some point, we are creating a single new node, using backdropCreateNode() without passing any argument for its type. In that case, BackdropWebTestCase::backdropCreateNode() is set to create the node with 'type' => 'page' by default. So instead of 6 pages + 6 posts we actually have 7 pages + 6 posts. I have now moved this bit at the very end of the test (after the check for 6 pages + 6 posts).

...do you have steps to reproduce the problem locally?

You can manually follow the logic/steps of the test, and as I mentioned in https://github.com/backdrop/backdrop-issues/issues/4862#issuecomment-755385814 you will see that there are 3 terms created instead of the 6 that the test assumes later (because the array that holds the terms to be created is being reset after creating the first 3 terms).

But @quicksketch beat me to it! ❤️

PS: Fun fact: there's this Greek saying: "κοιμάσαι κι η τύχη σου δουλεύει", which loosely translates to "your luck was plotting while you were sleeping", which is exactly how I feel about this now! 😄

[indigoxela]

Actually - IMO it's not odd that this test redundantly failed - the real oddness is that it passed so often. :stuck_out_tongue_winking_eye: In general, I think the increased frequency of random failures in GHA is caused by the changed test run order (fractions vs. all-in-one run). Which in turn helps to discover test (and in some cases functionality) flaws.

PS: "κοιμάσαι κι η τύχη σου δουλεύει" has a corresponding German saying: "Den Seinen gibt's der Herr im Schlaf".

[quicksketch]

Thanks for opening https://github.com/backdrop/backdrop-issues/issues/5205! I think I have the solution! I'll post a response over there.

I think the changes @klonos made for this issue are still helpful, even though I don't think they are the source of the problem. We can perhaps adapt this PR to just be clean-up if https://github.com/backdrop/backdrop-issues/issues/5205 is successful.

[quicksketch]

I reworked this PR to remove the temporary fix I found. The "real" fix for this issue is now included in https://github.com/backdrop/backdrop-issues/issues/5205.

The new PR at https://github.com/backdrop/backdrop/pull/3733 is now just clean-up.

[quicksketch]

Since https://github.com/backdrop/backdrop/pull/3733 is now only clean-up (reformatting, preventing notices, scoping variables) and it's still passing all tests I went ahead and merged this so we can close out this issue. We haven't had the actual test fail since #5205 was fixed.