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

E2E Tests: Updates the wait function name to discourage its usage #6932

Merged
merged 3 commits into from May 30, 2018

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented May 24, 2018

related #6822 (review)

@@ -107,6 +107,6 @@ export async function pressWithModifier( modifier, key ) {
*
* @return {Promise} Promise resolving after the given timeout
*/
export async function wait( timeout ) {
export async function waitDurationYesImCertainWaitForSelectorIsNotABetterOption( timeout ) {

This comment has been minimized.

@youknowriad

youknowriad May 24, 2018

Contributor

I wonder if we could just name it waitForAnimation (that's the current use-case) Or maybe just leave it as wait and add a comment.

@youknowriad youknowriad self-assigned this May 24, 2018

@youknowriad youknowriad requested a review from aduth May 24, 2018

@aduth

This comment has been minimized.

Member

aduth commented May 24, 2018

Hah, my example name was maybe more exaggerated than it ought need to be, though its purpose is of a similar vein to that of dangerouslySetInnerHTML.

I wonder if we could just name it waitForAnimation (that's the current use-case)

This is what I'd be curious to know; if we can identify these delays as necessary for certain behaviors like animations, I think such a name would be ideal (even better if the duration were not necessary, though I expect difficult to implement).

@noisysocks

This comment has been minimized.

Member

noisysocks commented May 29, 2018

Why do define our own wait function? Puppeteer has page.waitFor() which can accept a timeout number.

(But yeah, we should discourage its usage.)

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 29, 2018

@noisysocks Actually, I missed this one

@aduth

This comment has been minimized.

Member

aduth commented May 29, 2018

Could we set ESLint to at least warn of its use?

@aduth

aduth approved these changes May 30, 2018

Nice 👍 Thanks

@youknowriad youknowriad merged commit aedd474 into master May 30, 2018

2 checks passed

codecov/project 46.46% remains the same compared to 29092ae
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/wait-name branch May 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment