microsoft / playwright

Playwright is a framework for Web Testing and Automation. It allows testing Chromium, Firefox and WebKit with a single API.
https://playwright.dev
Apache License 2.0
67.25k stars 3.7k forks source link

[Feature]: Add `page.clock` method to check if `page.clock.install()` was called #32913

Open EwenDC opened 1 month ago

EwenDC commented 1 month ago

🚀 Feature Request

Add a new function to the page.clock object that returns true if page.clock.install() has been called in this browser context, and false otherwise.

Example

This function can be used on Page Object methods to assert that page.clock.install() was called in the test and throw an exception if it wasn't. This prevents calls to page.clock functions silently failing if page.clock.install() was not called within the test containing the Page Object.

Motivation

In our end-to-end test suite we follow the Page Object Model for structuring the code. This means that we have some page classes that have methods which make page.clock calls that are not visible in the test files themselves. Due to the necessity of page.clock.install() being called before page.goto(), we can't include the install call in the methods, and must rely on the test authors to remember to include it at the top of their tests. However, we find that it's really easy for test authors to forget to do this, and code reviewers to not notice it's absence. The kicker is that in our use case this won't cause tests to outright fail, only become flakey, making it even easier to miss it's absence.

Ideally it'd be nice if the page.clock.* calls immediately failed the test if page.clock.install()wasn't called, but I understand doing that would be a breaking change. We'd be happy to add some custom logic to our methods to throw an error if the clock wasn't installed, but Playwright doesn't currently expose a method to let us verify if the clock is installed or not. That is why I am proposing the addition of such a method.

Skn0tt commented 1 month ago

I think of Clock manipulation as being so essential to the test scenario, that I think hiding that inside the Page Object Model could be a little hard to read. Could you show us some code examples of your tests where you use this?

EwenDC commented 1 month ago

Sure. We have a frontend where customers can order services then edit them once they go live. The frontend will poll the backend to see when a service goes live, but it waits roughly between 60 to 90 seconds to make these polling calls. Obviously this is a long time to wait in a test, so we use fast forwarding to effectively make the frontend poll on demand.

Our page object looks something like this:

export class ServicePage extends PageWrapper {
  readonly pendingDeploymentMessage = this.page.getByText('This service is awaiting deployment.')

  async checkNotDeploying() {
    // Only start the polling loop if the service is still pending deployment
    if (await this.pendingDeploymentMessage.isVisible()) {
      await expect(async () => {
        await this.page.clock.fastForward('05:00')
        await expect(this.pendingDeploymentMessage).toBeHidden()
      }).toPass({ intervals: [0], timeout: 60_000 })
    } else {
      await expect(this.pendingDeploymentMessage).toBeHidden()
    }
  }
}

Which allows us to write like this in the tests:

  await servicePage.checkNotDeploying()
  await servicePage.updateName(newName)

If the test author forgets to call page.clock.install(), the test can still pass if the frontend happens to poll within the test timeout, but it's obviously far less reliable than when we're triggering polling events every ~5 seconds.

I think it's a pretty typical coding practice to extract logic that is both complicated, and used in a lot of places, into a central location like a page object. Plus, that function existed before Playwright even added the clock functionality, looking something like this:

  async checkNotDeploying() {
    // This only justifies being wrapped in a function due to the timeout
    await expect(this.pendingDeploymentMessage).toBeHidden({ timeout: 100_000 })
  }

Even without that abstraction layer, I don't understand the reasoning behind making the page.clock functions silently fail? I could easily see someone who's new to test writing start trying to use page.clock functions without the install call then get frustrated when they seemingly do nothing.