Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a plugin to disable the CSS animation for more stable e2e tests #13769

Merged
merged 1 commit into from
Feb 8, 2019

Conversation

youknowriad
Copy link
Contributor

Tries to address this #13739 by disabling the CSS animations in e2e tests.

@youknowriad youknowriad added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Feb 8, 2019
@youknowriad youknowriad self-assigned this Feb 8, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering what Travis thinks about this idea. I'm personally fine with the proposed tweak as it makes everything easier.

* Enqueue CSS stylesheet disabling animations.
*/
function enqueue_disable_animations_stylesheet() {
$custom_css = '* { animation-duration: 1ms !important; }';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's aggressive but I like it :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why does this have to be 1ms and not 0? It seems we leave open the potential for race conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me if we do 0 it means we disable animations entirely and 1ms is saying the animation are there (to match prod) but are very fast. Not sure if it's a great argument though :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems to me like the worst of both worlds though, we're not really testing the animation nor are we disabling them entirely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #13779. 0ms seems to be even better in my testing.

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 8, 2019
@youknowriad
Copy link
Contributor Author

It seems to work well aside for the "nux" test, I'll test it.

@youknowriad
Copy link
Contributor Author

I think this test is unstable regardless of the animations

@@ -154,6 +155,7 @@ beforeAll( async () => {

await trashExistingPosts();
await setupBrowser();
await activatePlugin( 'gutenberg-test-plugin-disables-the-css-animations' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be placed inside mu-plugins instead? We have there one plugin already:

https://github.com/WordPress/gutenberg/blob/master/packages/e2e-tests/mu-plugins/disable-login-autofocus.php

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although, it might be a bad idea as it would change the behavior when using Docker outside of e2e context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was exactly my thinking :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we disable this line in docker config:
https://github.com/WordPress/gutenberg/blob/master/docker-compose.yml#L16

Sorry about all notifications :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the e2e tests use this docker config though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the e2e tests use a different container but when you run them locally, we often run them on the default container to test. (that's what I do), and they'll fail because animations are enabled there.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the proposed solution. @aduth any thoughts before we proceed?

@aduth
Copy link
Member

aduth commented Feb 8, 2019

I'm happy with anything which gets rid of waitFor* and speeds up tests.

@gziolo gziolo merged commit 50ae68c into master Feb 8, 2019
@gziolo gziolo deleted the try/disable-animations-e2e-tests branch February 8, 2019 14:22
@gziolo
Copy link
Member

gziolo commented Feb 8, 2019

I'm merging it into master and I will open follow-ups if we find any issue as they pop up.

@aduth
Copy link
Member

aduth commented Feb 8, 2019

If we can change it to 0, we should change it to 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants