Skip to content
This repository has been archived by the owner on Feb 18, 2021. It is now read-only.

Remove and address sleep statement in wp-admin-jetpack-page.js #1739

Closed
wants to merge 2 commits into from
Closed

Remove and address sleep statement in wp-admin-jetpack-page.js #1739

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b6e3ea
Remove and address `sleep` statement in `wp-admin-jetpack-page.js`
juliaamosova Dec 26, 2018
dc81f3e
Update the name of the `pauseBetweenFocusAttempts` variable
juliaamosova Dec 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion lib/pages/wp-admin/wp-admin-jetpack-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' );
Copy link
Contributor

Choose a reason for hiding this comment

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

explicitWaitMS is available inside any PageObject via this.explicitWaitMS: https://github.com/Automattic/wp-e2e-tests/blob/master/lib/async-base-container.js#L22

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, thanks @brbrr 👍


export default class WPAdminJetpackPage extends AsyncBaseContainer {
constructor( driver ) {
Expand All @@ -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();";
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

await driverHelper.waitTillPresentAndDisplayed( this.driver, selector );
await this.driver.sleep( 1000 );
for ( let i = 0; i < explicitWaitMS / pauseBetweenFocusAttemptsMS; i++ ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for loop might be replaced with driver.wait function? They seem like quite similar

await this.driver.executeScript( connectJetpackButtonFocus );
let currentActiveElementId = await this.driver
.switchTo()
.activeElement()
.getId();
let connectJetpackButtonId = await ( await this.driver.findElement( selector ) ).getId();
Copy link
Contributor

Choose a reason for hiding this comment

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

are these params around findElement is needed? I think chained call with single await should do the job

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, single await would do the job here. Thanks for catching it!


if ( connectJetpackButtonId === currentActiveElementId ) {
break;
}
await this.driver.sleep( pauseBetweenFocusAttemptsMS );
}
return await driverHelper.clickWhenClickable( this.driver, selector );
}

Expand Down