Time and time again, we've seen regressions being introduced to Quick Edit. I don't want to know how many hours of manual testing Jesse Beach, Théodore (nod) Biadala and I have done.

Back when Quick Edit was written, Drupal 8 (and its testing infrastructure) weren't capable of executing JS. So we could not write tests. Earlier this year, a few months ago, support for that was added.

#2421427: Improve the UX of Quick Editing single-valued image fields was committed yesterday. It introduced a new in-place editor plugin. It also introduced JS test coverage for that particular in-place editor. It's time we also add JS test coverage for the rest of Quick Edit. That'll allow us to add features with confidence, but also fix bugs with confidence.

For example, #2661880: [PP-1] Quick Edit doesn't work for Custom Blocks referenced by other Custom Blocks had an RTBC patch, which I un-RTBC'd. Because it should have test coverage. But without a body of tests to extend, that's difficult and even unreasonable. So I postponed it on this issue.

19 of the 36 open Quick Edit issues are bug reports. That's more than 50%. Adding test coverage for each (or at least most) of them would make Quick Edit infinitely more maintainable. This issue makes that much more feasible.

Files: 

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

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.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Wim Leers’s picture

Issue tags: +blocker
Wim Leers’s picture

Status: Active » Needs review
FileSize
12.99 KB

Not done yet, but pretty far along :)

Wim Leers’s picture

FileSize
14.29 KB
3.04 KB

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);
Wim Leers’s picture

FileSize
20.77 KB
12.81 KB

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.

Wim Leers’s picture

FileSize
20.71 KB
17.64 KB

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

Wim Leers’s picture

FileSize
22.45 KB
13.42 KB
  1. Rename/refactor methods for consistency
  2. Add missing docblocks

The last submitted patch, 7: 2828528-7.patch, failed testing.

The last submitted patch, 8: 2828528-8.patch, failed testing.

Wim Leers’s picture

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: [PP-1] Quick Edit doesn't work for Custom Blocks referenced by other Custom Blocks and #2466715: [PP-1] Displaying the same entity twice on one page, but in different view modes, doesn't work as expected need.

The last submitted patch, 8: 2828528-8.patch, failed testing.

Wim Leers’s picture

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?
samuel.mortenson’s picture

Just read through the patch, looks good and the coverage is very thorough. So for "Does this set a good enough baseline of test coverage?", I would say yes!

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.

For example, in https://www.drupal.org/files/issues/panelizer-quickedit-2693163-10.patch (a patch to add Quick Edit support to Panelizer), I wrote the following to Quick Edit a text field:

+    // Wait until Quick Edit loads.
+    $condition = "jQuery('" . $entity_selector . " .quickedit').length > 0";
+    $this->assertJsCondition($condition, 10000);
+
+    // Initiate Quick Editing.
+    $this->triggerClick($entity_selector . ' [data-contextual-id] > button');
+    $this->click($entity_selector . ' [data-contextual-id] .quickedit > a');
+    $this->triggerClick($field_selector);
+    $this->assertSession()->assertWaitOnAjaxRequest();
+
+    // Trigger an edit with Javascript (this is a "contenteditable" element).
+    $this->getSession()->executeScript("jQuery('" . $field_selector . "').text('Hello world').trigger('keyup');");
+
+    // To prevent 403s on save, we re-set our request (cookie) state.
+    $this->prepareRequest();
+
+    // Save the change.
+    $this->triggerClick('.quickedit-button.action-save');
+    $this->assertSession()->assertWaitOnAjaxRequest();

If this patch had protected methods that were high-level, i.e. quickEditTextField($field_id, $new_text), I could extend QuickEditIntegrationTest and remove a lot of the code here. I also copy+pasted this code into #2815221: Add support for Quick Edit to the Content Moderation module, which would also benefit from having more protected methods in QuickEditIntegrationTest.

Wim Leers’s picture

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)

samuel.mortenson’s picture

Yes, as many reusable methods as possible! Ideally in a child class I would be able to start Quick Edit, initiate editing on a field, change its value, and save without ever writing a selector.

Wim Leers’s picture

FileSize
23.32 KB
4.38 KB

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

Wim Leers’s picture

Status: Needs review » Needs work

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.

samuel.mortenson’s picture

"What else could we move to protected methods, what else would be valuable?" I think this is subjective, but the general requirement I would have is that child classes would only be calling QuickEditIntegrationTest methods unless they're testing an edge case for Quick Edit, or a new inline editor.

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. Since you're the maintainer and I don't want to slow down the test coverage getting in, I'll leave it up to you to decide how abstract you get. :-) We can always work on this class in other issues if we need more methods.

One super tiny nit:

+    // 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'");

The comment says "title field", but this is actually testing that the field_tags field is a candidate for Quick Editing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
24.58 KB
5.02 KB

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.

Wim Leers’s picture

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

Wim Leers’s picture

FileSize
25.71 KB
32.31 KB

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

Wim Leers’s picture

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).

Status: Needs review » Needs work

The last submitted patch, 24: 2828528-24.patch, failed testing.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
4.31 KB
31.35 KB
38.67 KB

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: [PP-1] Quick Edit doesn't work for Custom Blocks referenced by other Custom Blocks a lot simpler.

Unassigning, because all done.

Wim Leers’s picture

FileSize
28.06 KB

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

The last submitted patch, 26: 2828528-26-8.3.x.patch, failed testing.

Wim Leers’s picture

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

The last submitted patch, 26: 2828528-26-8.3.x.patch, failed testing.

samuel.mortenson’s picture

I think this is a great start and I would RTBC it, but I notice there are two @todos still in the patch. Do those still need addressed?

Wim Leers’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 33: 2828528-33-8.3.x.patch, failed testing.

Wim Leers’s picture

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.

Wim Leers’s picture

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?
Wim Leers’s picture

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 :)

The last submitted patch, 37: 2828528-37-8.3.x.patch, failed testing.

Wim Leers’s picture

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

Status: Needs review » Needs work

The last submitted patch, 40: test-only-clean.patch, failed testing.

michielnugter’s picture

I took a quick look at the test and QuickEditIntegrationTest::testArticleNode() consistantly fails for me on PHP 7.

1) Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest::testArticleNode
Failed asserting that Array &0 (
    'node/1/title/en/full' => 'saving'
    'node/1/uid/en/full' => 'candidate'
    'node/1/created/en/full' => 'candidate'
    'node/1/body/en/full' => 'candidate'
    'node/1/field_tags/en/full' => 'activating'
) is identical to Array &0 (
    'node/1/uid/en/full' => 'candidate'
    'node/1/created/en/full' => 'candidate'
    'node/1/body/en/full' => 'candidate'
    'node/1/field_tags/en/full' => 'activating'
    'node/1/title/en/full' => 'candidate'
).

/Library/WebServer/Documents/drupal/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php:226
/Library/WebServer/Documents/drupal/core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php:205

Looking at the test it seems that there should be a wait before the assertEquals()? It looks like it's still busy processing the request..

BTW: typeInFormEditorTextInputField() seems like it could use a waitForDebounce() (or #2837676: Provide a better way to validate all javascript activity is completed) as the formUpdated event will be triggered eventually (after the debounce finishes).

EDIT: I got the getText() error also once, randomly. To me it looks like there are some javascript waits missing?

Wim Leers’s picture

#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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 45: test-only_18.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 45: test-only-clean.patch, failed testing.