Problem/Motivation

While debugging #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly we found that Javascript conditions/assertions always do an unnecessary 100ms wait and created https://github.com/jcalderonzumba/MinkPhantomJSDriver/pull/68 , which was accepted and released. We should use this new version now in Drupal to shave off a couple of seconds when running tests.

Proposed resolution

jcalderonzumba/gastonjs              v1.0.2             v1.2.0             PhantomJS API based server for webpage automation
jcalderonzumba/mink-phantomjs-driver v0.3.1             v0.3.3             PhantomJS driver for Mink framework
CommentFileSizeAuthor
#128 2832672-128.patch3.1 KBmartin107
#128 2832672-128.patch3.1 KBmartin107
#125 2832672-125.patch3.1 KBjibran
#120 combine-upgrade-2832672-120.patch10.07 KBjibran
#120 upgrade-2832672-120.patch3.74 KBjibran
#114 phantom-driver-2832672-113-25x-upgrade.patch5.99 KBmichielnugter
#114 interdiff.txt37.98 KBmichielnugter
#110 phantom-driver-2832672-110-25x-upgrade.patch42.29 KBmichielnugter
#110 interdiff-97-110.txt995 bytesmichielnugter
#109 201702050143081486219388.1182-before-create-new-filter-group-click.jpg73.16 KBtacituseu
#109 201702050143081486219388.018-after-findbyid-views-add-group-link.jpg74.66 KBtacituseu
#109 2832672-1xx_0.patch3.52 KBtacituseu
#108 good-run-annotated-excerpt-from-phantomjs.err_.log-fixed-typo.txt5.23 KBtacituseu
#107 14-filter.test.passes-annotated.png305.8 KBtacituseu
#107 bad-run-annotated-excerpt-from-phantomjs.err_.log_.txt5.22 KBtacituseu
#107 good-run-annotated-excerpt-from-phantomjs.err_.log_.txt5.23 KBtacituseu
#102 reason.mp4438.67 KBdroplet
#101 1-pixel-diff.patch2.67 KBdroplet
#99 test-only-2832672-99.patch3.16 KBdroplet
#95 phantom-driver-2832672-95-25x.patch42.31 KBmichielnugter
#93 phantom-driver-2832672-93-25x.patch41.43 KBmichielnugter
#90 phantom-driver-2832672-90-25x.patch41.78 KBmichielnugter
#88 phantom-driver-2832672-87-25x.patch41.29 KBmichielnugter
#85 2832672-83-no-extra-position.patch3.59 KBdroplet
#83 2832672-83-more-throbber.patch5.57 KBtacituseu
#82 test-only-waiting-on-shaking.patch3.17 KBdroplet
#79 2832672-79-throbber-test.patch3.64 KBtacituseu
#78 test-only-big-enough-for-shaking.patch3.68 KBdroplet
#76 2832672-72-view-button.patch3.39 KBdroplet
#73 2832672-72-css-selector.patch2.56 KBdroplet
#73 2832672-72-js.patch2.65 KBdroplet
#70 2832672-70-confirmation-only.patch3.46 KBtacituseu
#69 2832672-69-confirmation-only.patch1.45 KBtacituseu
#67 2832672-67-confirmation-only.patch880 bytestacituseu
#66 2832672-66.patch5.25 KBtacituseu
#65 2832672-65.patch4.48 KBtacituseu
#63 2832672-63.patch4.27 KBtacituseu
#61 2832672-61.patch3.85 KBtacituseu
#59 2832672-59.patch3.79 KBtacituseu
#56 2832672-55.patch2.58 KBdroplet
#54 2832672-53.patch4.67 KBdroplet
#52 2832672-52.patch4.6 KBdroplet
#50 2832672-50.patch4.57 KBdroplet
#47 upgrade-only.patch21.32 KBdroplet
#47 upgrade-only-old-composer.patch2.02 KBdroplet
#45 phantom-driver-2832672-45-50x.patch38.41 KBmichielnugter
#43 phantom-driver-2832672-42-50x.patch73.16 KBmichielnugter
#40 phantom-driver-2832672-40-50x.patch5.05 KBmichielnugter
#40 interdiff-31-40.txt1.1 KBmichielnugter
#31 phantom-driver-2832672-31-50x.patch5 KBmichielnugter
#31 interdiff-27-31.txt1.25 KBmichielnugter
#27 phantom-driver-2832672-27-updated.patch4.21 KBmichielnugter
#27 phantom-driver-2832672-27-updated-50.patch4.81 KBmichielnugter
#27 interdiff-22-27.patch2.75 KBmichielnugter
#22 phantom-driver-2832672-22.patch3.94 KBklausi
#20 interdiff.txt1.32 KBklausi
#20 phantom-driver-2832672-20.patch1.32 KBklausi
#18 interdiff.txt3.05 KBklausi
#18 phantom-driver-2832672-18.patch3.98 KBklausi
#16 interdiff.txt893 bytesklausi
#16 phantom-driver-2832672-16.patch5.97 KBklausi
#14 filter.test.passes.jpg71.9 KBmichielnugter
#14 filter.test.fails_.jpg71.76 KBmichielnugter
#12 interdiff.txt1.31 KBklausi
#12 phantom-driver-2832672-12.patch5.89 KBklausi
#9 interdiff.txt826 bytesklausi
#9 phantom-driver-2832672-9.patch5.25 KBklausi
#7 interdiff.txt1.35 KBklausi
#7 phantom-driver-2832672-7.patch4.45 KBklausi
#5 interdiff.txt609 bytesklausi
#5 phantom-driver-2832672-5.patch3.09 KBklausi
#2 phantom-driver-2832672-2.patch2.5 KBklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Needs review » Needs work

