Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
As can be seen here: https://www.drupal.org/pift-ci-job/2306059 after #3260707: QuickEditJavascriptTestBase must be declared abstract will be committed, test Drupal\Tests\quickedit\FunctionalJavascript\SettingsTrayIntegrationTest::testQuickEditLinks will fail.
Interestingly it will fail on Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayTestBase::openBlockForm which was green when the same quickedit tests were triggered from the Core module.
There was 1 error:
1) Drupal\Tests\quickedit\FunctionalJavascript\SettingsTrayIntegrationTest::testQuickEditLinks
WebDriver\Exception\UnknownError: unknown error: Element ... is not clickable at point (504, 79). Other element would receive the click: ...
(Session info: headless chrome=74.0.3729.157)
(Driver info: chromedriver=2.38.552522 (437e6fbedfa8762dec75e2c5b3ddb86763dc9dcb),platform=Linux 4.9.0-0.bpo.6-amd64 x86_64)
/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/Exception.php:156
/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/AbstractWebDriver.php:165
/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/AbstractWebDriver.php:245
/var/www/html/vendor/instaclick/php-webdriver/lib/WebDriver/Container.php:239
/var/www/html/vendor/behat/mink-selenium2-driver/src/Selenium2Driver.php:806
/var/www/html/vendor/behat/mink-selenium2-driver/src/Selenium2Driver.php:792
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php:146
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php:205
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php:157
/var/www/html/vendor/behat/mink/src/Element/NodeElement.php:153
/var/www/html/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayTestBase.php:54
/var/www/html/modules/contrib/quickedit-quickedit/tests/src/FunctionalJavascript/SettingsTrayIntegrationTest.php:107
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726
Test passes green on my local install and looking at the above error it seems like this is a "timing" issue. Looks like we're trying to click on something that's not ready. It _was_ ready when triggering the same test from the Core quickedit module, possible due to multiple FunctionalJavascriptTests running simultaneously slowing things down just enough.
This seems hard to debug since it seems to require a fix in the core test Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayTestBase::openBlockForm but triggered from this Contrib module quickedit.
Personally I have no idea how to alter core for a test-run in Contrib.
Steps to reproduce
Run test Drupal\Tests\quickedit\FunctionalJavascript\SettingsTrayIntegrationTest::testQuickEditLinks
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 3261254-17.patch | 1.99 KB | dww |
| #7 | CLICK_CLUNK-1643558738.jpg | 57.61 KB | spokje |
Issue fork quickedit-3261254
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
spokjeComment #3
spokjePostponing on #3260707: QuickEditJavascriptTestBase must be declared abstract.
Since that issue has priority Major, I think this one should mirror that.
Comment #4
spokjeBumping to Priority
Criticalsince this is blocking Critical issue #3261096: Include quickedit plugins from editor and image modulesComment #5
dwwBlocker is in, back to active.
Thanks for the assessment on the failure, @Spokje! Yeah, super annoying that this passed as part of core and fails now. Also, I thought #3032275: Create a fault-tolerant method for interacting with links and fields in Javascript tests was supposed to fix these sorts of race conditions in JS tests consistently.
I don't have time to look more closely today. Possibly later this weekend, or more likely early next week. If anyone can bring this forward before then, huzzah!
Thanksl
-Derek
Comment #7
spokjeSo here's our problem:
The "Page One-thingy" slightly overlaps the region where the click event should happen.
Apparently enough to make it unclickable.
The magic mentioned by @dww in #5 can't prevent this from happening, since that basically waits for 10 seconds after the first click fails and hopes the blocking element is gone by then. In this case that element is here to stay.
Why this doesn't happen when we run the exact same test as part of Core instead of Contrib: Not A Clue!
Comment #9
spokjeLet's move the whole "block-to-check" in to the header region to make sure there's enough space between it and the node with its "Page One-thingy".
EDIT: I also added a dataProvider for the themes that are used to test
testQuickEditLinks, in the previous test a test failure using any of them would not report in which theme the failure occurred.This is in MR!5.
Comment #11
spokjeComment #12
spokjeIf we're _only_ after fixing the tests, here's a patch with just the "Move-The-Bloody-Block-To-The-Header"-bit.
Comment #14
spokjeRight...
Let's forget about the patch then, I have _no_ clue why that won't work (and no interest to put more time into it to try and find out).
IMHO this test is just very, very brittle.
Comment #15
catchIf everything blocking a 1.0.0 release is ready except this test failing, I would consider just marking this test skipped and keeping this issue open to track it.
Comment #16
dwwSweet, thanks for tracking that down, @Spokje! I'd merge this as-is, but I'm sad that you introduced a `@dataProvider` for the test as scope creep from the task at hand. Using those in
FunctionalJavascripttests costs so many wasted cycles, energy, C02, and $$, that I really don't want to do that. So I'm going to revert those parts of the MR and make sure this still works locally. 😉 Stay tuned...Thanks again,
-Derek
Comment #17
dwwThe MR branch passes locally for me. *Vastly* simpler diff to review now, too. ;) We could revert some other out-of-scope whitespace fixes, but screw it. Let's see what the bot says. Here's the current MR diff in patch form (but it doesn't need testing since the bot is working on the MR for that).
Comment #19
dwwBot's happy, merged! 🎉 Thanks again, @Spokje.