Playwright: BaseContainer constructor to accept list of selectors to wait on

[worldomonation]

Task Details Currently, many of the page/components implemented for Playwright use the following pattern:

class declaration
    constructor {
        super( page, selector_A)
    }

    async _postInit() {
        page.waitForSelector( selector_B )
        page.waitForSelector( selector_C  )
    }

Look at refactoring the BaseContainer's constructor to accept a list of selectors, so that the async _init() method can wait on multiple selectors.

[WunderBart]

I've been trying to look at this from the opposite site and actually started leaning towards it:

What if we don't pass expected element(s) at all?

How do we really benefit from them? It is possible that a certain object model will be used in a test scenario in which its initially expected element is not even relevant. For example, in the CommentsComponent model - do we need the commentArea element to be present in order to initialize? Or are we just making the test depend on another element that just makes the whole thing more fragile? Let's say we just want to use postComment from that component - all we need is the text input and submit button to be present in order to really test what we want to test, which is already validated in that method.

I'm aware that the expected selector is optional right now, but maybe even that might encourage a bad practice leading to broken tests in the long run.

Am I missing something here? What do you think, @worldomonation?

[worldomonation]

What if we don't pass expected element(s) at all?

Radical thoughts! They challenge existing assumptions and help identify unnecessary practices, so thanks for that.--

For example, in the CommentsComponent model - do we need the commentArea element to be present in order to initialize?

In my opinion, this is an emphatic yes.

Having the objects wait for specific selector(s) to finish loading and visible on page reinforces assurance that page/component is in a good, known state for interactions to begin. This is even more important in JS/TS because of how almost everything is basically async.

I'll illustrate my case with an example of a problem I ran into while implementing the NewPostFlow some weeks back.

The call NewPostFlow.newPostFromNavbar() executed perfectly acceptably on my local machine (MacBook Pro M1), never failing a single time to start a new post. Once the tests were complete, I pushed to TeamCity and found this method to flip flop between permafail and intermittent fail. Each time the failure point was the same though: the dashboard would load, the click call would fire, and nothing happens.

It took me quite some time and stress to figure out why Playwright, the supposedly super reliable and smart framework, was having this issue.

I was finally able to make the flow function reliably on both local machine and TeamCity by using Expect calls, waiting on a couple of selectors and using more waitForLoadState. The flow has not failed a single time since being figured out almost three weeks ago, which shows the wait was necessary.

There are a some other instances where I've encountered similar situations as above. Most of the time, the root cause of the issue is that on TeamCity, things execute and load just a little bit slower than local development environment; and Playwright runs too fast for its own good, so it is overly eager to match a selector and start executing actions even if the target element may not quite be ready.

It is possible that a certain object model will be used in a test scenario in which its initially expected element is not even relevant.

This is indeed (theoretically and likely in practice) true with many of the components and pages I've implemented so far.

If I may rephrase your concern - is it that we are potentially waiting on unnecessary load state and/or selector(s) which end up taking up more time?

It is possible to profile what Playwright is doing under the hood by exporting the environment variable DEBUG=pw:api and running the test. Doing so shows that waiting for .comments-area takes only 3ms on the local machine:

  pw:api waiting for selector ".comments-area" to be visible +0ms
  pw:api   selector resolved to visible <div id="comments" class="comments-area">…</div> +3ms
  pw:api <= page.waitForSelector succeeded +3ms

I've also checked the build logs on TeamCity as I have set the debug environment variable there to inspect what's happening:

2021-06-15T21:39:59.941Z pw:api waiting for selector ".comments-area" to be visible
2021-06-15T21:39:59.971Z pw:api   selector resolved to visible <div id="comments" class="comments-area">…</div>

On TeamCity it appears to require 30ms to resolve the selector, but this is also under condition where multiple parallel tests are executing in magellan.

Overall, the time loss per waiting for the .comments-area selector appears to range from 3-30ms depending on the environment.

---

Your suggestion made me think over again the question of coding style as well.

Perhaps the responsibility to wait for elements prior to executing actions should lie with the object methods. Currently not all the methods I have implemented follow this as some rely on the object to verify the page state at initialization.