The last submitted patch, 2: phantom-driver-2832672-2.patch, failed testing.

klausi’s picture

Very interesting. So with this patch we immediately see even more JavascriptTestBase fails. assertWaitOnAjaxRequest() looks most suspicious to me because it has a way too generic JS condition that might not apply correctly in all instances.

A JS condition should test for example if an element is visible. Checking if jQuery is active is not specific enough and seems dangerous to me.

Next steps for this issue:
1) Make a patch that runs Drupal\Tests\views_ui\FunctionalJavascript\FilterCriteriaTest 50 times which reliably fails all the time.
2) Try to remove all assertWaitOnAjaxRequest() from FilterCriteriaTest and replace them with proper visibility conditions until the 50 test runs pass.

klausi’s picture

Here is a patch that repeats the failing test 50 times and should always fail.

Status: Needs review » Needs work

The last submitted patch, 5: phantom-driver-2832672-5.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
4.45 KB
1.35 KB

Let's try a proper JS condition instead of the dubios assertWaitOnAjaxRequest() where the test fails.

Status: Needs review » Needs work

The last submitted patch, 7: phantom-driver-2832672-7.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
5.25 KB
826 bytes

So the modified javascript condition also fails on the testbot. But only on PHP 5 on the testbot, not on PHP 7. Same locally, everything passing for me.

Let's see how long the JS assertion actually takes on the testbot and if it runs into the timeout.

klausi’s picture

Parent issue: » #2807237: PHPUnit initiative

Status: Needs review » Needs work

The last submitted patch, 9: phantom-driver-2832672-9.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
5.89 KB
1.31 KB

Does an even higher timeout help?

Status: Needs review » Needs work

The last submitted patch, 12: phantom-driver-2832672-12.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
71.76 KB
71.9 KB

I applied the patch in #12 and ran the tests via run-tests and directly via phpunit on php 5.5 and 7.1.

I had 1 fail on 5.5 so far locally, so it's actually reproducable. I added screenhots:

    $this->createScreenshot('/Library/Webserver/Documents/_testscreenshots/filter.before.' . microtime(true) . '.jpg');
    $this->assertJsCondition('jQuery("a:contains(\'Remove group\')").is(":visible")');
    $this->createScreenshot('/Library/Webserver/Documents/_testscreenshots/filter.' . microtime(true) . '.jpg');

I attached 2 screenshots, one for which the test passed and one for a failed test.

As you can see the progress indicator is missing. It seems then that the click on the button somehow failed. No matter how you wait, with this version of Mink (upgraded from 1.6 to 1.7) / jcalderonzumba this test will always randomly fail.

So, is this a bug in Mink / jcalderonzumba or in Drupal?

michielnugter’s picture

This got me thinking a little more. Might the failed click have something to do with the random fails in #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly?

Both have something to do with clicking a button..

