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.

Sign up for GitHub

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

Jump to bottom

feat(FrameManage): add title for WaitTask #2292

Merged
merged 1 commit into from
Mar 30, 2018
Merged

feat(FrameManage): add title for WaitTask #2292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 5 additions & 4 deletions lib/FrameManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,7 @@ class Frame {
waitForFunction(pageFunction, options = {}, ...args) {
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, 'function', polling, timeout, ...args).promise;
}

/**
Expand All @@ -696,7 +696,8 @@ class Frame {
const waitForVisible = !!options.visible;
const waitForHidden = !!options.hidden;
const polling = waitForVisible || waitForHidden ? 'raf' : 'mutation';
return this.waitForFunction(predicate, {timeout: options.timeout, polling}, selectorOrXPath, isXPath, waitForVisible, waitForHidden);
const timeout = helper.isNumber(options.timeout) ? options.timeout : 30000;
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 a drive-by? Looks like an unrelated bugfix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it is related to this comment:

#2084 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Your comment link didn't work for me.

But if I understand correctly, the page.waitForSelector/page.waitForXPath don't currently have default timeout 30 seconds (despite documentation claiming so), and this change brings it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, created it from mobile (it is working on mobile... strange)

The comment was: #2084 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see now. Thank you for the explanation.

return new WaitTask(this, predicate, `${isXPath ? 'XPath' : 'selector'} "${selectorOrXPath}"`, polling, timeout, selectorOrXPath, isXPath, waitForVisible, waitForHidden).promise;

/**
* @param {string} selectorOrXPath
Expand Down Expand Up @@ -769,7 +770,7 @@ class WaitTask {
* @param {number} timeout
* @param {!Array<*>} args
*/
constructor(frame, predicateBody, polling, timeout, ...args) {
constructor(frame, predicateBody, title, polling, timeout, ...args) {
if (helper.isString(polling))
console.assert(polling === 'raf' || polling === 'mutation', 'Unknown polling option: ' + polling);
else if (helper.isNumber(polling))
Expand All @@ -791,7 +792,7 @@ class WaitTask {
// Since page navigation requires us to re-install the pageScript, we should track
// timeout on our end.
if (timeout)
this._timeoutTimer = setTimeout(() => this.terminate(new Error(`waiting failed: timeout ${timeout}ms exceeded`)), timeout);
this._timeoutTimer = setTimeout(() => this.terminate(new Error(`waiting for ${title} failed: timeout ${timeout}ms exceeded`)), timeout);
this.rerun();
}

Expand Down
14 changes: 13 additions & 1 deletion test/frame.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,12 @@ module.exports.addTests = function({testRunner, expect}) {
await page.evaluate(element => element.remove(), div);
await waitForFunction;
});
it('should respect timeout', async({page}) => {
let error = null;
await page.waitForFunction('false', {timeout: 10}).catch(e => error = e);
expect(error).toBeTruthy();
expect(error.message).toContain('waiting for function failed: timeout');
});
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), 100)), {timeout: 0}).catch(e => error = e);
Expand Down Expand Up @@ -292,7 +298,7 @@ module.exports.addTests = function({testRunner, expect}) {
let error = null;
await page.waitForSelector('div', {timeout: 10}).catch(e => error = e);
expect(error).toBeTruthy();
expect(error.message).toContain('waiting failed: timeout');
expect(error.message).toContain('waiting for selector "div" failed: timeout');
});

it('should respond to node attribute mutation', async({page, server}) => {
Expand All @@ -318,6 +324,12 @@ module.exports.addTests = function({testRunner, expect}) {
const waitForXPath = page.waitForXPath('//p[normalize-space(.)="hello world"]');
expect(await page.evaluate(x => x.textContent, await waitForXPath)).toBe('hello world ');
});
it('should respect timeout', async({page}) => {
let error = null;
await page.waitForXPath('//div', {timeout: 10}).catch(e => error = e);
expect(error).toBeTruthy();
expect(error.message).toContain('waiting for XPath "//div" failed: timeout');
});
it('should run in specified frame', async({page, server}) => {
await utils.attachFrame(page, 'frame1', server.EMPTY_PAGE);
await utils.attachFrame(page, 'frame2', server.EMPTY_PAGE);
Expand Down