Determine cause and fix random fail in \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks

Ok here is test to see if #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways cause the random failure.

2 patches that will run \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks() only 30x. 1 reverts #2830882 also

I think I might have found the random fail point

 if (isset($new_page_text)) {
        $page->pressButton($button_text);
        // Make sure the changes are present.
        $new_page_text_locator = "$block_selector $label_selector:contains($new_page_text)";
        $this->assertElementVisibleAfterWait('css', $new_page_text_locator);
        // The page is loaded with the new change but make sure page is
        // completely loaded.
        $this->assertPageLoadComplete();
      }

The new part is assertPageLoadComplete(). Basically it makes sure all contextual links have been loaded.
I think what was happening:

1. $page->pressButton($button_text); Submitted the form which did an ajax submit and ajax redirect
2. $this->assertElementVisibleAfterWait('css', $new_page_text_locator); assert the page was redirected. But it is possible the page was not completely ready. Contextual links could have not finished loading. Because when the page gets loaded the [data-contextual-id] elements are empty and then contextual js loads the links either through ajax call or local storage.

So we need to actually wait for all contextual links to be loaded.

I also added assertPageLoadComplete to the overridden version of drupalGet() because this should happen everytime the page is loaded not just redirects.

But also added

 // If setup() is complete then all all page loads need extra checks.
    if ($this->setupComplete) {
      $this->assertPageLoadComplete();;
    }

Because in setup() when logging in this won't be true.

I was going to try to figure out how I could emulate slow testbots to make the random error happen all the time which I think would prove the fix.

But then I realized the slow testbots could just slowing down Drupal response which cause this error.

So these 2 patches simply add a .25 second sleep to index.php.

I am not running the whole test suite because I just want to prove the .25 second pause will make HEAD fail consistently on OutsideInBlockFormTest and that the fix in #5 passes even with the pause.

This is behavior I am seeing locally when I run these tests.

So 1 patch has the fix and1 doesn't. Running OutsideInBlockFormTest 5x.

1. 😱 Actually leaving that there was mistake and actually my patch fails without it. So the contextual links aren't the problem but I think they should be added to the final patch because I still see them as a possible cause for random failure. I will remove this from the next patch just to prove the other fix that will come fixes it without it though.

2. I think that will not work it the JS condition needs to return a TRUE.

After fixing #8.1 I looked for another possible problem.

Looking that error was happening in the random fails and with patch 2902191-6-slow_drupal_no_fix.patch I realized that OutsideInBlockFormTest::testBlocks() actually loops through all core themes except Seven and I didn't know which theme was causing the error. @see

foreach ($this->getTestThemes() as $theme) {
    $this->enableTheme($theme);

So put a simple print $theme line in the test locally to determine which theme was breaking it. It turned out to be Stark.

But then I tried

foreach (['stark', 'bartik'] as $theme) {

And then it failed on Bartik. Basically it wasn't failing because of Stark or Bartik it always failed on the 2nd theme in the loop.

So I looked at this code chunk again where it failed.

switch ($block_plugin) {
        case 'system_powered_by_block':
          // Confirm "Display Title" is not checked.
          $web_assert->checkboxNotChecked('settings[label_display]');
          // Confirm Title is not visible.
          $this->assertEquals($this->isLabelInputVisible(), FALSE, 'Label is not visible');
          $page->checkField('settings[label_display]');
          $this->assertEquals($this->isLabelInputVisible(), TRUE, 'Label is visible');
          // Fill out form, save the form.
          $page->fillField('settings[label]', $new_page_text);

          break;

        case 'system_branding_block':
          // Fill out form, save the form.
          $page->fillField('settings[site_information][site_name]', $new_page_text);
          break;
      }

      if (isset($new_page_text)) {
        $page->pressButton($button_text);
        // Make sure the changes are present.
        $new_page_text_locator = "$block_selector $label_selector:contains($new_page_text)";
        $this->assertElementVisibleAfterWait('css', $new_page_text_locator);
        $web_assert->assertWaitOnAjaxRequest();
      }

      $this->openBlockForm($block_selector);

The test was failing when $block_plugin == 'system_branding_block'.
So what happens in that case is the Branding blocks is opened the site name is changed and form is submitted.

Then the intention of this line
$this->assertElementVisibleAfterWait('css', $new_page_text_locator);
is to wait for the new text, the site name, from the form submit to take effect on the site. That should mean that the form has been saved and the page has been redirected.

So for the first time through this is true but for the second time through the site name has already been changed to $new_page_text!
So essentially after the first iteration of the loop there is not a wait after the form is submitted which means the next line
$this->openBlockForm($block_selector);
could happen either before the ajax redirect. This seems to be happening when testbots are slow.

So the quick fix to this problem is to use a random site name in every iteration of the loop. That way it will truly wait for the form to be submitted and the new site name to be on the page.

This patch provides that fix without the previous fixes. I still think the previous fixes could have also been the cause of the problem under different timing conditions. We should not interact with the page until all the contextual links are loaded.

I will though provide another patch after this to hopefully make the test less confusing..........

So #11 passes it and we could use that fix.

I think though the fact that it took me this long to figure out that loop would cause problems with the site name and nobody else has ever found this potential problem when review this test points to the fact that looping and doing the same sequence for each theme in the same method is confusing for this method because is complex user interaction.

So this patch removes the loop and adds $theme parameter to the testBlocks() function. It then used getTestThemes() in providerTestBlocks() to provide a test case for every block and theme combination. This changes the test cases from 3 to 15.

For me running locally this changes running the testBlocks() only from 3.41 minutes(slow I know running in VM) to 7.4 minutes. So not 5x as long because of test setup I guess.

For me the simpler test is more important than the faster test time. I also think before this patch is finished we should add back changes in #6 because assertPageLoadComplete() still could help stop other random failures.

UPDATE: Actually Test time difference
2902191-12-slow_drupal-no_loop.patch

12:30:53 Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockF 16 passes
12:35:43 Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockF 16 passes
12:40:32 Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockF 16 passes
12:45:23 Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockF 16 passes
12:50:13 Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockF 16 passes
12:50:13
12:50:13 Test run duration: 24 min 11 sec

2902191-10-slow-random_site_name.patch

10:54:53 Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockF 7 passes
10:58:43 Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockF 7 passes
11:02:33 Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockF 7 passes
11:06:24 Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockF 7 passes
11:10:13 Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockF 7 passes
11:10:13
11:10:14 Test run duration: 19 min 12 sec

Adding back the changes from #9 that waits for all contextual links to be loaded on page loaded. These changes don't fix the current random failure but it is an extra check that hopefully will avoid unknown random failures in the future.

Important This still has the usleep() call in index.php to simulate slow testbots and only runs OutsideInBlockFormTest

I removed the var $setupComplete because it confusing.

All I had to do was check for the permission access contextual links before calling assertAllContextualLinksLoaded()

I think this is clearer.

This includes a "slow_drupal" patch with the usleep() command and the runs the 5x times.

2902191-14.patch removes these changes and would be commit ready if RTBC'ed

Here is a no whitespace patch for #14 to make it easier to review because removing the loop adds a bunch of lines to the patch that are just indentation changes.

Changing to "major"

If we look at 2902191-6-slow_drupal_no_fix.patch in #6 this patch demonstrates in a "slow drupal" situation, simulated by add like usleep to index.ph might happen in slow testbot HEAD will still likely fail.

So I think this is "major" in the sense the sooner or later if not fixed it will almost certainly break head.

Removing text I meant to add to #14

@vaplas thanks for the review!
#18
1. yep
2. Good point
3. Added a comment
4. Added a @todo about existing issue #2821724: Create Javascript Tests for Contextual Links
5. fixed
6. added "typeof jQuery !== 'undefined'" and made all 1 condition.
#19 Thanks for making #2902968: How many random fails due to slow bots? look like this is the only test that fail 😅
#20 Yes will make debugging easier.

@vaplas thanks for finishing this up. It looks good to me for RTBC. But since I did most of the patch I shouldn't RTBC.

@vaplas since you provided most of the reviews and your patch on #23 was just 1 character difference I think you could RTBC, if you think it is good.

@vaplas thanks for RTBC! Patch needed reroll. Here it is

I meant to upload the "no_whitespace" file as a ".txt" so the tests would not run

@vaplas good catch yes that was a mistake.
Also fixing the keys as you suggested.

Changing to new settings_tray.module component. @drpal thanks for script help!

@vaplas thanks for the rerolls!!

Setting back to RTBC since it has just been rerolls since it was RTBC confirm the reroll in #40

@droplet #34 that could be problem but this problem because of where it is failing in SettingsTrayBlockFormTest.php:130(seen recently here https://www.drupal.org/pift-ci-job/763335) is because of the problem described in #11 because the site name text already being on the page. waitForOffCanvasToOpen() is called 7 times directly and openBlockForm() which contains it is called 7 times also but the random fail has been consistent on SettingsTrayBlockFormTest.php:130 so I think we should commit this issue before worrying about other problems.

@catch thanks for committing and thanks everyone who worked on this issue.

Hopefully we just avoided HEAD breaking in a way this test has done in past and causing a major pain for everyone!