EDIT:
I looked a little closer to the changes from 0.3.1 to 0.3.3, there is a little more to this upgrade than it seems at first sight.
Other changes not in the CHANGELOG:
- The dependencies are updated, making Mink switch from ~1.6 to ~1.7
- The way Javascript code is evaluated is changed (https://github.com/jcalderonzumba/MinkPhantomJSDriver/commit/c25bba0357d...)

klausi’s picture

@michielnugter: nice research!

I tried to reproduce locally on Ubuntu with PHP 5.5, but PhantomJS randomly hangs again (already reported by other people at https://github.com/ariya/phantomjs/issues/14286 ).

So I have to continue my guessing game here by triggering the testbot. Let's try to click the link before the fail in a different way and see if that works.

Status: Needs review » Needs work

The last submitted patch, 16: phantom-driver-2832672-16.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
3.98 KB
3.05 KB

So that also made no difference.

Reverting back to the original test and clicking repeatedly in a while loop, let's see if that makes the test pass.

michielnugter’s picture

Status: Needs review » Needs work

Yup, that makes it work..

Maybe debounce() or something like that is causing problems?

klausi’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
1.32 KB

Let's try some waiting before the click instead.

Status: Needs review » Needs work

The last submitted patch, 20: phantom-driver-2832672-20.patch, failed testing.

klausi’s picture

Ooops, wrong patch file, fixed that.

I don't understand views-admin.js. It has a hidden "Add filter group" button but it adds an artificial link with an event handler that then clicks the button. Why can't we just show the button in place of the link?

At this point my working theory is that the "Create new filter group" link is visible but views-admin.js is not done yet attaching the behavior to click the button. What would be a good jQuery condition that we could use in the test to make sure the event handlers are ready for the link?

michielnugter’s picture

I see the problem now.

If you click on the "And/Or Rearrange" button an ajax request is fired, returning the HTML for the pop-up. The ajax requests is finished but it also triggers loading additional JavaScript files. After these files are loaded (I assume) Drupal starts executing the behaviors.

The test however continues immediately and it's pretty much random if the test succeeds (the javascript files are loaded fast and behaviors are executed) or failes (loading takes a bit longer and behaviors aren't executed yet).

Funny thing: our performance gain (not waiting an extra 100ms) actually exposed this bug. Before the testbot had an extra 100ms to finish which was enough apparently..

michielnugter’s picture

I created #2837676: Provide a better way to validate all javascript activity is completed which could be a fix for this issue. I'd like to know if the created issue is a good solution for this problem.

michielnugter’s picture

#2837676: Provide a better way to validate all javascript activity is completed is in and should have fixed our random fail for this issue. However, because the usage of assertWaitOnAjaxRequest is discouraged and still required I think we should add a comment for this testcase to indicate the wait is required in this case.

Would this be in scope for this issue?

klausi’s picture

Status: Needs review » Needs work

Yes, absolutely.

The patch needs work anyway because the current blind wait for 500ms is surely not good enough.

michielnugter’s picture

The 500ms is not required anymore as the assertWaitOnAjaxRequest now waits for the complete request to finish, yay :)

I updated the test to either use the new wait methods or document why they're not used. I attached a 50x version as well to see if it doesn't trigger random fails.

michielnugter’s picture

Status: Needs work » Needs review

The last submitted patch, 27: phantom-driver-2832672-27-updated-50.patch, failed testing.

klausi’s picture

Status: Needs review » Needs work

That does not work, test is failing now at line 65.

michielnugter’s picture

Checked the logs, the fail is random unfortunately.

Changed the implementation slightly to make it wait for the UI first and then the ajax request.

michielnugter’s picture

Status: Needs work » Needs review

The last submitted patch, 27: interdiff-22-27.patch, failed testing.

The last submitted patch, 27: phantom-driver-2832672-27-updated.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 31: phantom-driver-2832672-31-50x.patch, failed testing.

droplet’s picture

Should / Can we split into 2 issues?

One for upgrade
One for improving & fixed

michielnugter’s picture

sounds like a plan, we have to postpone this one for the fix-issue though..

klausi’s picture

No, we can only upgrade with all tests passing. Tests have to be green at all times.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
5.05 KB

I think the problem may be with that the link was available in the DOM but not yet visible. I changed the way the button is checked. It now passes locally using run-tests.sh.

Status: Needs review » Needs work

The last submitted patch, 40: phantom-driver-2832672-40-50x.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review

Right 8.4.x...

michielnugter’s picture

Status: Needs review » Needs work

