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

epfremmer · 2020-06-17T18:32:25Z

When using things like await this.users.scopeByName('some name').waitForVisible(); the promise immediately rejects because the scoped selector cannot be found.

Example error:

waitForVisible: waitForVisible(<func>): findChild([data-cs-selector="user-row"],<func>): scopeBy([data-cs-selector="user-row"],<func>): Could not find match (length 19)

kellyselden · 2020-06-18T09:11:37Z

Travis CI has been really bad lately. It's not picking up new builds.

epfremmer · 2020-06-19T14:44:21Z

Is this because this is on a fork? Did a force push and seems to have picked it up that time

epfremmer · 2020-06-19T20:18:40Z

@kellyselden finally got this to behave 🎉

kellyselden · 2020-06-21T08:49:25Z

packages/page-objects/src/base-element.js

+        try {
+          return await this._browser.waitUntil(async () => await this.isExisting());
+        } catch (e) {
+          // if the waitUntil condition fails fall through to the original non-scoped


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

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

kellyselden · 2020-06-21T09:12:59Z

packages/page-objects/test/acceptance/api-verification-test.js

+
+      await this.open('index.html');
+
+      this.page._browser._browser.options.waitforTimeout = 10000;


The browser is global, this will leak across the whole test suite.

Added a timeout value reset in afterEach hook

kellyselden · 2020-06-21T09:13:40Z

packages/page-objects/test/acceptance/api-verification-test.js

+          element.disabled = false;
+
+          document.body.appendChild(element);
+        }, 100);


This has a race condition. If the assertion runs before this, the test will fail.

This is intentional I want scopeBy to start running and respond to the element changing state in order to test that the wait condition is working.

kellyselden · 2020-06-21T09:29:56Z

packages/page-objects/src/base-element.js

@@ -41,6 +41,16 @@ class BaseElement extends BasePageObject {

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


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

What do you want it called?

waitForScoped?

kellyselden · 2020-06-21T09:30:53Z

packages/page-objects/src/base-element.js

@@ -41,6 +41,16 @@ class BaseElement extends BasePageObject {

  async waitForInsert() {
    await handleError.call(this, async () => {
+      if (typeof this._selector === 'function') {
+        try {
+          return await this._browser.waitUntil(async () => await this.isExisting());


Should this go in the browser's waitForExist implementation?

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.

kellyselden · 2020-06-21T09:32:27Z

packages/page-objects/src/element.js

@@ -12,10 +12,12 @@ class Element extends BaseElement {
  }

  async waitForEnabled() {
+    await this.waitForInsert();


I don't think these are necessary. The existing assumption is that you are already dealing with an existing element when you call these methods. You are waiting for an element to go from disabled to enabled. If you're element is missing to begin with, the consumer should be tasked with calling waitForInsert first.

I feel like it is better developer ergonomics for consumers to not have to be concerned with that, as well as, fewer wait condition lines in tests.

I'll remove this, but I think we should consider putting it back in to make for a smoother testing experience.

When using a function style selector wait conditions fail early if the scoped selector does not yield any results. Instead we should continue to wait/check until the wait condition times out. Additionally in order to retain custom friendly error messages that include the selector that an exception occurred on we fallback to the original wait behavior which includes the custom errors.

before waiting on other conditions like enabled/visible

kellyselden · 2020-07-21T11:42:53Z

packages/page-objects/src/base-element.js

@@ -41,6 +41,16 @@ class BaseElement extends BasePageObject {

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


waitForScoped?

kellyselden · 2020-07-21T11:44:35Z

packages/page-objects/test/acceptance/api-verification-test.js

+    if (this.browser && this.browser.options) {
+      // some of the tests need to validate wait conditions and modify the wait
+      // timeout so this needs to be reset to avoid leaking context between tests
+      this.browser.options.waitforTimeout = 0;


You should check out isExisting. It would be better to not assume any hard coded numbers here.

kellyselden · 2020-07-21T11:45:11Z

packages/page-objects/test/acceptance/api-verification-test.js

-    await this.writeFixture('index.html', `
-      <input class="foo">
-    `);
+  describe(Element.prototype.waitForEnabled, function() {


Since the waiting in these methods was removed, I don't think any of these extra tests are needed anymore.

kellyselden · 2020-07-21T11:47:18Z

packages/page-objects/src/base-element.js

@@ -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()));


return value is not used in this method.

kellyselden · 2020-07-21T12:41:16Z

packages/page-objects/test/acceptance/api-verification-test.js

+
+      await this.open('index.html');
+
+      this.page._browser._browser.options.waitforTimeout = 10000;


do you think 1 second is enough?

10000 = 10,000 = 10 seconds, no?

yep, I'm proposing switching to 1000, and also asking if that would be enough.

kellyselden · 2020-07-21T13:12:22Z

packages/page-objects/test/acceptance/api-verification-test.js

+        .to.eventually.be.fulfilled;
+    });
+
+    it('waits for scoped element to be inserted', async function() {


maybe putting these in their own describe block would be better? not sure, but then you could set the new timeout in the beforeEach.

epfremmer force-pushed the fix-wait-for-scoped-page-object branch 6 times, most recently from 1bf8a3c to c5f670a Compare June 19, 2020 20:10

kellyselden requested changes Jun 21, 2020

View reviewed changes

epfremmer force-pushed the fix-wait-for-scoped-page-object branch from c5f670a to 0e77136 Compare June 22, 2020 18:35

Edward Pfremmer added 2 commits June 22, 2020 13:38

fix: remove capability to wait for elements to exist first

d68f9aa

before waiting on other conditions like enabled/visible

epfremmer force-pushed the fix-wait-for-scoped-page-object branch from 0e77136 to d68f9aa Compare June 22, 2020 18:38

epfremmer requested a review from kellyselden June 23, 2020 15:01

kellyselden requested changes Jul 21, 2020

View reviewed changes

kellyselden reviewed Jul 21, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

epfremmer commented Jun 17, 2020

kellyselden commented Jun 18, 2020

epfremmer commented Jun 19, 2020 •

edited

epfremmer commented Jun 19, 2020 •

edited

kellyselden Jun 21, 2020

epfremmer Jun 22, 2020

kellyselden Jun 21, 2020 •

edited

epfremmer Jun 22, 2020

kellyselden Jun 21, 2020

epfremmer Jun 22, 2020

kellyselden Jun 21, 2020

epfremmer Jun 22, 2020

kellyselden Jul 21, 2020

kellyselden Jun 21, 2020

epfremmer Jun 22, 2020

kellyselden Jun 21, 2020

epfremmer Jun 22, 2020

kellyselden Jul 21, 2020

kellyselden Jul 21, 2020

kellyselden Jul 21, 2020

kellyselden Jul 21, 2020

kellyselden Jul 21, 2020

MichalBryxi Jul 21, 2020

kellyselden Jul 21, 2020

kellyselden Jul 21, 2020


		await this.open('index.html');

		this.page._browser._browser.options.waitforTimeout = 10000;

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

Are you sure you want to change the base?

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

Conversation

epfremmer commented Jun 17, 2020

kellyselden commented Jun 18, 2020

epfremmer commented Jun 19, 2020 • edited

epfremmer commented Jun 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyselden Jun 21, 2020 • edited

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

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

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

Choose a reason for hiding this comment

epfremmer commented Jun 19, 2020 •

edited

epfremmer commented Jun 19, 2020 •

edited

kellyselden Jun 21, 2020 •

edited