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.

ZeiP’s picture

Status: Needs work » Needs review
FileSize
38.4 KB

Let's try this.

Status: Needs review » Needs work

The last submitted patch, 48: 2828528-48.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ZeiP’s picture

Status: Needs work » Needs review
FileSize
38.1 KB

Well, it wasn't the same error, so I think the fix might've worked, but there's still something wrong. Let's try a new patch with the coding standards errors fixed.

Status: Needs review » Needs work

The last submitted patch, 50: 2828528-50.patch, failed testing. View results

ZeiP’s picture

Status: Needs work » Needs review
FileSize
38.15 KB

Just testing if I could get a better error message from the testbot.

Status: Needs review » Needs work

The last submitted patch, 52: test_2828528-52.patch, failed testing. View results

ZeiP’s picture

I think this fixes finding the Bold button.

ZeiP’s picture

Status: Needs work » Needs review
ZeiP’s picture

FileSize
6.29 KB

Okay, this seems to work now. Attached is an interdiff; basically I just changed the operator used to find the Bold button (because it seems to now include the keyboard shortcut in the title) and fixed some coding standards errors. Ready for review.

Edit: Never mind, forgot I added a bit of debug code in. I'll need to remove that, sorry!

Status: Needs review » Needs work

The last submitted patch, 54: test_2828528-54.patch, failed testing. View results

ZeiP’s picture

Status: Needs work » Needs review
FileSize
38.28 KB

Trying to use a new wait method to make sure the toolbar is available when asserting.

Status: Needs review » Needs work

The last submitted patch, 58: 2828528-58.patch, failed testing. View results

ZeiP’s picture

Status: Needs work » Needs review
FileSize
38.28 KB

Retry with correct argument order.

ZeiP’s picture

Ok, it seems that using waitForElement() fixed the occasional fail issue. Attached is a final patch and an interdiff for it, should be ready for review.

Status: Needs review » Needs work

The last submitted patch, 61: 2828528-61.patch, failed testing. View results

Wim Leers’s picture

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

👏👍

Status: Needs review » Needs work

The last submitted patch, 63: 2828528-63.patch, failed testing. View results

ZeiP’s picture

Status: Needs work » Needs review
FileSize
38.88 KB

Heh, thanks, no problem! :) Trying to get into contributing to core and this seemed like a nice issue for that. Unfortunately my work on this issue has been a bit spammy, because I couldn't get the JS tests to work properly in my local environment, and have to use testbot for running each iteration instead.

Hopefully won't take too many more trials to figure this out. Adding a more permanent idea of the debugging check; this allows us to see a more useful stack/error message from the testbot when the quickedit toolbar assertion fails. Let's try.

Status: Needs review » Needs work

The last submitted patch, 66: 2828528-66.patch, failed testing. View results

droplet’s picture

@ZeiP,

You could use this snippet to do a quick test

+++ b/core/scripts/run-tests.sh
@@ -142,7 +142,13 @@
+if (in_array('Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest', $test_list)) {
+  // Do the test 25 times.
+  $test_list = array_fill(0, 1, 'Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest');
+}
+else {
+  $test_list = [];
+}
droplet’s picture

FileSize
39.57 KB

I will provide interdiff later

droplet’s picture

FileSize
39.72 KB
droplet’s picture

FileSize
39.72 KB

(Don't use this patch)

droplet’s picture

FileSize
14.56 KB

Ahh I got the problem. Needs some input from @wim since I don't familiar with quickedit Default behavior. (I thought it's a bug)

No "Title" when your mouse doesn't hover it

droplet’s picture

// Click the title field.

And on the test, it says `Click` but I can't see any click action on test.

ZeiP’s picture

Status: Needs work » Needs review
FileSize
38.88 KB

I added waitForElements() without creating the necessary object first; trying to fix that.

Status: Needs review » Needs work

The last submitted patch, 74: 2828528-74.patch, failed testing. View results

droplet’s picture

Status: Needs work » Needs review
FileSize
40.19 KB

Many errors in the test tho..

fixed few tests and see if any random fail on test bot. I guess it can be split and commit test without problems first

Status: Needs review » Needs work

The last submitted patch, 76: test-75.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

droplet’s picture

EDITED: The problem on Title is fixed. Now it's failed on a diff point.

droplet’s picture

Don't work on this patch. Use patch #78

I commented out the fail test and see how other tests performed. and then we able to go back and fix it quick.

droplet’s picture

One note:

$this->assertEntityInstanceFieldStates

the ordering of JS object/array doesn't matter. We should update this assertion to verify per object entry instead.

ZeiP’s picture

Status: Needs work » Needs review
FileSize
38.92 KB

For some reason #74 was missing the change I was trying to make there. Attached is the patch again, this time with the change present.

This change is missing all @droplet's work after #66.

droplet’s picture

Patch #81 is equal to #68. But one GREEN, and one RED.

Trying to run it 30 times.

Status: Needs review » Needs work

The last submitted patch, 82: random-fail-test.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

droplet’s picture

To check the failure rate. It's about 50/50
https://dispatcher.drupalci.org/job/drupal_patches/21546/console

ZeiP’s picture

Right, sorry – it isn't equal, but it indeed does the same thing. So the random fail problem still exists.

Wim Leers’s picture

Thanks so much for pushing this forward!

So the random fail problem still exists.

:( :( :(

Any ideas?

droplet’s picture

no idea. I guess it's a PhantomJS bug. I tried:

- adding sleep(10), enough time to load everything in local
- use JS perform a click.

It always failed to this point:
https://github.com/drupal/drupal/blob/612c1fa68cfca2346c3d981383827278dc...
(hidden bug? I didn't dig into quickedit)

drop the label (Title) test for now I'd say.

tacituseu’s picture

Did some debugging on testbot:
1. good run:
good run
2. bad run:
bad run
3. weird run, with title repeated at the bottom:
weird
4. weirder, with "Member for: 11 seconds":
weirder
5. even weirder, textfield and button under tags
even weirder

All screenshots (patch).

Did find one bug/race producing similar result (toolbar without "TItle ->") and permanent error state, will describe steps to reproduce in next post.

tacituseu’s picture

There is a race/error state when clicking on the quickeditable field during toolbar's flight, to reproduce:
1. setup, give yourself some time/space (not needed - triggered even without this but makes it easier) by adding 200px margin, hover over the page/node title
setup
2. click on 'Authored by' field to start the animation
travel
3. before the toolbar reaches the target and loads the widget quickly click on the title again, results:
error 1
error 2
script

the toolbar is producing JS errors when moving cursor around to focus/unfocus the quickeditable fields, has no 'Title ->' prefix, and the close 'x' doesn't work.

droplet’s picture

droplet’s picture

It seems not the race issue on this failture

droplet’s picture

Trigger with JS and then click on Title

droplet’s picture

droplet’s picture

last try today. Sadly, testbot has no more phantomjs.log?