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

fix: premature resolution of wait conditions when using scopedBy page objects #326

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: premature resolution of wait conditions when using scopedBy page objects #326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d036635
fix: premature resolution of wait conditions when using scopedBy
Jun 17, 2020
d68f9aa
fix: remove capability to wait for elements to exist first
Jun 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions packages/page-objects/src/base-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ class BaseElement extends BasePageObject {

async waitForInsert() {
await handleError.call(this, async () => {
if (typeof this._selector === 'function') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These can go in a reusable function, kinda like handleError so they both can use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you want it called?

Copy link
Collaborator

Choose a reason for hiding this comment

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

waitForScoped?

try {
return await this._browser.waitUntil(async () => await this.isExisting());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this go in the browser's waitForExist implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that first and ran into so many issues because of all of the areas in which waitForExists impacts. Opted to hoist it up to here for simplicity sake.

} catch (e) {
// if the waitUntil condition fails fall through to the original non-scoped
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will unfortunately double the waiting time of failing tests, right? 30 secs to 60 secs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the default timeout is still 30 seconds. I tested this by commenting out all of the timeout overrides in one of the tests and allowing it to fail, which it did after 30 seconds

// call so we continue to throw custom human readable faltest errors about the
// element selector that is missing
}
}

await this._browser.waitForInsert(this._selector);
}, async elementError => {
// If it's an expected error, that means a parent doesn't exist,
Expand All @@ -56,6 +66,16 @@ class BaseElement extends BasePageObject {

async waitForDestroy() {
await handleError.call(this, async () => {
if (typeof this._selector === 'function') {
try {
return await this._browser.waitUntil(async () => !(await this.isExisting()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

return value is not used in this method.

} catch (e) {
// if the waitUntil condition fails fall through to the original non-scoped
// call so we continue to throw custom human readable faltest errors about the
// element selector that was not destroyed
}
}

await this._browser.waitForDestroy(this._selector, ...arguments);
}, () => {
// If it's an expected error, that means a parent doesn't exist,
Expand Down
Loading