Problem/Motivation

#2919373: Prevent Settings Tray functionality for blocks that have configuration overrides introduced a test that introduced test failures on PHP 5 & PostgreSQL 9.1/SQLite 3.8:
https://www.drupal.org/pift-ci-job/864394
https://www.drupal.org/pift-ci-job/864395
https://www.drupal.org/pift-ci-job/865205
https://www.drupal.org/pift-ci-job/866984
https://www.drupal.org/pift-ci-job/866991

These failures are actually coming from Quick Edit ajax calls. Probably no other test actually is having these

Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testOverriddenConfigurationRemoved
Zumba\GastonJS\Exception\JavascriptError: One or more errors were raised in the Javascript code on the page.
If you don't care about these errors, you can ignore them by
setting js_errors: false in your Poltergeist configuration (see documentation for details).
AjaxError:
An AJAX HTTP request terminated abnormally.
Debugging information follows.
Path: /subdirectory/quickedit/attachments
StatusText: error
ResponseText:
ReadyState: 0
AjaxError:
An AJAX HTTP request terminated abnormally.
Debugging information follows.
Path: /subdirectory/quickedit/attachments
StatusText: error
ResponseText:
ReadyState: 0
at http://php-apache-jenkins-php5-5-postgres9-1-2809/subdirectory/core/misc... in error
at http://php-apache-jenkins-php5-5-postgres9-1-2809/subdirectory/core/misc... in complete
at http://php-apache-jenkins-php5-5-postgres9-1-2809/subdirectory/core/asse... in i
at http://php-apache-jenkins-php5-5-postgres9-1-2809/subdirectory/core/asse... in fireWith
at http://php-apache-jenkins-php5-5-postgres9-1-2809/subdirectory/core/asse... in A
at http://php-apache-jenkins-php5-5-postgres9-1-2809/subdirectory/core/asse...
at :0 in openUrl
at phantomjs://platform/webpage.js:281 in open
at phantomjs://code/web_page.js:71
at phantomjs://code/browser.js:202 in visit
at phantomjs://code/browser.js:1272 in serverRunCommand
at phantomjs://code/poltergeist.js:38 in serverRunCommand
at phantomjs://code/server.js:76 in handleRequest
at phantomjs://code/server.js:22

/var/www/html/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserBase.php:119
/var/www/html/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserBase.php:99
/var/www/html/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserNavigateTrait.php:19
/var/www/html/vendor/jcalderonzumba/mink-phantomjs-driver/src/NavigationTrait.php:15
/var/www/html/vendor/behat/mink/src/Session.php:148
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:682
/var/www/html/core/modules/system/tests/src/FunctionalJavascript/OffCanvasTestBase.php:16
/var/www/html/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php:666

FAILURES!
Tests: 26, Assertions: 733, Errors: 1.

Proposed resolution

We need to actually test Quick edit separately so that if other modules want to test with Quick Edit on they don't have to worry that there will be problems.
#2938308: Add QuickEdit Javascript tests

For now though there is no reason for Quick Edit to enabled except on \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testQuickEditLinks

Remaining tasks

Write patch

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#13 2938309-13-8_4.patch2.22 KBtedbow
#3 2938309-3.patch2.2 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Title: Remove Quick edit when not need from Settings Tray tests » Do not install Quick edit when not needed from Settings Tray tests
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review
FileSize
2938309-3.patch2.2 KB

So Quick edit is needed for 2 tests

  1. \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testQuickEditLinks()
  2. \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testCustomBlockLinks()

Here is a patch that enables it and grants permission just for those tests.

Adding test runs for all combos against 8.6.x but also 8.5.x test with PHP 5.5 & PostgreSQL 9.1, which specifically failed

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Do not install Quick edit when not needed from Settings Tray tests » Only install Quick Edit when necessary for Settings Tray tests
Status: Needs review » Reviewed & tested by the community

Pretty crazy that this only happens on Postgres … but:

  1. +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