The last submitted patch, 43: phantom-driver-2832672-42-50x.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
38.41 KB

Time for some coffee I guess.

Retried the package upgrade..

Status: Needs review » Needs work

The last submitted patch, 45: phantom-driver-2832672-45-50x.patch, failed testing.

droplet’s picture

What does the bot say?

The last submitted patch, 47: upgrade-only-old-composer.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 47: upgrade-only.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
4.57 KB

Status: Needs review » Needs work

The last submitted patch, 50: 2832672-50.patch, failed testing.

droplet’s picture

@michielnugter's patch worked on my local too.

What does bot say again?

Status: Needs review » Needs work

The last submitted patch, 52: 2832672-52.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
4.67 KB

checking the test log, some of them are passed and it took almost 10 sec to execute it. Increasing the timeout.

Status: Needs review » Needs work

The last submitted patch, 54: 2832672-53.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
2.58 KB

I could see above test is failed.
https://dispatcher.drupalci.org/job/drupal_patches/329/console

last test, what if it doesn't upgrade the mink driver?

Status: Needs review » Needs work

The last submitted patch, 56: 2832672-55.patch, failed testing.

michielnugter’s picture

I was able to reproduce the random fails locally now on 8.4.x using run-tests.sh :(

tacituseu’s picture

Status: Needs work » Needs review
FileSize
3.79 KB

Just checking something, not sure this is the best way to do this.

Status: Needs review » Needs work

The last submitted patch, 59: 2832672-59.patch, failed testing.

tacituseu’s picture

Once more...

Status: Needs review » Needs work

The last submitted patch, 61: 2832672-61.patch, failed testing.

tacituseu’s picture

Status: Needs work » Needs review
FileSize
4.27 KB

Some more tracking...

Status: Needs review » Needs work

The last submitted patch, 63: 2832672-63.patch, failed testing.

tacituseu’s picture

tacituseu’s picture

tacituseu’s picture

What happens is '#views-add-group-link' is created and .on('click.views-rearrange-filter-handler', $.proxy(this, 'clickAddGroupButton')); is bound just fine, but in failed tests it never actually triggers, because in

FilterCriteriaTest::testFilterCriteriaDialog() {
...
$create_new_filter_group->click();
...

misses the link, it clicks just above it ;)

For details search phantomjs.err.log for: "( QEvent::Type(MouseButtonPress) ) QPoint(252,".
- the good ones have:
Mouse Event: "mousedown" ( QEvent::Type(MouseButtonPress) ) QPoint(252,324) ) 1 1
and a couple of lines higher result of .position() on it:
result QVariant(QString, "{\"value\":{\"top\":313,\"right\":357,\"left\":146,\"bottom\":335,\"width\":211,\"height\":22}}")
- the bad one have:
Mouse Event: "mousedown" ( QEvent::Type(MouseButtonPress) ) QPoint(252,300) ) 1 1
result QVariant(QString, "{\"value\":{\"top\":289,\"right\":357,\"left\":146,\"bottom\":311,\"width\":211,\"height\":22}}")
So possibly late .css load or some element (button) hiding script makes it "jump".
Screenshots from #14 were very helpful, attached confirmation patch just makes the button easier target.

Status: Needs review » Needs work

The last submitted patch, 67: 2832672-67-confirmation-only.patch, failed testing.

tacituseu’s picture

tacituseu’s picture

now with test from #55, changed selector to 'named_exact' because:

For conditions based on the content of elements, the named selector will try to find an exact match first. It will then fallback to partial matching in case there is no result for the exact match.

Status: Needs review » Needs work

The last submitted patch, 70: 2832672-70-confirmation-only.patch, failed testing.

tacituseu’s picture

Failing now have .position():
result QVariant(QString, "{\"value\":{\"top\":269,\"right\":357,\"left\":146,\"bottom\":330,\"width\":211,\"height\":61}}")
and the good ones:
result QVariant(QString, "{\"value\":{\"top\":293,\"right\":357,\"left\":146,\"bottom\":354,\"width\":211,\"height\":61}}")

droplet’s picture

@tacituseu,

Are you a member of tesbot team? how do you obtain the debug message from d.org testbot?

tacituseu’s picture

@droplet: 'View results on dispatcher' then 'Build Artifacts', not a member

Status: Needs review » Needs work

The last submitted patch, 73: 2832672-72-css-selector.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
3.39 KB

