Allow options.timeout to be `0` to disable timeout #887

sukrosono · 2017-09-26T17:34:30Z

Intended to close #782 , sorry if this one look really premature.

every feature should be accompanied by a test
are we count this one count as feature? if yes is this correct way to test it?

Clarification:
the puppeteer launch options is documented accept 0 to disable timeout. Is #782 only match goto options?

i already test with fit with 100 sec, and it pass.

on my windows machine test 1 spec always fail and that is:

Page Network Events Page.Events.Response should not report body unless request is finished

so i just rely on travis

documentation:
should i also change page.goBack(options) , page.goForward(options) ? let me know if i miss some

aslushnikov

yay, thanks for the pull request!

aslushnikov · 2017-09-27T09:59:30Z

lib/NavigatorWatcher.js

@@ -40,8 +40,10 @@ class NavigatorWatcher {

    this._eventListeners = [];

-    const watchdog = new Promise(fulfill => this._maximumTimer = setTimeout(fulfill, this._timeout))
-        .then(() => 'Navigation Timeout Exceeded: ' + this._timeout + 'ms exceeded');
+    const watchdog = new Promise(fulfill => {


this is a little tricky: there should be no watchdog if there's no timeout timer.

I'd rather do:

const navigationPromises = []; if (this._timeout) { const watchdog = new Promise(fulfill => { // ... }); navigationPromises.push(watchdog); }

aslushnikov · 2017-09-27T10:04:19Z

test/test.js

@@ -619,6 +619,16 @@ describe('Page', function() {
      expect(error.message).toContain('Navigation Timeout Exceeded: 1ms');
      process.removeListener('unhandledRejection', unhandledRejectionHandler);
    }));
+    it('should work when option.timeout set to 0', SX(async function() {


this test will run for quite some time - which is unfortunate.
Developer's time is precious! :)

I'd rather make sure that timeout: 0 doesn't set a zero timeout watchdog:

it('should disable timeout when its set to 0', SX(async function() { let error = null; await page.goto(PREFIX + '/grid.html', {timeout: 0}).then(e => error = e); expect(error).toBe(null); }));

aslushnikov · 2017-09-27T10:06:29Z

docs/api.md

@@ -594,7 +594,7 @@ Navigate to the next page in history.
 #### page.goto(url, options)
 - `url` <[string]> URL to navigate page to. The url should include scheme, e.g. `https://`.
 - `options` <[Object]> Navigation parameters which might have the following properties:
-  - `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds.
+  - `timeout` <[number]> Maximum navigation time in milliseconds, defaults to 30 seconds, pass `0` to disable timeout.


There's a bunch of other methods that rely on NavigationWatcher, so you should add this clarification there as well:

page.goForward() page.boBack(); page.reload(); page.waitForNavigation()

sukrosono · 2017-09-28T08:30:31Z

test/test.js

+      let error = null;
+      page.goto(PREFIX + '/empty.html', {timeout: 0}).catch(e => error = e);
+      // const sleep = e => new Promise(ff => setTimeout(ff, e));
+      // await sleep(90000);


Developer's time is precious! :)

agree, sorry but leave it as comment. Perhaps someone want to inspect this.

Please, drop these comments: they are not useful for other puppeteer developers.

sukrosono · 2017-09-28T08:44:50Z

lib/NavigatorWatcher.js

+      const watchdog = new Promise(fulfill => this._maximumTimer = setTimeout(fulfill, this._timeout))
+          .then(() => 'Navigation Timeout Exceeded: ' + this._timeout + 'ms exceeded');
+      navigationPromises.push(watchdog);
+    }


this is as the @aslushnikov suggestion, it's simple and clear code. But it think it still have problem if user pass timeout 0 as a string. I'd like add more conditional check, or we just rely on the documentation?

@brutalcrozt yes, we should rely on documentation in this case and expect user to pass in a number. There's also a check that timeout is always a number: https://github.com/GoogleChrome/puppeteer/blob/c46c41d89caefd1e1951055fb041521a2bcc933b/lib/NavigatorWatcher.js#L28

sukrosono · 2017-09-28T09:06:09Z

Clarification:

the puppeteer launch options is documented accept 0 to disable timeout.

my question:

is it same the page options and puppeteer options?
is it completely different thing? what's the relation?

as the puppeteer options which first documented as accept 0 to disable timeout. I can't see it's implementation, i hope just i who miss it. Also when i look up Puppeteer, Browser and Launcher class, that class not importing NavigationWatcher. Sorry dude if i just to noisy 😄

aslushnikov

thanks for working on this! Left a few more comments before we can merge this.

aslushnikov · 2017-09-29T19:26:03Z

lib/NavigatorWatcher.js

+      const watchdog = new Promise(fulfill => this._maximumTimer = setTimeout(fulfill, this._timeout))
+          .then(() => 'Navigation Timeout Exceeded: ' + this._timeout + 'ms exceeded');
+      navigationPromises.push(watchdog);
+    }


@brutalcrozt yes, we should rely on documentation in this case and expect user to pass in a number. There's also a check that timeout is always a number: https://github.com/GoogleChrome/puppeteer/blob/c46c41d89caefd1e1951055fb041521a2bcc933b/lib/NavigatorWatcher.js#L28

aslushnikov · 2017-09-29T19:26:50Z

test/test.js

+    it('should disable timeout when its set to 0', SX(async function() {
+      server.setRoute('/empty.html', (req, res) => {});
+      let error = null;
+      page.goto(PREFIX + '/empty.html', {timeout: 0}).catch(e => error = e);


await is missing:

await page.goto(PREFIX + '/empty.html', {timeout: 0}).catch(e => error = e);

aslushnikov · 2017-09-29T19:28:13Z

test/test.js

+      let error = null;
+      page.goto(PREFIX + '/empty.html', {timeout: 0}).catch(e => error = e);
+      // const sleep = e => new Promise(ff => setTimeout(ff, e));
+      // await sleep(90000);


Please, drop these comments: they are not useful for other puppeteer developers.

aslushnikov · 2017-09-29T19:28:26Z

test/test.js

+      // const sleep = e => new Promise(ff => setTimeout(ff, e));
+      // await sleep(90000);
+      expect(error).toBe(null);
+    }));// 100000


nit: please drop the comment here as well

aslushnikov · 2017-09-29T22:19:02Z

is it same the page options and puppeteer options?
is it completely different thing? what's the relation?

These are different timeouts. One is a navigation timeout, another one is a browser launcher timeout.

sukrosono · 2017-09-30T03:21:24Z

https://github.com/GoogleChrome/puppeteer/blob/c46c41d89caefd1e1951055fb041521a2bcc933b/lib/Launcher.js#L177

Ok this what i looking for, i just miss it. Thanks for the review, thank for your cooperation 😄

Allow options.timeout to be 0 to disable timeout

fa81395

aslushnikov reviewed Sep 27, 2017

View reviewed changes

sukrosono changed the title ~~Allow options.timeout to be 0 to disable timeout~~ [WIP] Allow options.timeout to be 0 to disable timeout Sep 27, 2017

update related documentation

29ac71d

sukrosono commented Sep 28, 2017

View reviewed changes

aslushnikov reviewed Sep 29, 2017

View reviewed changes

drop unnecessary comment

c8b3b55

change testing spec to grid.html instead of empty.html

848aebb

sukrosono changed the title ~~[WIP] Allow options.timeout to be 0 to disable timeout~~ Allow options.timeout to be 0 to disable timeout Sep 30, 2017

aslushnikov approved these changes Sep 30, 2017

View reviewed changes

aslushnikov merged commit 53531c9 into puppeteer:master Sep 30, 2017

vsemozhetbyt mentioned this pull request Oct 6, 2017

the timeout option on the goTo command can't be disabled #965

Closed

alex-kuck mentioned this pull request Oct 11, 2017

Configurable Timeouts awesome-inc/puppeteer-mocha#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow options.timeout to be `0` to disable timeout #887

Allow options.timeout to be `0` to disable timeout #887

sukrosono commented Sep 26, 2017 •

edited

aslushnikov left a comment

aslushnikov Sep 27, 2017

aslushnikov Sep 27, 2017

aslushnikov Sep 27, 2017

sukrosono Sep 28, 2017

aslushnikov Sep 29, 2017

sukrosono Sep 28, 2017 •

edited

aslushnikov Sep 29, 2017

sukrosono commented Sep 28, 2017

aslushnikov left a comment

aslushnikov Sep 29, 2017

aslushnikov Sep 29, 2017

aslushnikov Sep 29, 2017

aslushnikov Sep 29, 2017

sukrosono Sep 30, 2017

aslushnikov commented Sep 29, 2017

sukrosono commented Sep 30, 2017

Allow options.timeout to be 0 to disable timeout #887

Allow options.timeout to be 0 to disable timeout #887

Conversation

sukrosono commented Sep 26, 2017 • edited

aslushnikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukrosono Sep 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukrosono commented Sep 28, 2017

Clarification:

aslushnikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslushnikov commented Sep 29, 2017

sukrosono commented Sep 30, 2017

Allow options.timeout to be `0` to disable timeout #887

Allow options.timeout to be `0` to disable timeout #887

sukrosono commented Sep 26, 2017 •

edited

sukrosono Sep 28, 2017 •

edited