Resolve random failure in LayoutBuilderTest so that it can be added to HEAD

Seems to be in testing contextual links. Tomorrow I can try chopping up the test instead of having such a single long running method.

Otherwise I'm out of ideas

Stepped through this logic with @Wim Leers, and I think we found the issue.

The custom override for clickContextualLink() is needed because this is the only place in core where the result of clicking on a contextual link doesn't leave the page (this uses the Off Canvas dialog and AjaxResponse to stay on the page).

The logic as written in HEAD:

1. Force the "pencil icon" portion of the contextual link to be visible.
2. Attempt to find the link within the page (just that the markup is present)
3. If the link does not exist, fail
4. If the link is not visible, press the "pencil icon" to show the list of links
5. Wait for the link to be present on the page
6. If the link is still not visible, fail
7. Otherwise, click the link

Step 6 is where the fail is.
Step 5 is where our bug is.

The waitForLink() is pointless, since the link already exists on the page.
What we need is waitForVisibleLink(), which doesn't exist.
Instead I combined the innards of waitForLink within the call of a waitForVisibleElement.

We need to revert the Layout Builder commit for this unfortunately. This is failing at a significant rate in HEAD which will cause serious disruption for other issues. I've done so.

This would be critical otherwise, but we can rescope it to fix and add back this test after Layout Builder (as an alpha module) is committed without it.

@vaplas++!
I think you're onto something.

The module was recommitted without the test, so here's the original test but with interdiff-press applied.

Thanks @vaplas! I think we're good.

Can we post an interdiff from what was originally committed to this patch, and a multi-run run-tests.sh hack patch with the intended fix, queued a number of times? Every patch here that did have multiple runs failed on the test at least once.

This is major, since it would be a testing gate blocker for a major feature.

negatively affected my memory too

😂😂😂

Some of the routing has changed, hence the above fail.
Here it is adjusted for those changes.

Ah yes, that was the other change now that #2922033: Use the Layout Builder for EntityViewDisplays landed.

I reviewed the patch for readability, consistency, and code style and it was difficult to find anything wrong. :) A couple very minor things:

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
@@ -0,0 +1,440 @@
+    'layout_builder',
+    'node',
+    'block_content',
+    'field_ui',
+    'layout_test',

Are these in this order for any particular reason? Imports are normally alphabetical so this order threw me off a bit.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
@@ -0,0 +1,440 @@
+        // Mink press() works unstable, so the trigger('click') is used here.

The wording seems awkward. Perhaps:

// Mink press() is unstable, so the trigger('click') is used here.

Thanks.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
@@ -22,11 +22,11 @@ class LayoutBuilderTest extends JavascriptTestBase {
-    'layout_builder',
...
+    'layout',

This caused the fail :)

This patch should add a @Todo to use #2892440: Provide helper test method to wait for an element to be removed from the page

What is the status of this now with all the chromedriver stuff and with nightwatch coming?

In #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder We have JS testing. I have put a lot of work on that issue make sure they test doesn't fail randomly.

The tests there extend \Drupal\FunctionalJavascriptTests\WebDriverTestBase which is what is suppose to be used now. NightWatch is still not ready write tests in I think.

Also related to this issue:https://www.drupal.org/drupalorg/docs/drupal-ci/customizing-drupalci-tes...
You don't have to edit run-test.sh to run single test multiple times.

Where does this stand after the move away from PhantomJS

Re #61 this patch change to use WebDriverTestBase so it won't use phantomjs
It also changes core/drupalci.yml instead of run-tests.sh changes to run in multiple times.
I realize the test I added #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder always do a page load between operation with contextual links so it might not have problems describe in this issue.

Whoops forgot to test locally. Needed to update for #2936358: Layout Builder should be opt-in per display (entity type/bundle/view mode)

We had 50 passing test runs let's create a real patch now.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
@@ -0,0 +1,434 @@
+  protected $minkDefaultDriverClass = DrupalSelenium2Driver::class;