Thanks @tacituseu!

Status: Needs review » Needs work

The last submitted patch, 76: 2832672-72-view-button.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
3.68 KB

very unstable :s

#73 JS one worked anyway.

tacituseu’s picture

Status: Needs review » Needs work

The last submitted patch, 79: 2832672-79-throbber-test.patch, failed testing.

michielnugter’s picture

What I can conclude from all the tests above is that it might be the case that it might be possible that the add button could not be clicked.

I've been looking at the behavior of the dialog in views and there is some animation going on when opening the dialog and iteracting with it.

I think the test should wait somehow for everything to finish moving and initializing before continueing the test. It seems the jQuery :animate doesn't catch this yet. I think a random wait of even 1 ms in javascript would fix this because of the way the javascript engine works.

Gone think about this.

droplet’s picture

Status: Needs work » Needs review
FileSize
3.17 KB
tacituseu’s picture

More guessing, 24 is just such a nice number, and it happens to be the .gif size.
Edit: doesn't look like it.

Status: Needs review » Needs work

The last submitted patch, 83: 2832672-83-more-throbber.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
3.59 KB

Problems come from dialog.position.js. To see if it's true.

EDIT:

Patch is failed but this is a NEW failing point.

The problem is really come from dialog.position.js. It displayed at half bottom part first, and then move to middle aligned with non-animated CSS.

During the movement, PhantomJS performed a click.

For now, as a temp workaround for random failture,
1. Commit Patch #73, the JS patch first.
2. [new issue] Introduce a '.is-loaded' class #82 (Or checking jQuery UI to see if there's already an option for that)
3. [new issue, must-do] Rewrite dialog.position.js for better performance.

Thanks ALL.

Status: Needs review » Needs work

The last submitted patch, 85: 2832672-83-no-extra-position.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review

I have an alternative solution which checks on the dialog beeing opened or not via javascript. I added a javascript file via a test module to enable this.

Might be needed to move the test module and rename it, for now it's to see what you think of this direction.

@droplet:
1: As this isn't a random failure currently occuring on the test-bot I'd say we should wait untill we have a solution we can all agree on. As long as we don't update the PhantomJS driver this test will not fail randomly.
2: I think I actually have that with this patch right? Using a different approach though..
3: If and when we rewrite something I'd say we should wait and do it in ES6, seems like wasted effort to do it earlier and it's not that bad currently I think.

michielnugter’s picture

Is it really this kind of a day... Patch now attached..

Status: Needs review » Needs work

The last submitted patch, 88: phantom-driver-2832672-87-25x.patch, failed testing.

michielnugter’s picture

FileSize
41.78 KB

Redid the update via composer, hope it works this time.

Question: shouldn't I only add composer.json and not the .lock?

michielnugter’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 90: phantom-driver-2832672-90-25x.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
41.43 KB

Third time's the charm?

Status: Needs review » Needs work

The last submitted patch, 93: phantom-driver-2832672-93-25x.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
42.31 KB

grrrrr

Status: Needs review » Needs work

The last submitted patch, 95: phantom-driver-2832672-95-25x.patch, failed testing.

droplet’s picture

1: As this isn't a random failure currently occuring on the test-bot I'd say we should wait untill we have a solution we can all agree on. As long as we don't update the PhantomJS driver this test will not fail randomly.

Really? So assertWaitOnAjaxRequest make the diff? Comment #7x ~ #8x of my patches are not upgrade the Mink driver.

2: I think I actually have that with this patch right? Using a different approach though..

We have custom event in dialog.position.js. the last event is .trigger('dialogContentResize');

Anyway, anyone knows how to enable SEVEN theme in D8 testcase? I wonder if testbot will fail with Seven or not.

Lendude’s picture