---

Ultimately I still think the optional selector in Expect should remain as it is useful in controlling the speed at which Playwright executes instructions in slower environments. It helps avoid avoidable TimeoutErrors and saves a lot of engineer's time and headache not having to troubleshoot it.

[WunderBart]

I was finally able to make the flow function reliably on both local machine and TeamCity by using Expect calls, waiting on a couple of selectors (...).

This is interesting, as it was almost the exact opposite for me when fixing flaky tests in Selenium (if I understand the issue correctly). The more arbitrary elements were awaited for, the more likely the test would be flaky and slow the whole suite down. The key to simulating a reliable element interaction has been for me to focus only on the element that the user is actually interacting with. There were a lot of examples where the test was i.e waiting for a container/wrapper element in order to click a certain inner item, while only that very item was the relevant element there and should be awaited for. Having said that, I'm pretty sure we can achieve the same with Playwright when applying some combination of waitForSelector and waitForElementState. Would you be able to provide an example where an element required the presence of other elements in order to be interacted with? Also, I might have misunderstood the issue you were having with the NewPostFlow, but happy to dig into it anyway :D

---

Perhaps the responsibility to wait for elements prior to executing actions should lie with the object methods. Currently not all the methods I have implemented follow this as some rely on the object to verify the page state at initialization.

I agree. I don't think that any other element than the one we're interacting with would reinforce stability assurance. For application flow testing, I think we need to focus only on elements that are subject to the test.

---

If I may rephrase your concern - is it that we are potentially waiting on unnecessary load state and/or selector(s) which end up taking up more time?

Yes, but my bigger concern is something else. If they're unrelated/unnecessary waits, it also adds up to the stack of things that can potentially change and break the test. Every additional element that we wait for is increasing the risk of test flakiness, so we always need to make sure only the absolutely necessary elements are involved.

---

Ultimately I still think the optional selector in Expect should remain as it is useful in controlling the speed at which Playwright executes instructions in slower environments. It helps avoid avoidable TimeoutErrors and saves a lot of engineer's time and headache not having to troubleshoot it.

I'm not sure I understand the issue of controlling speed between environments. Could you provide an example, please?

[worldomonation]

I'm not sure I understand the issue of controlling speed between environments. Could you provide an example, please?

This question can be addressed by the NewPostFlow and I will try to go into more detail as to why tests were failing unexpectedly and was resolved by making it wait for selectors.

The section of the test in question was implemented as wp-likes-spec.

The way it was written, the following actions took place (high-level overview):

• invoke LoginFlow.login()
  • LoginPage.login() is called with username/password params
  • LoginPage.login() submits form, initiating navigation

(at this point, NewPostFlow takes over in the execution)

• instantiate NewPostFlow as a synchronous object
• invoke NewPostFlow.newPostFromNavbar()

  async newPostFromNavbar(): Promise< void > {
      const navbar = await NavbarComponent.Expect( this.page );
      await navbar.clickNewPost();
  }

I had to iterate and try a bunch of different approaches for clickNewPost, but this revision should be a fairly good indicator of what I was trying to do.

(note: each revision I tried worked consistently on local machine, but would fail intermittently but often enough that TeamCity marked it as a failing test.)

In the revision above clickNewPost did the following:

• wait for the new post button to appear
• Promise.all the following:
  • wait for network idle (intended to wait for GutenbergEditor to show up)
  • wait for navigation
  • click the new post button

This approach did not work either.

One of the issues that I eventually nailed down was that NavbarComponent.Expect() waited for the navbar to appear as it should (including the new post button) but it would execute the click action within milliseconds after and this was way too fast for TeamCity agents to keep up. As a result, Playwright thought it had clicked on the new post button, and it indeed had, but the frontend was not ready to actually action on the click until the loading progressed further. Checking the replay footage (deleted now) the execution clearly stops once the home dashboard loads.

At this point, I decided to make NewPostFlow.newPostFromNavbar() wait for the sidebar and home dashboard before allowing execution to continue. The change was introduced in this commit.

