Problem/Motivation

See upstream issues:

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#25 interdiff_20-25.txt518 bytesspokje
#25 3314710-25.patch1.19 KBspokje
#20 3314710-20.patch1.4 KBalexpott
#19 3314710-19.patch4.32 KBalexpott
#19 3314710-2.test-only.patch2.93 KBalexpott
#7 3314710-7.patch1.38 KBalexpott
#7 6-7-interdiff.txt619 bytesalexpott
#6 3314710-6.patch1.38 KBalexpott
#2 3314710-2.patch3.97 KBalexpott
#2 3314710-2.test-only.patch2.93 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3314710-2.test-only.patch2.93 KB
new 3314710-2.patch3.97 KB

The last submitted patch, 2: 3314710-2.test-only.patch, failed testing. View results

The last submitted patch, 2: 3314710-2.test-only.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes

Adding upstream related issues to the summary. This pretty clearly a cause of random fails.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Normal » Critical
Issue tags: +Random test failure
StatusFileSize
new 3314710-6.patch1.38 KB

Here's a patch which complies with our coding standards. Let's see what happens with the rest of the test suite.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 6-7-interdiff.txt619 bytes
new 3314710-7.patch1.38 KB

Fixing capitalisation issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

https://github.com/minkphp/MinkSelenium2Driver/pull/333 might also be relevant...

The last submitted patch, 6: 3314710-6.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 7: 3314710-7.patch, failed testing. View results

quietone’s picture

quietone commented

None of the failures in #6 and #7 are the one that is from the fail test in #2, which is
WebDriver\Exception\CurlExec: Curl error thrown for http POST to http://chromedriver-jenkins-drupal8-core-regression-tests-68651:9515/session/e035c0c985e1fc0be9662b63581124c9/execute with params: {"script":"(function (element) {\n var event = document.createEvent(\"HTMLEvents\");\n\n event.initEvent(\"drop\", true, true);\n event.dataTransfer = {};\n\n element.dispatchEvent(event);\n}(arguments[0]));","args":[{"ELEMENT":"0.5203560764679722-5"}]}

Based on that evidence I'd like to see this in. Should the test in #2 run more that 50 times?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I think the evidence in #2 is quite clear that this is a source of random fails. Given it fails consistently without the code and consistently passes with. I don't think upping the 50 is going to show us much given the fail rate.

I think the big question for this issue is correctness. I guess what we should add is a dragTo test and confirm that on DrupalCI these events are still triggered.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Reviewed & tested by the community

I think the big question for this issue is correctness. I guess what we should add is a dragTo test and confirm that on DrupalCI these events are still triggered.

I'm all about correctness, but doesn't the fact that test using dragTo consistently fail without the patch and pass with the patch already prove that the patch-version of dragTo is triggering?

Also, since TestBot is failing all over the place lately, making testing not so much fun and causing extra test-cycles to be used, can the above mentioned test be done in a follow-up to get this one in quickly?

Putting on RTBC for the above reasoning. Happy to corrected :)

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

RTBC+1, these random fails are really hard to reproduce outside of DrupalCI and we don't have enough visibility into DrupalCI to figure out what is going wrong, so for now let's get this in and hopefully resolve some of the weird random failures we have seen this week.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

My concern is that the dragstart and drop events are not being triggered anymore. Otherwise why does the upstream code exist? That's why I think we should add a test that proves these events are being triggered on our test system so at least we know that things have not changed there. Maybe no core test relies on these being triggered but maybe something in contrib or custom does?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

https://github.com/minkphp/MinkSelenium2Driver/pull/75 was rejected but we could just add the method with a different name to our own driver?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

A comment from 2014 that still applies today:

Without a way to detecting which events are fired natively we can't safely use Syn, because there is a risk, that event can be fired twice (native and via syn), which indeed may result in some weird results.

https://github.com/minkphp/MinkSelenium2Driver/issues/51#issuecomment-31...

It seems possible that we have both native and synthetic events being fired - and that could be the cause of some random fails? Especially given the async nature of JavaScript, if some events fire twice, and sometimes in an unexpected order, this could cause undefined/non-deterministic behaviour?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Looking at fails like https://www.drupal.org/pift-ci-job/2498577 - the dragTo related fails are the stale element ones. The lines of code that causes this are:

        $script = <<<JS
(function (element) {
    var event = document.createEvent("HTMLEvents");

    event.initEvent("drop", true, true);
    event.dataTransfer = {};

    element.dispatchEvent(event);
}({{ELEMENT}}));
JS;
        $this->withSyn()->executeJsOnElement($destination, $script);

in \Behat\Mink\Driver\Selenium2Driver::dragTo().

I think we can do something a bit more defensive here. That will hopefully get us round the randoms without changing things too much.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 3314710-2.test-only.patch2.93 KB
new 3314710-19.patch4.32 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 3314710-20.patch1.4 KB

If #19 is good here is the patch to commit.

The last submitted patch, 19: 3314710-2.test-only.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

I like the new approach as it's even less of a BC break than before. Still not even sure we need to fire those synthetic events, but this works around the issue in a better way. The results in #19 are great so to me #20 is RTBC.

spokje’s picture

spokje
Primary language Dutch
commented

Cross-post RTBC (now +1)

quietone’s picture

quietone commented 
+++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
@@ -2,6 +2,8 @@
+// cspell:ignore buttonup buttondown
+

This isn't needed anymore.

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new 3314710-25.patch1.19 KB
new interdiff_20-25.txt518 bytes

Nightowl @quietone proving her nightvision there...

The last submitted patch, 20: 3314710-20.patch, failed testing. View results

spokje’s picture

spokje
Primary language Dutch
commented
Status: Reviewed & tested by the community » Needs review

So I think I got very lucky with the testrun in #25, but the testrun in #20 is also without any of the WebDriver\Exception\CurlExec: Curl error thrown for http POST exceptions.

Putting back to NR, since changes were made since the last RTBC, but I think this is one less annoying random-test-failure.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

The changes from #20 to #25 are only extraneous comment removal. The original RTBCs still stand.

  • lauriii committed 4b98aae on 10.1.x 
    Issue #3314710 by alexpott, Spokje, longwave: dragTo random fails for...

  • lauriii committed 931361f on 10.0.x 
    Issue #3314710 by alexpott, Spokje, longwave: dragTo random fails for...

  • lauriii committed c0d437e on 9.5.x 
    Issue #3314710 by alexpott, Spokje, longwave: dragTo random fails for...

  • lauriii committed c738157 on 9.4.x 
    Issue #3314710 by alexpott, Spokje, longwave: dragTo random fails for...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 10.1.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 4b98aae and pushed to 10.1.x. Also cherry-picked all the way down to 9.4.x hoping tests are a bit more stable as a result of this. Thanks!

quietone’s picture

quietone commented

I looked at the test runs for 9.4, 9.5, 10.0 and 10.1 after this commit and there is only once instance of this particular failure. It is on 9.4.x. That is a huge improvement. Thanks!

https://www.drupal.org/pift-ci-job/2498743 Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest::testRemove,

Status: Fixed » Closed (fixed)

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