@droplet, something along these lines should work (taken from a webtest in #2701829: Extension objects should not implement \Serializable, so might not translate 1-1)

$themes = ['classy', 'seven', 'bartik'];

    /** @var \Drupal\Core\Extension\ThemeInstallerInterface $theme_installer */
    $theme_installer = $this->container->get('theme_installer');
    $theme_installer->install($themes);

    /** @var \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler */
    $theme_handler = $this->container->get('theme_handler');
    foreach ($themes as $theme) {
      $theme_handler->setDefault($theme);
      // Do stuff.......
droplet’s picture

Status: Needs work » Needs review
FileSize
3.16 KB

Status: Needs review » Needs work

The last submitted patch, 99: test-only-2832672-99.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
2.67 KB

I hope I'm wrong, we spend such long time for 1px diff. What does the bot say?

droplet’s picture

FileSize
438.67 KB

Damn! It passed! So that Mink PhantomJS or PhantomJs itself perform a click on boundary edges.

We should run JSTest on SEVEN theme instead that we have more human reviews on it. It will not shake.

tacituseu’s picture

@droplet according to debug log it first gets the bounding box of the link e.g. for a good run: {\"top\":313,\"right\":357,\"left\":146,\"bottom\":335,\"width\":211,\"height\":22}}") and then calculates middle point (QPoint(252,324)) and simulates mousedown/mouseup on those coordinates, but for bad run Y turns out at 300px and just before simulated click the link moves by 24px down, so it is no longer in that place.

Also observe in your video from #102, @0:09 just after the dialog pops up there is something funny going on with .ui-dialog-titlebar seems to have 2 bottom borders spread around 20 pixels apart, it disappears in the same frame the dropdown for 'Add/Or Rearrange' does.
Edit: looks like it is from: <div class="views-override clearfix form--inline views-offset-top" data-drupal-views-offset="top"></div>.

droplet’s picture

Ummm.. Unexplainable... Anyway, the video is captured from Chrome, with CPU throttled (20x slowdown).

The movement from the video less than 24px. And if it clicking on the middle point, it may not miss clicking. (Assume it will click on the element even when it's shaking since the big button on #78 is also working.)

And the only thing I found that about 24px is the padding of
.views-ui-dialog .scroll

.ui-dialog-titlebar added after Modal is shown:

$('body').once('viewsDialog').on('dialogContentResize.viewsDialog', '.ui-dialog-content', handleDialogResize);

And this event changed the CSS overflow also.

seems to have 2 bottom borders spread around 20 pixels apart

here: #2849628: Strange bar in View UI filters Modal when there are no filter options

tacituseu’s picture

@droplet: For 24px difference I meant in #67, the bounding box fits the screenshot from #14 (https://www.drupal.org/files/issues/filter.test.passes.jpg), your video looks different, maybe different browser or some changes in HEAD since.

Lendude’s picture

We should run JSTest on SEVEN theme instead that we have more human reviews on it. It will not shake.

Well if we really want comprehensive test coverage, we should run all BTB test (so including all JTB test) on all core themes, since the theme can have a big impact on things being clickable/seen/active.

But running them on Seven might not be a bad idea. No idea how much impact that would have on the test run times.

tacituseu’s picture

Attached are annotated relevant parts of the log from #66 + a picture from #14, in #67 I tried to test it by making button larger so it would be clicked at least on the edge even by bad click, but dialog.position.js compensated (results in #72) and moved the whole dialog.

1000

tacituseu’s picture

tacituseu’s picture

Got it! screenshots from testbot https://www.drupal.org/pift-ci-job/590102, also patch that produces them. Check /jenkins-drupal_patches-1127/artifacts/simpletest/ for the rest.
Creation of .jpg delays it enough to pass but it grabs the screen at the right moment to show what actually happens.
Edit: '-after-findbyid-views-add-group-link.jpg' should be really named '-after-waitForElementVisible-views-add-group-link.jpg' didn't want to re-roll/re-test just for that.

michielnugter’s picture

@tacituseu
1. Cool to learn some more about the test-bot and how to create screenshots. Thanks for that!
2. Very good to have proof that that the problem with the random fail is indeed the dialog positioning!

I created a follow-up patch on my previous attempt (interdiff is for that), I changed it a little to trigger after the event fired by the dialog resize. On my machine it passed 25x in a row. Let's see what the test-bot thinks of this and if it's actually a good solution, I'd love feedback on that.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/composer.lock
    @@ -994,7 +1168,7 @@
    -            "time": "2016-05-10T14:11:45+00:00"
    +            "time": "2016-05-10 14:11:45"
    

    unrelated composer.lock changes - please update your composer installation before running composer update.

  2. +++ b/core/misc/dialog/dialog.position.js
    @@ -59,6 +59,8 @@
    +
    +    console.log(event);
    

    debug statement that should be removed?

  3. +++ b/core/modules/system/tests/modules/dialog_test/dialog_test.libraries.yml
    @@ -0,0 +1,7 @@
    +    - core/drupal
    \ No newline at end of file
    

    missing newline at the end of file

  4. +++ b/core/modules/system/tests/modules/dialog_test/js/dialog.debug.js
    @@ -0,0 +1,23 @@
    +  // Register the aftercreate to the dialog has opened.
    +  $('#drupal-modal').on('dialogContentResize', function (event) {
    +    Drupal.dialog_debug.has_open_dialog = true;
    +  });
    +  // Register the beforecreate and afterclose to indicate the dialog has been closed.
    +  $(window).on('dialog:beforecreate dialog:afterclose', function () {
    +    Drupal.dialog_debug.has_open_dialog = false;
    +  });
    

    what is an "aftercreate"? the code is not doing any "aftercreate"?

  5. +++ b/core/modules/views_ui/tests/src/FunctionalJavascript/FilterCriteriaTest.php
    @@ -14,7 +14,7 @@ class FilterCriteriaTest extends JavascriptTestBase {
    -  public static $modules = ['node', 'views', 'views_ui'];
    +  public static $modules = ['node', 'views', 'views_ui', 'dialog_test'];
    

    wait - so we can only test this if we enable a special dialog_test module? That seems wrong, why can't we come up with a test case that always works? Or fix Views UI instead so that a simple link does not jump around unexpectedly?

tacituseu’s picture

The dialog.position.js::resetSize() might be getting triggered because as part of .click() simulation PhantomJS does scrollIntoView() on #views-add-group-link, so it might be a race within .click() on client side with 20ms debounce on resetSize()

var autoResize = debounce(resetSize, 20);
Mixologic’s picture

wow. I didnt even know we could take screenshots with the existing testbots. But Im glad I decided to grab the simpletest directory as an artifact just in case people needed it.

michielnugter’s picture

@klausi:

1, 2, 3 and 4:
Sorry about that, fixed in this patch.

5:
It's not Views UI but the dialog implementation that's causing this. After opening a dialog a resize / position is triggered to make sure the dialog is viewed in the center of the page. This is triggered debounced on window resize which in turn is triggered on dialog creation. (dialog.position.js lines 92-109).

The code/implementation is not wrong, it has it's use. I don't know if the repositioning/resize can be done in css but I think a javascript solution will be required.

I couldn't think of a way to be able to wait on all resize events to be done without the test module. It can't be done with a single line of javascript because the resize is debounced a wait on event is not enough.

The only alternative to the test module is adding a css class after the dialog is finished as @droplet did in #82.

So to make the dialog opened state testable we either need to:
- Change the core javascript and add the class (#82)
- Add the test module
- I can't think of another solution but would love to see suggestions :)

michielnugter’s picture

Priority: Normal » Major

Setting to Major, allthough I'm not 100% sure. The dialog problem and random fails occured in another issue for me:

See:
https://www.drupal.org/node/2846428#comment-11938505

We need to fix the random error, should be split this off into a seperate issue?

michielnugter’s picture

Priority: Major » Normal
Status: Needs review » Postponed

I created a separate issue for the dialog assertion change as it could be relavant for other issues (webdriver) as well.

Postponed this issue untill #2856047: Avoid random failures in JavascriptTestBase when testing functionality in a dialog is in.

jibran’s picture

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

Title: Upgrade jcalderonzumba/mink-phantomjs-driver for better test performance » Upgrade jcalderonzumba/* for better test performance
Issue summary: View changes
Status: Postponed » Needs work
jibran’s picture

Title: Upgrade jcalderonzumba/* for better test performance » [PP-1] Upgrade jcalderonzumba/* for better test performance
Status: Needs work » Needs review
FileSize
3.74 KB
10.07 KB

Here is a new patch after removing #2856047: Avoid random failures in JavascriptTestBase when testing functionality in a dialog bits also uploaded a combined patch.

The last submitted patch, 120: upgrade-2832672-120.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 120: combine-upgrade-2832672-120.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

We need to update at least the driver urgently for PHP 7.2 support - see #2929477: Update jcalderonzumba/mink-phantomjs-driver

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 125: 2832672-125.patch, failed testing. View results

jibran’s picture

martin107’s picture

martin107’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 128: 2832672-128.patch, failed testing. View results

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

Status: Needs work » Closed (outdated)

Since we use Webdriver now, I'm pretty sure we don't care about upgrading these anymore.

But many thanks for the excellent research done here!