search

Automattic / wp-e2e-tests

Automated end-to-end tests for WordPress.com
https://github.com/Automattic/wp-calypso
GNU General Public License v2.0
110 stars 20 forks source link

Updating `dismissAllAlerts` to use async/await style #1768

Closed JavonDavis closed 5 years ago

JavonDavis commented 5 years ago

Small update to dismissAllAlerts to use async/await.

alisterscott commented 5 years ago

Thanks for the PR @JavonDavis

I tried running a gutenberg editor spec when adding a fail statement within a single spec in the file, and it didn't work as the alert remained open

I then tried the same thing on master and it also didn't work 🤔

I was thinking we could try an approach I've used previously where I've run JavaScript to override the beforeunload event in the browser to not show the alert, however it didn't seem to work in Calypso 🤔

What do you think about such an approach? In ruby it looked something like:

browser.execute_script('window.onbeforeunload = null')

JavonDavis commented 5 years ago

I tried running a gutenberg editor spec when adding a fail statement within a single spec in the file, and it didn't work as the alert remained open

I then tried the same thing on master and it also didn't work 🤔

I was thinking we could try an approach I've used previously where I've run JavaScript to override the beforeunload event in the browser to not show the alert, however it didn't seem to work in Calypso 🤔

What do you think about such an approach? In ruby it looked something like:

browser.execute_script('window.onbeforeunload = null')

@alisterscott this gave me a pretty hard time today I tried different approaches, including the one you suggested with no luck... I then realised in this case we're dismissing the alert when to actually leave the page we'd need to accept the alert, for this to work I had to replace dismissAllAlerts with acceptAllAlerts and accept the alert if present in the after step of the signup specs. I'm not sure if there'll be any adverse effects to the change from dismiss to alert in mocha-hooks but I'll check that out in CI.

alisterscott commented 5 years ago

I then realised in this case we're dismissing the alert when to actually leave the page we'd need to accept the alert,

It drove me crazy yesterday too. And oh wow, I didn't even think of that. Good work figuring that out.

JavonDavis commented 5 years ago

wp-calypso-gutenberg-post-editor-spec.js seems to be passing in CI but is failing locally for me, @Automattic/flowpatrol anyone else experiencing this? In any case I think it's unrelated to this change and can be handled in a separate PR, thoughts on merging this?

Stojdza commented 5 years ago

@JavonDavis It fails for me locally as well. I suppose we are good to go with these changes 👍