Add Quick Edit Functional JS test coverage

I'll be taking this on. I'm fairly far along already.

Note that Jesse Beach did write some test coverage for Quick Edit, it lives in the "Front-end Automated Testing" module that uses QUnit/Testswarm. See http://cgit.drupalcode.org/fat/tree/tests/edit.tests.js for the test coverage.

We won't be able to copy/paste that, not remotely. But we can ensure that most if not all of the assertions there will exist in the new test.

Not done yet, but pretty far along :)

I just spent half a day trying to modify the title field or body field via Quick Edit. And was *this* close to giving up.

Turns out that phantomjs's default keyboard events are not actually native, they're synthetic. And as such, they're not respected by contenteditable. This has been a long-standing issue:

1. https://github.com/detro/ghostdriver/issues/401
2. https://github.com/ariya/phantomjs/issues/11289
3. http://phantomjs.org/api/webpage/method/send-event.html -> this API is not exposed in \Zumba\Mink\Driver\PhantomJSDriver, hence it seems impossible to even emulate non-synthetic events… which means there's no hope of testing contenteditable

Eventually, I found a working work-around in http://sticksnglue.com/wordpress/phantomjs-1-9-and-keyboardevent/, combined with some hackery:

    $js_simulate_user_typing = <<<JS
function () {
  window.KeyboardEvent = function(eventString) {
    var keyboardEvent = document.createEvent('KeyboardEvent');
    keyboardEvent.initKeyboardEvent(eventString, true, true, window, 1, 0, 0);
    return keyboardEvent;
  };
  var el = document.querySelector('h1.js-quickedit-page-title > span');
  el.textContent = '$text_to_type';
  el.dispatchEvent(window.KeyboardEvent('keyup'));
}()
JS;
    $this->getSession()->evaluateScript($js_simulate_user_typing);

After quite a bit of struggling with just about everything else, because PhantomJS does not send any event natively, hence most JS doesn't actually work, I found work-arounds for everything that got in the way of testing a common scenario: fixing the title and adding a new tag, but also starting to quick edit a field without actually changing anything.

Clean-up: move the test method up higher, the assertion helpers down below.

1. Rename/refactor methods for consistency
2. Add missing docblocks

This does not capture every nuance of http://cgit.drupalcode.org/fat/tree/tests/edit.tests.js, but it gets the majority. In fact, it tests with much more confidence in most respects.

This should make it quite easy to add the test coverage that #2661880: Quick Edit doesn't work for Custom Blocks referenced by other Custom Blocks and #2466715: Displaying the same entity twice on one page doesn't work as expected need.

This should be reviewed from 2 POVs:

1. Does this set a good enough baseline of test coverage?
2. Does this provide sufficient structure/assertion methods to help future Quick Edit test coverage?

Oh, yes, definitely. I just ran out of steam.

You mean these two, right?

1. +++ b/core/modules/quickedit/tests/src/FunctionalJavaScript/QuickEditIntegrationTest.php
   @@ -0,0 +1,498 @@
   +    // @todo Clean this up once PhantomJS is fixed, see http://sticksnglue.com/wordpress/phantomjs-1-9-and-keyboardevent/.
   +    $text_to_type = $node->label() . ' Llamas are awesome!';
   +    $js_simulate_user_typing = <<<JS
   +function () {
   +  window.KeyboardEvent = function(eventString) {
   +    var keyboardEvent = document.createEvent('KeyboardEvent');
   +    keyboardEvent.initKeyboardEvent(eventString, true, true, window, 1, 0, 0);
   +    return keyboardEvent;
   +  };
   +  var el = document.querySelector('h1.js-quickedit-page-title > span');
   +  el.textContent = '$text_to_type';
   +  el.dispatchEvent(window.KeyboardEvent('keyup'));
   +}()
   +JS;
   +    $this->getSession()->evaluateScript($js_simulate_user_typing);
   

   Should be moved to typeInPlainTextEditor($css_selector, $text_to_append)

2. +++ b/core/modules/quickedit/tests/src/FunctionalJavaScript/QuickEditIntegrationTest.php
   @@ -0,0 +1,498 @@
   +    $input = $this->cssSelect('.quickedit-form-container > .quickedit-form[role="dialog"] form.quickedit-field-form input[name="field_tags[target_id]"]')[0];
   +    $input->setValue($input->getValue() . ', bar');
   +    $js_simulate_user_typing = <<<JS
   +function () {
   +  var el = document.querySelector('.quickedit-form-container > .quickedit-form[role="dialog"] form.quickedit-field-form input[name="field_tags[target_id]"]');
   +  window.jQuery(el).trigger('formUpdated');
   +}()
   +JS;
   +    $this->getSession()->evaluateScript($js_simulate_user_typing);
   

   Should be moved to typeInFormEditorTextInputField($input_name, $text_to_append)

Did the refactoring asked in #15, and described in #16+#17.

Having done that, I went back to #15 and wondered about this:

As someone who has written a lot of Quick Edit patches for contrib, I wonder if we could move more of the logic in testArticleNode() to protected methods.

What else could we move to protected methods, what else would be valuable?

1. +++ b/core/modules/quickedit/tests/src/FunctionalJavaScript/QuickEditIntegrationTest.php
   @@ -0,0 +1,526 @@
   +    // Wait for the saving of the title field to complete.
   +    $this->assertJsCondition("Drupal.quickedit.collections.fields.get('node/1/field_tags/en/full[0]').get('state') === 'candidate'");
   

   This would definitely be helpful. An awaitEntityInstanceFieldState($entity_type_id, $entity_id, $entity_instance_id, $field_name, $awaited_state) method wouldbe very helpful for this.

2. +++ b/core/modules/quickedit/tests/src/FunctionalJavaScript/QuickEditIntegrationTest.php
   @@ -0,0 +1,526 @@
   +    // Assert the "Loading…" popup appears.
   +    $this->assertSession()->elementExists('css', '.quickedit-form-container > .quickedit-form[role="dialog"] > .placeholder');
   +    // Wait for the form to load.
   +    $this->assertJsCondition('document.querySelector(\'.quickedit-form-container > .quickedit-form[role="dialog"] > .placeholder\') === null');
   

   This could be an awaitFormEditor() method.

3. +++ b/core/modules/quickedit/tests/src/FunctionalJavaScript/QuickEditIntegrationTest.php
   @@ -0,0 +1,526 @@
   +    $quickedit_entity_toolbar = $this->getSession()->getPage()->findById('quickedit-entity-toolbar');
   +    $save_button = $quickedit_entity_toolbar->find('css', 'button.action-save');
   +    $save_button->press();
   +    $this->assertSame('Saving', $save_button->getText());
   

   This could be a saveQuickEdit() method.

For example, you added a typeInPlainTextEditor() method, but a chid class would still have to write routines to check the initial field states, initiate Quick Edit on a field, wait for Quick Edit to load, click save, and assert that saving is complete.

Yes, but those things are part of the thing you want to assert anyway. And there are helpers already to do those things. It's quite likely that many tests will want to test modifying a title, so without typeInPlainTextEditor(), they'd all have to duplicate that logic.

However, you're right that it's a tough thing to balance :)

Fixed the nit in #20 and did everything in #19.

Should this update the test coverage introduced by #2421427: Improve the UX of Quick Editing single-valued image fields?

I stupidly did not yet realize I hadn't split it up in a base class and a test class yet. Did that now.

And here is then the update for the test coverage introduced by #2421427: Improve the UX of Quick Editing single-valued image fields, so that that test coverage is also more strict and uses a pattern similar to that used here. It also adds a QuickEditImageEditorTestTrait so that other tests can more easily include tests of the image in-place editor (it lives on that trait instead of on QuickEditJavaScriptTestBase because it's not guaranteed to be available: it's only available if the image module is installed).

That of course failed because #2421427: Improve the UX of Quick Editing single-valued image fields is a 8.3.x-only thing. The 8.2.x version of this patch should exclude those changes.

The final @todo:

// @todo test custom block

Finished that now too. This will make adding test coverage for #2661880: Quick Edit doesn't work for Custom Blocks referenced by other Custom Blocks a lot simpler.

Unassigning, because all done.

Still one hunk for 8.3.x snuck into the 8.2.x patch. Rerolled.

This is a blocker for at least #2661880: Quick Edit doesn't work for Custom Blocks referenced by other Custom Blocks, #2314185: [PP-1] DrupalBehaviorError: attach ; quickedit: entityElement.get(...) is undefined and #2635712: Cannot use Quick Edit to delete an image.

I don't quite understand why #26's 8.3.x patch failed. It passes locally. Retesting.

