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

feat(page): add an option to disable timeout for waitForFunction #2252

Merged
merged 4 commits into from Mar 28, 2018

Conversation

yanivefraim
Copy link
Contributor

closes #2200

Maybe we should consider adding this feature to all available timeouts? (waitForSelector, waitForXPath etc.)

@@ -145,6 +145,12 @@ module.exports.addTests = function({testRunner, expect}) {
await page.evaluate(element => element.remove(), div);
await waitForFunction;
});
it('should disable timeout when its set to 0', async({page}) => {
let error = null;
const res = await page.waitForFunction(() => new Promise(res => setTimeout(() => res(42), 1000)), {timeout: 0}).catch(e => error = e);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's do it 100ms to have less impact on test time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, done...

@aslushnikov
Copy link
Contributor

@yanivefraim we're figuring the typescript issues; seems to be related to today's ts release.

@aslushnikov
Copy link
Contributor

Maybe we should consider adding this feature to all available timeouts? (waitForSelector, waitForXPath etc.)

@yanivefraim by the way, this patch just did this for waitForSelector and waitForXPath. Can you please update documentation to mention this?

@yanivefraim
Copy link
Contributor Author

Maybe we should consider adding this feature to all available timeouts? (waitForSelector, waitForXPath etc.)

@yanivefraim by the way, this patch just did this for waitForSelector and waitForXPath. Can you please update documentation to mention this?

@aslushnikov - Good catch (I thought about it but forgot to update docs). I updated the docs accordingly.
BTW - Do you think we should add tests for those as well?

@@ -1406,7 +1406,7 @@ Shortcut for [page.mainFrame().waitForSelector(selector[, options])](#framewaitf
- `options` <[Object]> Optional waiting parameters
- `visible` <[boolean]> wait for element to be present in DOM and to be visible, i.e. to not have `display: none` or `visibility: hidden` CSS properties. Defaults to `false`.
- `hidden` <[boolean]> wait for element to not be found in the DOM or to be hidden, i.e. have `display: none` or `visibility: hidden` CSS properties. Defaults to `false`.
- `timeout` <[number]> maximum time to wait for in milliseconds. Defaults to `30000` (30 seconds).
- `timeout` <[number]> maximum time to wait for in milliseconds. Defaults to `30000` (30 seconds). Pass `0` to disable timeout.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please change frame.waitForSelector and frame.waitForXPath as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, sorry (missed those). done...

@aslushnikov
Copy link
Contributor

BTW - Do you think we should add tests for those as well?

@yanivefraim I think we're good

@aslushnikov aslushnikov merged commit abb05e0 into puppeteer:master Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

page.waitForFunction does not support disable timeout
2 participants