• SidebarComponent would wait for the #secondary element to appear
• MyHomePage would wait for the .customer-home__main and .stats to appear

Incorporating the new components and pages:

    async newPostFromNavbar(): Promise< void > {
        await SidebarComponent.Expect( this.page );
        const navbarComponent = await NavbarComponent.Expect( this.page );
        await navbarComponent.clickNewPost();
        await GutenbergEditorPage.Expect( this.page );
    }

By making the execution wait on a core element that should be present with the component/page, this allows us to control how fast Playwright executes without introducing fixed waits. In theory, this code can work with throttled 3G speed connections all the way up to 1Gbps fibre (as long as wait doesn't exceed 30000ms).

Hopefully this illustrates the need to make Playwright pace itself by either making it wait on an element and/or making it wait for element state.

[worldomonation]

I think my example in the previous comment should also address this part of the question, but let me know if I did not!

Would you be able to provide an example where an element required the presence of other elements in order to be interacted with?

[WunderBart]

Thank you for the thorough explanation, I appreciate it! 🙇

The issue with elements being "dead" when clicked by the driver has been occurring with Selenium as well. It's a tricky one, as it can be caused by a variety of reasons, but I don't think it's ever TC agents not keeping up. The click can sometimes occur too soon (i.e. when the JS has not kicked fully in yet or the element is still moving), but in my experience, it could always be addressed by creating a robust selector and/or detecting the target element state. Depending on other elements presence has shown to be unreliable in the longer run, and I'll try to explain why I think that is. I'll also see if we can make interacting with the New Post button not depend on other elements 🤞

Why relying on other elements' presence is bad?

When trying to control the execution speed with elements load time, we're introducing something that is not constant and can lead to unpredictable results. When we make an element rely on other element(s) presence, we can expect the test to break mainly in 2 situations - A: depender element selector changed or B: depender element load time changed (i.e. due to a refactor). While A isn't that bad as it's fairly easy to spot and fix, B can cause flakiness that could potentially be really hard to trace and fix. In our Selenium E2Es, B was many times the exact reason for a spec to suddenly become flaky, and almost every time the way to fix it reliably was by constructing a proper selector or a waiting function focused solely on the target element. There's another con to this practice - the depender elements snowball effect. Because it's been a known practice to wait for other elements before interacting with the target element (many times applied by copying a bad example), in places where those elements weren't delaying the execution enough anymore, even more elements (or sleeps) were introduced in order to fix the flakiness, eventually creating an even bigger ticking bomb and one that's harder to disarm. I hope it all makes sense, as it's why I think this should never be good practice to make an element interaction depend on the presence of other elements, and should be avoided at all cost.

Can we click the Masterbar's Write button successfully without waiting for other elements beforehand?

It seems like we can! 😄 In https://github.com/Automattic/wp-calypso/pull/53871, I made the click happen directly, but with 2 differences to the original implementation:

1. The waitForNavigation is called beforehand because it's a redirecting click, and
2. The selector has higher specificity to ensure we're targeting a single node.

Regarding 1, I think it was simply omitted by accident. 😄 Regarding 2, I think the important part is that we're making sure there's no other element that can be clicked. The .masterbar__item-new is a class selector, and we can't really be sure it's not used on any other element. Rising the specificity can often do the trick, but we need to remember to use user-facing attributes as much as possible to not make the selector too brittle. See the TC builds for that PR here. What do you think? Have I missed something? I hope it helps!

[worldomonation]

Whew, that's a lot of stuff to unpack!

Can we click the Masterbar's Write button successfully without waiting for other elements beforehand?

Thanks for trying to make this flow simpler yet robust in your branch! It's always good to have different pair of eyes on any given codebase since we all have different perspectives and approaches. It really is a difficult task to craft selectors that are accurate, not burdensome to read and yet flexible - more art than anything.

I reviewed the run history on TC and noticed that on Run 238 this particular flow failed once. Perhaps by adding some more element state checks and/or enabledness checks this rare intermittent failure can be weeded out. Let's iterate on the PR :)

---

Why relying on other elements' presence is bad?

Good points on selector snowball effect.

I think I am coming around on this particular topic. There is still a (shrinking) part of me that believes default selectors do hold value in the page objects - but reviewing some of the earlier objects I've written I can see how things could have been done differently. In particular, some of the _postInit methods are not up to my current standards, and it's only been a few months since this all started 😆

Overall, I think it's worth spending some time this week to refactor the library to remove or reduce:

• selector stacking
• _postInits
• default selectors in Expect

As well as spending some time crafting better selectors that prioritize user facing attributes.

Would this course of action be something you'd support?

[WunderBart]

I reviewed the run history on TC and noticed that on Run 238 this particular flow failed once. Perhaps by adding some more element state checks and/or enabledness checks this rare intermittent failure can be weeded out. Let's iterate on the PR :)

It looks like it's unrelated to the changes in that PR, no? 🤔. As a side note, that error stack is not really helpful:

Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/teamcity-0/buildAgent/work/c4a9d5b38c1dacde/test/e2e/specs/specs-playwright/wp-likes__comment-spec.js)
    at listOnTimeout (internal/timers.js:554:17)
    at processTimers (internal/timers.js:497:7)