I think this is indeed ready for RTBC… except that the patch is failing for 8.3.x on testbot, but is passing locally :(

One of the @todos was more of a nice-to-have, the other one I created an issue for and linked to it in this reroll.

13:27:27 PHP Fatal error:  Class 'Drupal\Tests\quickedit\FunctionalJavascript\QuickEditJavaScriptTestBase' not found in /var/www/html/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php on line 15
13:27:27 
13:27:27 Fatal error: Class 'Drupal\Tests\quickedit\FunctionalJavascript\QuickEditJavaScriptTestBase' not found in /var/www/html/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php on line 15
13:27:27 FATAL Drupal\Tests\image\FunctionalJavascript\QuickEditImageTest: test runner returned a non-zero error code (255).
13:27:27 Drupal\Tests\image\FunctionalJavascript\QuickEditImageTest     0 passes   1 fails     

I'm sensing a case-sensitivity bug here, but I'm not seeing it.

I've triple-checked the case-sensitive stuff here. The filenames and classnames all have consistent casing. I have no idea what I can do anymore to push this forward. I pinged a testbot maintainer on IRC, hopefully he'll have insights:

15:53:11 <WimLeers> Mixologic: I have one more question for you. This one has been causing me to question my ability to read. https://www.drupal.org/node/2828528#comment-11811520 has a patch for 8.2.x and one for 8.3.x. The 8.3.x one is a superset of the 8.2.x one. Both include a new test base class: QuickEditJavaScriptTestBase. Both make the new QuickEditIntegrationTest extend
15:53:11 <WimLeers> that base class. But the 8.3.x one also updates an EXISTING test to use that base class: QuickEditImageTest. It's that last class (QuickEditImageTest) that's failing with inexplicable grandiosity:
15:53:18 <Druplicon> https://www.drupal.org/node/2828528 => Add Quick Edit Functional JS test coverage #2828528: Add Quick Edit Functional JS test coverage => 35 comments, 1 IRC mention
15:53:21 <WimLeers> 00:37:19.727 PHP Fatal error:  Class 'Drupal\Tests\quickedit\FunctionalJavascript\QuickEditJavaScriptTestBase' not found in /var/www/html/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php on line 15
15:53:21 <WimLeers> 00:37:19.730 
15:53:21 <WimLeers> 00:37:19.730 Fatal error: Class 'Drupal\Tests\quickedit\FunctionalJavascript\QuickEditJavaScriptTestBase' not found in /var/www/html/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php on line 15
15:53:21 <WimLeers> 00:37:19.933 FATAL Drupal\Tests\image\FunctionalJavascript\QuickEditImageTest: test runner returned a non-zero error code (255).
15:53:21 <WimLeers> 00:37:19.935 Drupal\Tests\image\FunctionalJavascript\QuickEditImageTest     0 passes   1 fails              
15:53:29 <WimLeers> It's reproducible on testbot
15:53:32 <WimLeers> I can't reproduce it locally
15:53:43 <WimLeers> I've triple-checked case sensitivestuff
15:53:48 <WimLeers> I really don't see it
15:53:54 <WimLeers> Mixologic: Have you seen this kind of thing before?

Mixologic didn't see any problems, but he noticed that it's strange to have class QuickEditJavaScriptTestBase extends JavascriptTestBase {, where one has JavaScript and the other has Javascript. So, making that consistent.

It wouldn't make sense if this would fix it, but … here's hoping anyway :)

Still the same, as expected. I wish I was wrong :(

#40: Can you provide an interdiff? What did you change?

#42: no, the failure on 8.3.x is not due to random fails. If you look at the full log for #37's 8.3.x patch, you'll see this:

00:39:04.338 PHP Fatal error:  Class 'Drupal\Tests\quickedit\FunctionalJavascript\QuickEditJavascriptTestBase' not found in /var/www/html/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php on line 15
00:39:04.340 
00:39:04.340 Fatal error: Class 'Drupal\Tests\quickedit\FunctionalJavascript\QuickEditJavascriptTestBase' not found in /var/www/html/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php on line 15
00:39:04.546 FATAL Drupal\Tests\image\FunctionalJavascript\QuickEditImageTest: test runner returned a non-zero error code (255).
00:39:04.548 Drupal\Tests\image\FunctionalJavascript\QuickEditImageTest     0 passes   1 fails        

So it's PHP not being able to find a certain class, for a reason I can't figure out.

Reuploading #40's patches to see if something has changed (HOPEFULLY!).

OMG thank you @ZeiP for getting this going again! Your work on this is immensely appreciated!

👏👍

Thanks so much for pushing this forward!

So the random fail problem still exists.

:( :( :(

Any ideas?

Closed #2938308: Add QuickEdit Javascript tests as a duplicate.

#2917218: Test to prevent regression between quickedit and ckeditor landed, which added some Quick Edit JS test coverage. Hopefully that means we can land this too.

Thanks, @vaplas! 🎉🙏

+++ b/core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
@@ -17,6 +18,11 @@ class QuickEditImageTest extends QuickEditJavascriptTestBase {
+  protected $minkDefaultDriverClass = DrupalSelenium2Driver::class;

Should this be removed once #2942900: Convert JavascriptTestBase Tests to use DrupalSelenium2Driver lands?

Can't believe I lost track of this issue for so long, I'm very sorry!

Also, this is now reliably passing tests. I think it's high time to finally get this committed, so that we finally don't regress in Quick Edit anymore.

I last worked on this 1.5 years ago, so I think that makes me eligible to RTBC this.

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