@@ -294,6 +293,8 @@ protected function openBlockForm($block_selector, $contextual_link_container = '
+    $this->container->get('module_installer')->install(['quickedit']);

@@ -511,6 +512,8 @@ protected function createBlockContentType($label, $create_body = FALSE) {
+    $this->container->get('module_installer')->install(['quickedit']);

    +1 for only installing the module when necessary. 

  2. +++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
@@ -294,6 +293,8 @@ protected function openBlockForm($block_selector, $contextual_link_container = '
+    $this->grantPermissions(Role::load(RoleInterface::AUTHENTICATED_ID), ['access in-place editing']);

@@ -511,6 +512,8 @@ protected function createBlockContentType($label, $create_body = FALSE) {
+    $this->grantPermissions(Role::load(RoleInterface::AUTHENTICATED_ID), ['access in-place editing']);

    And granting the permission only when necessary.

tacituseu’s picture

tacituseu CreditAttribution: tacituseu commented
Issue summary: View changes

It is the slowest environment, timing sensitive things tend to trip there first, adding another case on PHP 5.6. Might be caused by CSS animations (see: #2828528-96: Add Quick Edit Functional JS test coverage, #2901626: CSS animations cause \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest random fails).

tacituseu’s picture

tacituseu CreditAttribution: tacituseu commented
Issue summary: View changes

Also failing on PHP 5 & SQLite 3.8

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs review

Should we split that out to a separate test class so we can put quickedit back into $modules[] just in that class?

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

@catch we could but that would involve also creating new base test class that both the existing test class and the new one would extend because the new test class would need some of the protected methods in SettingsTrayBlockFormTest

I started the patch and at least these would need to be moved to the base test class.

  1. assertEditModeDisabled
  2. assertEditModeEnabled
  3. assertOffCanvasBlockFormIsValid
  4. disableEditMode
  5. enableEditMode
  6. getTestThemes
  7. openBlockForm
  8. pressToolbarEditButton

It seems very disruptive to fix this random failing test.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

re #8 I could refactor this into 2 test classes and base but I was thinking it would be much easier to review if we didn't do that.

But should I make follow up to split SettingsTrayBlockFormTest into multiple class because now that I look at it we have other methods like testOverriddenBlock and testOverriddenConfigurationRemoved that are the only methods that need the test module settings_tray_override_test. This conceptually also might better under a new class SettingTrayConfigurationOverrideTest

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Priority: Normal » Critical

It think these random failures are suppose to be critical?

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

Re #9 I have created a follow up that splits the test class so other modules also don't have to be installed when they aren't needed

I have already created the patch for that issue so it isn't forgotten after this issue is committed.

@catch does that address your concerns in #3?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Reviewed & tested by the community

OK that's more than I'd anticipated, since this is critical the follow-up is a good plan.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
2938309-13-8_4.patch2.22 KB

Here is backport to 8.4.x. version to mitigate potential fails in testing there as well, since this is a critical.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2938309-13-8_4.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Reviewed & tested by the community

sorry my 8.4.x patch kicked the issue to Needs Work

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

I queued an 8.4.x run for the backport as well.

  • xjm committed e1a7b35 on 8.6.x 
    Issue #2938309 by tedbow, tacituseu, Wim Leers: Only install Quick Edit...

  • xjm committed a03daf0 on 8.5.x 
    Issue #2938309 by tedbow, tacituseu, Wim Leers: Only install Quick Edit...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.6.x-dev » 8.4.x-dev

I think this is sufficient to mitigate the high fail rate we're seeing in HEAD, given #2939814: Split SettingsTrayBlockFormTest into multiple class to only enable modules when needed as the followup. We might still see some fails on those two methods? But let's work around what we can for now.

Committed and pushed to 8.5.x. Leaving at RTBC against 8.4.x while the test run finishes.

  • xjm committed 227b9fa on 8.4.x 
    Issue #2938309 by tedbow, tacituseu, xjm, Wim Leers: Only install Quick...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

And, committed the backport to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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