Add title to WaitTask for debugging purposes #2084

felixfbecker · 2018-02-23T09:32:29Z

Closes #2037

googlebot · 2018-02-23T09:32:32Z

Thanks for your pull request. t looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

felixfbecker · 2018-02-23T09:34:20Z

I signed it!

googlebot · 2018-02-23T09:34:23Z

CLAs look good, thanks!

aslushnikov · 2018-02-23T18:40:57Z

lib/FrameManager.js

    this._polling = polling;
    this._timeout = timeout;
    this._predicateBody = helper.isString(predicateBody) ? 'return ' + predicateBody : 'return (' + predicateBody + ')(...args)';
+    this._title = title;


there's no need to save the title

There is also no need to save this._timeout, but it's helpful when using a debugger to see what the WaitTask does

this._timeout is used in rerun() method.

but it's helpful when using a debugger to see what the WaitTask does

I don't think it's a big of a deal: it's easy to save title once the debugging need occurs.

aslushnikov · 2018-02-23T18:41:35Z

lib/FrameManager.js

  /**
   * @param {!Frame} frame
   * @param {Function|string} predicateBody
+   * @param {string} [title] what is being waited for (used in timeout error message)


nit: drop the comment and square braces around title

* @param {string} title

The square brackets mean it's optional

Optional parameters are marked with = sign, e.g.:

* @param {string=} title

However, I don't think it should be optional.

I was looking at https://esdoc.org/manual/tags.html#optional-and-default

/** * @param {number} [param] - this is optional param. */ function myFunc(param){...} /** * @param {number} [param=10] - this is default param. */ function myFunc(param){...}

We use closure annotations, there's a link to the manual in CONTRIBUTING.md

Check out optionals in closure annotations: https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#optional-parameter-in-a-param-annotation

aslushnikov · 2018-02-23T18:42:19Z

lib/FrameManager.js

    const timeout = helper.isNumber(options.timeout) ? options.timeout : 30000;
    const polling = options.polling || 'raf';
-    return new WaitTask(this, pageFunction, polling, timeout, ...args).promise;
+    return new WaitTask(this, pageFunction, options.title || 'function', polling, timeout, ...args).promise;


let's not add options.title; this will be an outlier in our api. Just passing in "function", or maybe stringified function body is enough.

Then the core use case would not be solved, which was waitForSelector, which calls into waitForFunction internally.

An alternative approach would be to catch every error and prepend the error message. Would you prefer that?

I see. I'd prefer copying the three lines of waitForFunction in __waitForSelectorOrXPath method.

aslushnikov · 2018-02-23T18:43:49Z

lib/FrameManager.js

    // Since page navigation requires us to re-install the pageScript, we should track
    // timeout on our end.
-    this._timeoutTimer = setTimeout(() => this.terminate(new Error(`waiting failed: timeout ${timeout}ms exceeded`)), timeout);
+    this._timeoutTimer = setTimeout(() => this.terminate(new Error(`waiting ${title ? 'for ' + title : ''} failed: timeout ${timeout}ms exceeded`)), timeout);


i'd expect title to be always present, so no need for ternary operator

aslushnikov · 2018-02-23T19:26:18Z

@felixfbecker thanks for working on this; it should help a lot to debug pptr script failures.

aslushnikov · 2018-03-15T18:54:39Z

@felixfbecker Are still interested in landing this?

felixfbecker · 2018-03-15T19:03:18Z

Yes, just didn't find the time to update it yet.

yanivefraim · 2018-03-24T09:14:53Z

@felixfbecker - will be happy to help here, if you don’t have time..
@aslushnikov

aslushnikov · 2018-03-29T17:46:52Z

@yanivefraim please do! I'd love to have this.

yanivefraim · 2018-03-29T19:23:49Z

@yanivefraim please do! I'd love to have this.

@felixfbecker - is it ok if I will start a new PR?

aslushnikov · 2018-03-30T19:41:47Z

Closing in favor of #2292

felixfbecker force-pushed the timeout-error-message branch from 27a96e8 to 6625b3c Compare February 23, 2018 09:37

Add title to WaitTask for debugging purposes

f5b4d03

felixfbecker force-pushed the timeout-error-message branch from 6625b3c to f5b4d03 Compare February 23, 2018 09:43

aslushnikov reviewed Feb 23, 2018

View reviewed changes

yanivefraim mentioned this pull request Mar 30, 2018

feat(FrameManage): add title for WaitTask #2292

Merged

aslushnikov closed this Mar 30, 2018

felixfbecker deleted the timeout-error-message branch April 2, 2018 20:53

Add title to WaitTask for debugging purposes #2084

Add title to WaitTask for debugging purposes #2084

Uh oh!

Conversation

felixfbecker commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Feb 23, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

felixfbecker commented Feb 23, 2018

Uh oh!

googlebot commented Feb 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixfbecker Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aslushnikov commented Feb 23, 2018

Uh oh!

aslushnikov commented Mar 15, 2018

Uh oh!

felixfbecker commented Mar 15, 2018

Uh oh!

yanivefraim commented Mar 24, 2018

Uh oh!

aslushnikov commented Mar 29, 2018

Uh oh!

yanivefraim commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aslushnikov commented Mar 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

felixfbecker commented Feb 23, 2018 •

edited

Loading

felixfbecker Feb 23, 2018 •

edited

Loading

yanivefraim commented Mar 29, 2018 •

edited

Loading