Remove and address sleep statement in wp-admin-jetpack-page.js
#1739
Conversation
juliaamosova
added
[Status] Needs Jetpack e2e Testing
| @@ -4,6 +4,9 @@ import { By } from 'selenium-webdriver'; | |||
|
|
|||
| import * as driverHelper from '../../driver-helper'; | |||
| import AsyncBaseContainer from '../../async-base-container'; | |||
| import config from 'config'; | |||
|
|
|||
| const explicitWaitMS = config.get( 'explicitWaitMS' ); | |||
There was a problem hiding this comment.
explicitWaitMS is available inside any PageObject via this.explicitWaitMS: https://github.com/Automattic/wp-e2e-tests/blob/master/lib/async-base-container.js#L22
There was a problem hiding this comment.
Noted, thanks @brbrr 👍
| @@ -13,8 +16,23 @@ export default class WPAdminJetpackPage extends AsyncBaseContainer { | |||
|
|
|||
| async connectWordPressCom() { | |||
| const selector = By.css( 'a.jp-jetpack-connect__button' ); | |||
| const pauseBetweenFocusAttemptsMS = 200; | |||
| const connectJetpackButtonFocus = | |||
| "document.getElementsByClassName( 'jp-jetpack-connect__button')[0].focus();"; | |||
There was a problem hiding this comment.
hm, I'm not quite sure what the problem you trying to fix here, but the connection button will be changed: Automattic/jetpack#8618.
Is the issue you trying to fix is somehow related to the fact that button not doing what it should right after page load? It might be a good idea to fix this on Jetpack side instead.
There was a problem hiding this comment.
Is the issue you trying to fix is somehow related to the fact that button not doing what it should right after page load? It might be a good idea to fix this on Jetpack side instead.
I am trying to remove and address the sleep statement. It was originally added here with no explanation why:
https://github.com/Automattic/wp-e2e-tests/pull/584/files#diff-e88aeb376f9b63bf70f5877bdece3966R14
My guess is that the test runs too fast and clicks on the button when it is not ready to be clicked on yet. I was not able to reproduce it locally though.
Since you mentioned the button will be changed, it makes sense to see if the issue will be present after the change takes affect. What do you think?
| .switchTo() | ||
| .activeElement() | ||
| .getId(); | ||
| let connectJetpackButtonId = await ( await this.driver.findElement( selector ) ).getId(); |
There was a problem hiding this comment.
are these params around findElement is needed? I think chained call with single await should do the job
There was a problem hiding this comment.
You are right, single await would do the job here. Thanks for catching it!
| await driverHelper.waitTillPresentAndDisplayed( this.driver, selector ); | ||
| await this.driver.sleep( 1000 ); | ||
| for ( let i = 0; i < explicitWaitMS / pauseBetweenFocusAttemptsMS; i++ ) { |
There was a problem hiding this comment.
Is this for loop might be replaced with driver.wait function? They seem like quite similar
|
Closing this PR since The situatuation around |
Removed and addressed
sleepstatement with aforloop that checks if theConnect Jetpackbutton ID matches with the currently active element ID (element in focus).I used the JavaScriptExecutor to get the focus on the button - as far as I know, there is no other way to do it using webdriver. JavaScriptExecutor is not ideal because it doesn't return anything (or rather returns
undefined) but it does the job when the action needs to be made.I also looked at doing the following:
Rewrite existing
waitTillFocusedfunction but I didn't want to use JavaScriptExecutor in it (for the reason described above) so I passed on that option;Make a separate function out of the loop I wrote but the JavaScriptExecutor stopped me again. It gets the element using
getElementsByClassNamewhich can not always be applied to all elements. I passed on this option as well.After considering the 2 options above, I decided to leave the loop as one-time use.
To test:
The change affects the following specs:
wp-jetpack-onboarding-spec.jswp-jetpack-connect-spec.jsconnect-disconnect-spec.jsjetpack-connect-flow.jswhich is a part ofgutenberg-markdown-block-spec.js