This can be removed now.

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,434 @@
   +  public static $modules = [
   

   This should be protected

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTestBase.php
   @@ -0,0 +1,34 @@
   +  public function enableLayoutsForBundle($path, $allow_custom = FALSE) {
   

   I am going to change all of the other tests that make sense to extend this class so they can use this method.

Now that we are going to have a base class for these tests it makes sense to move some of the functionality that repeats in these tests to the base class. I think if didn't have issue with the random failure we would have been using for the base class all along.

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,429 @@
   +    $this->clickLink('Add Block');
   +    $assert_session->assertWaitOnAjaxRequest();
   +
   +    $assert_session->elementExists('css', '#drupal-off-canvas');
   +
   +    $assert_session->linkExists('Powered by Drupal');
   +    $this->clickLink('Powered by Drupal');
   

   This sequence is repeated many times in this class and other tests when want to add a block and we are not always consistent about check if the links exist first.

   I am going to create openAddBlockForm() that will do this sequence given a block link text.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,429 @@
   +  protected function waitForOffCanvasForm($expected_form_id, $timeout = 10000) {
   +    $page = $this->getSession()->getPage();
   +    return $page->waitFor($timeout / 1000, function () use ($page, $expected_form_id) {
   +      // Ensure the form ID exists, is visible, and has the correct value.
   +      $form_id_element = $page->find('hidden_field_selector', ['hidden_field', 'form_id']);
   +
   +      // Ensure the off canvas dialog is visible.
   +      $off_canvas = $page->find('css', '#drupal-off-canvas');
   +      if (!$off_canvas || !$off_canvas->isVisible()) {
   +        return NULL;
   +      }
   +      return $form_id_element;
   

   We return NULL if the off-canvas dialog is not there but we don't actually check the return value ever.

   We should change this to assertOffCanvasFormAfterWait and fail if the form is not there or hidden field is not there.

Thank, for creating the proper patch. Hopefully, my review helps with 1 and 2.

1. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinkClickTrait.php
   @@ -16,20 +16,38 @@
   +      $button = $page->waitFor(10, function () use ($element) {
   

   Why are we not using waitForButton() here?

2. +++ b/core/modules/contextual/tests/src/FunctionalJavascript/ContextualLinkClickTrait.php
   @@ -16,20 +16,38 @@
   +      $link = $page->waitFor(10, function () use ($link) {
   

   Why are we not using waitForLink() here?

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,387 @@
   +    $layout_url = $this->node->toUrl()->setAbsolute()->toString() . '/layout';
   ...
   +    $layout_url = $this->node->toUrl()->setAbsolute()->toString() . '/layout';
   

   This doesn't need to be absolute.

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,387 @@
   +    $this->drupalGet($this->node->toUrl('canonical'));
   ...
   +    $this->drupalGet($this->node->toUrl('canonical'));
   ...
   +    $this->drupalGet($this->node->toUrl('canonical'));
   ...
   +    $assert_session->addressEquals($this->node->toUrl('canonical'));
   ...
   +    $assert_session->addressEquals($this->node->toUrl('canonical'));
   

   'canonical' is not needed at all.

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,387 @@
   +    $assert_session->linkExists('Two column');
   ...
   +    $assert_session->assertWaitOnAjaxRequest();
   ...
   +    $assert_session->linkExists('Add Section');
   

   These can be combined into waitForLink()

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,387 @@
   +    $assert_session->assertWaitOnAjaxRequest();
   ...
   +    $assert_session->elementExists('css', '.layout__region--second .block-system-powered-by-block');
   ...
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $assert_session->elementExists('css', '#drupal-off-canvas');
   ...
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $assert_session->elementExists('css', '#drupal-off-canvas');
   

   These can be combined into waitForElement()

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTestBase.php
   @@ -0,0 +1,83 @@
   +    $this->assertNotEmpty($assert_session->waitForElementVisible('css', "#drupal-off-canvas a:contains('$block_title')"));
   ...
   +    $this->assertNotEmpty($page->waitFor($timeout / 1000, function () use ($page, $expected_form_id) {
   

   This should be assertNotNull()

@jibran thanks for the review again.
re #68

1. We can't use waitForButton() here because we need find the need to button this with the current element.

   But actually can do this

   $button = $assert_session->waitForElementVisible('css', "$selector .contextual button");
   $this->assertNotEmpty($button);
   $button->press();
   

   Which means we change the lines above to the find the link because we don't need the $element variable anymore.

2. Because that will not check if the link is visible and also we need find the link only within $selector
3. Fixed
4. Replace 1 of these with $this->assertNotEmpty($assert_session->waitForElementVisible('named', ['link', 'Two column']));
   We don't really need to assertWaitOnAjaxRequest() if we know there is something we can check for that will be on the page after wait.

   waitForLink() won't work because it can cause random failures since the link might exist but be on the off-canvas dialog and not visible yet.

5. We waiting for the off-canvas dialog to open when have a had a lot of random failures when don't >assertWaitOnAjaxRequest() first. So I think we should leave these..

I also noticed

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTestBase.php
@@ -0,0 +1,83 @@
+    $this->assertNotEmpty($assert_session->waitForElementVisible('css', "#drupal-off-canvas a:contains('$block_title')"));

I think this more clear as
$this->assertNotEmpty($assert_session->waitForElementVisible('named', ['link', $block_title]));
The only difference would be if the $block_title also was somewhere else besides the off-canvas dialog.

Also these is a lot of changes so uploaded a x25 patch first to just run layout builder javascript tests.

1. got the test a failure

   Current page is "/node/1/layout", but "/subdirectory/node/1/layout" expected.

   This because the removing the absolute url. We can just use hardcoded "node/1/layout" like most tests do.

2. aslo

   WebDriver\Exception\UnknownError: unknown error: Element ... is not clickable at point (562, 575). Other element would receive the click: ...
   (Session info: headless chrome=62.0.3202.94)
   (Driver info: chromedriver=2.33.506092 (733a02544d189eeb751fe0d7ddca79a0ee28cce4),platform=Linux 4.9.0-0.bpo.6-amd64 x86_64)

   Which I think the off-canvas dialog being in the process of opening so there is motion problem. Adding assertWaitOnAjaxRequest() call because this also checks for jQuery(':animated').length === 0

Whoops here is the interdiff

I chatted with @tim.plunkett about another related issue and he recommended a trait instead of base class.

I think this make sense especially because other core module may need to make Layout Builder integration tests so enableLayoutsForBundle() would very useful there.

HMM. The test failure was in \Drupal\Tests\BrowserHtmlDebugTrait::initBrowserOutputFile()

if (!is_dir($this->htmlOutputDirectory)) {
   mkdir($this->htmlOutputDirectory, 0775, TRUE);
}

So thinking totally unrelated to this issue and also it only failed once so maybe general DrupalCI error

Retesting to make sure.

Hi all,

I want to chime in here because I am also seeing some random failures in my feature test suite that depends on adding blocks within layout builder.

The basic issue is this: clicking the 'Save layout' link is not reliably saving the layout.

My assertion for saving the layout was this:

$this->clickLink('Save Layout');
$assert_session->pageTextContains('The layout has been saved.');

About 1 in 10 times this would randomly fail for no apparent reason. Instead of being redirected to the the display UI page (where the confirmation message would be displayed), I would still be on the layout builder UI page.

In order to fix this is in my own test suite, I implemented a retry loop:

$retries = 3;
for ($attempt = 0; $attempt < $retries; $attempt++) {
  $this->clickLink('Save Layout');
  try {
    $assert_session->pageTextContains('The layout has been saved.');
  }
  catch (ResponseTextException $exception) {

    // The retry limit has been reached. stop retying and throw the
    // exception.
    if ($attempt === $retries - 1) {
      throw $exception;
    }

    // Try again.
    continue;
  }

  // No exception was thrown during the last attempt.
  return;
}

If I add some debugging output in the try/catch statement, I still see that failures occur (still at a rate of about 1 in 10), but thus far this has resolved my own random failures.

I hope this can shed some light on what is going on here. Is there anything that can be investigated as far as why the save does not always occur? I've only seen this in tests, if I use the UI myself I've never had a save failure.

In order to get this issue done, I'd like to hold off on any helpful traits or other refactoring.
Let's add the test as it is.

This moves all of the trait code directly into the new class and only retains the changes to core/modules/settings_tray/tests/modules/settings_tray_test_css/css/css_fix.theme.css

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,517 @@
   +    'settings_tray_test_css',
   

   We shouldn't need. This \Drupal\Tests\system\FunctionalJavascript\OffCanvasTest doesn't having anything like this.

   If we could have x30 patch that removes and proves it is good that would be good.

   If it fails I think we should duplicate the test module instead of having a settings_tray test module here. And with a @todo to remove it here https://www.drupal.org/project/drupal/issues/2901792#comment-12695082

   That issue has a patch to make a system module testing module.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,517 @@
   +  protected function clickContextualLink($selector, $link_locator, $force_visible = TRUE) {
   

   I think we should have a @todo point to https://www.drupal.org/project/drupal/issues/2918718 to remove this.

Fixed both!

Looks good 🎉

30x test runs isn't enough to prove that a random fail is resolved or not unless the test is failing like 50% of the time, so queuing multiple test runs of the 30-test patch. However, we also usually want a version of the 30-test-runs patch that does not include the fix, to prove that the number of times we ran the test is enough to repeatedly expose the fail. Otherwise a 30-run patch isn't useful. Statistics! ;)

Also, did anyone try running this locally a few hundred times?

@xjm

30x test runs isn't enough to prove that a random fail is resolved or not unless the test is failing like 50% of the time, so queuing multiple test runs of the 30-test patch.

Looks they all came back green!

However, we also usually want a version of the 30-test-runs patch that does not include the fix, to prove that the number of times we ran the test is enough to repeatedly expose the fail.

Yes but since this patch just introduces a new test that is not in 8.7.x then I am don't know what "patch that does not include the fix" would actually be. I don't think we can do that here.

Also, did anyone try running this locally a few hundred times?

I haven't but now that you ran it 11 times above that is 330 passing on Drupalci!

found 1 remaining problem though.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
index e07c95194e..89600c5b7f 100644
--- a/core/modules/settings_tray/tests/modules/settings_tray_test_css/css/css_fix.theme.css

--- a/core/modules/settings_tray/tests/modules/settings_tray_test_css/css/css_fix.theme.css
+++ b/core/modules/settings_tray/tests/modules/settings_tray_test_css/css/css_fix.theme.css

We shouldn't need the change in this file because we are no longer including this test module.

Oh, I meant to do that in #79 when I stopped loading the module in the test
New diffstat:

 core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php | 518 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 518 insertions(+)

Looks good!

#87 is totally unrelated non javascript test EntityReferenceAdminTest. Looks to be random failure

The 330 test runs still actually isn't enough without knowing the base fail rate. Say the base fail rate was 1% (which is still a pretty high fail rate). Running the test 330 times with a 1% random fail would mean there would be a (.99)^330 = 4% chance every single test run would pass even with the random fail being present. This is why I asked about running the test locally a few hundred times, to know what the base fail rate was previously and compare it to now.

Of course the best thing is always to make a version of the test that fails 100% of the time (by generating whatever random condition is causing the fail 100% of the time, whether with a wait() to slow things down, or hardcoding the random character that breaks the test, or whatever). Anyway, since it looks like the fix involves something like a CSS transition (lol?) then it'd be harder to create that 100% fail patch for something like that. Looks like that change was removed from the patch now.

Regarding x EntityReferenceAdminTest, I confirmed https://www.drupal.org/pift-ci-job/1184261 is in HEAD and has failed twice in the past two weeks. Seems to be missing a core critical issue though... Anyone want to file that for me? :P

Is the earlier version of the patch that included the random fail? We could test that the same number of times to expose the failure rate that's being resolved?

OK, reading back on the issue, the original fail rate for the test was in the 5-10% range, so at that point I'm more comfortable with 330 test runs being enough. ;)

Posted #3030902: Random fail in EntityReferenceAdminTest.

This looks good. Just some nitpicks:

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,518 @@
   +    // Drag one block from one region to another.
   +    $this->drupalGet($layout_url);
   +    $this->markCurrentPage();
   +
   +    $assert_session->linkExists('Add Section');
   +    $this->clickLink('Add Section');
   +    $this->assertNotEmpty($assert_session->waitForElementVisible('named', ['link', 'Two column']));
   +
   +    $this->clickLink('Two column');
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $page->pressButton('Add section');
   +    $assert_session->assertWaitOnAjaxRequest();
   

   The inline comment doesn't describe what's happening in the next 10 lines; can we remove it?

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,518 @@
   +    $this->assertNoElementAfterWait('.layout__region--second .block-system-powered-by-block');
   +    $assert_session->elementTextNotContains('css', '.layout__region--second', 'Powered by Drupal');
   +    // Drag the block from one layout to another.
   +    $page->find('css', '.layout__region--content .block-system-powered-by-block')->dragTo($page->find('css', '.layout__region--second'));
   

   "From one layout to another" sounds wrong. From one section to another? From one region within the same section to another?

   Also there's a missing blank line before the comment.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,518 @@
   +    $assert_session->assertWaitOnAjaxRequest();
   +    // Ensure the drag succeeded.
   

   Missing blank line before the comment again.

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,518 @@
   +    // Ensure the drag persisted after reload.
   ...
   +    // Ensure the drag persisted after save.
   

   Nitpick, but "Ensure the drag persisted" is confusing. Puns abound. How about "Ensure the dragged block is still in the correct position after reload/save"?

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,518 @@
   +    // Configure a block.
   

   I think what's imoportant here is "Reconfigure a block and ensure that the layout content is updated"?

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,518 @@
   +  /**
   +   * Tests configurable layouts.
   +   */
   +  public function testConfigurableLayouts() {
   

   This method name confused me ("Aren't they all?") but it's about configurable layout templates a.k.a. section layouts. I guess we never finalized that terminology issue, so that's why "layouts" is used here without any modifier. We could still clarify the method name.

7. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -0,0 +1,518 @@
   +   * Tests bypassing the Off Canvas dialog.
   

   Nit: "Off Canvas dialog" is wrong. It should be one of "off-canvas dialog".

@xjm thanks for the review

Fixed
Fixed
Fixed
fixed
fixed
changed to testConfigurableLayoutSections()
fixed

Thanks for the review and fixes!

Thanks, committed to 8.7.x. I also backported it to 8.6.x as a test addition; we'll see in an hour or so if that was a bad idea. :P

OK @tedbow indicated a reason it's not 8.6.x-compatible; reverted. Sorry for the emails.

Fixing credit; d.o somehow unchecked people that are included in the commit message.

This is still causing random fails. See https://www.drupal.org/pift-ci-job/1196600