Do you know why this is not printing at least the line it's been thrown at in the spec.js file? It reminds me of the same kind of issue we had with Selenium not that long ago. I've tweaked it by proxying an error handler inside the driver promise in https://github.com/Automattic/wp-calypso/pull/52669, as it was making the debugging quite difficult.

[WunderBart]

In particular, some of the _postInit methods are not up to my current standards, and it's only been a few months since this all started 😆

It means progress!! 😄 🙌

[WunderBart]

Overall, I think it's worth spending some time this week to refactor the library to remove or reduce:

• selector stacking
• _postInits
• default selectors in Expect

As well as spending some time crafting better selectors that prioritize user facing attributes.

Would this course of action be something you'd support?

Re: selector stacking

As a general rule, we need to focus on the target elements and avoid making them depend on other elements' presence. Wherever we can factor it out I think we should do it 👍

Re: _postInits

I think post-init calls make most sense for page models, where i.e. we'd want to close the cookies popup or a welcome modal. It would be good to review those as well.

Re: default selectors in Expect

I've been thinking about this one, and IMO they make sense only for the Component models. Every UI component has a wrapper element whose presence can indeed indicate the component's readiness to be interacted with. It doesn't hold as well for the Page models, where a page generally operates over a set of components and doesn't have its distinct presence selector. Now that I think of it, Page components are starting to feel like an unnecessary abstraction layer that can potentially become a maintenance burden. Why not just use flows? 🤔 I obviously don't yet have a clear idea of what could be better for us in this context, just wanted to raise awareness and possibly start a discussion. Any thoughts on that, @worldomonation?

[worldomonation]

It looks like it's unrelated to the changes in that PR, no?

I suspect it is related because the step at which it fails is Enter post title under the test suite Comment and like on a new post. The video replay shows that it failed to click on the new post button to launch into the GutenbergEditorPage. Luckily it seems to have occurred only once, and passed in subsequent tries so it likely only needs an additional state or selector wait.

As a side note, that error stack is not really helpful:

I agree, I had to deal with this early on in the work and I thought it was resolved. To my knowledge, it might have something to do with the different timeouts interacting - mocha, playwright and who knows what else. I don't think it's worth spending time trying to fix, as the jest or playwright-runner migration is on the horizon.

I think post-init calls make most sense for page models,

In a (coming) PR I looked at each file and rewrote it so as to not rely on both default selectors and postInits where possible. I decided against removing the postInits from a couple of abstraction files though; I'd be interested in seeing your review on that.

Now that I think of it, Page components are starting to feel like an unnecessary abstraction layer that can potentially become a maintenance burden. Why not just use flows?

I touched on this a little in the P2 post pciE2j-lC-p2, but it does become difficult to draw distinction on what should constitute a component, page and flow. I often struggle with this question when starting to write a new abstraction.

Given the current state of the abstractions here, do you think we should default to having perhaps just pages or components, but not both?