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
65.74k stars 3.58k forks source link

[Feature] Extend locator api with functions like waitForSelector() #8577

Closed MindaugasMateika closed 2 years ago

MindaugasMateika commented 3 years ago

Given the pseudo-POM below and the case where you would need to wait for an element for any reason, you'd have to repeat the selector in two places. It hurts reusability.

export class PageObject {
  private readonly page: Page
  private readonly someButton: Locator

  constructor(page: Page) {
    this.page = page
    this.someButton = this.page.locator('#someButtton')
  }

  async clickSomeButton() {
    await this.page.waitForSelector('#someButton')
    await this.someButton.click()
  }
}

While using the old method, you could set selector value once and then simply reuse the variable as much as you want:

POM with page actions ``` export class PageObject { private readonly page: Page private readonly someButton: string constructor(page: Page) { this.page = page this.someButton = ('#someButtton') } async clickSomeButton() { await this.page.waitForSelector(this.someButton) await this.page.click(this.someButton) } } ```

It'd be nice if locator API would have an option to use functions like waitForSelector(). My use case involves only this function but there might be others.

pavelfeldman commented 3 years ago
async clickSomeButton() {
    await this.page.waitForSelector('#someButton')
    await this.someButton.click()
}

As actions wait for the selector, you don't need this line above at all. Could you suggest other use cases where locator.waitFor would be useful? I want to add it myself, but every time I see a use case, there is a better alternative using existing locator api or new async expect matchers...

MindaugasMateika commented 3 years ago

Yes, this was a bad example. Dunno why I wrote that. Actually, in my case, I was slightly disappointed that assertions do not auto-wait but I guess it's out of the question to implement since Playwright uses Jest except, right?

Anyhow, this is where I ran into a problem:

test.only('Login', async ({page}) => {
  await page.goto('https://www.vinted.com')
  await page.locator('#onetrust-accept-btn-handler').click()
  await page.locator('[data-icon-name="x"]').click()
  await page.goto('https://www.vinted.com/member/general/login')
  await page.locator('#login').fill('pwtest')
  await page.locator('#password').fill('Pwtest123')
  await page.locator('text="Log in"').last().click()
  await expect(page.locator('#user-menu-button')).toBeVisible()
})

After clicking login page does redirection so assertion immediately fails

    locator.isVisible: Execution context was destroyed, most likely because of a navigation.
    =========================== logs ===========================
      checking visibility of "#user-menu-button"
    ============================================================

And I was hoping to do something like await expect(page.locator('#user-menu-button').waitForSelector()).toBeVisible() :>

yury-s commented 3 years ago

If clicking 'Log in' leads to a complex navigation you may need to insert explicit waitForURL to ensure that the page navigates to the final URL before checking the user menu button, would that work?

MindaugasMateika commented 3 years ago

Hm, it does. I'll close the issue then. Thank you for your time!

mhils commented 3 years ago

As actions wait for the selector, you don't need this line above at all. Could you suggest other use cases where locator.waitFor would be useful? I want to add it myself, but every time I see a use case, there is a better alternative using existing locator api or new async expect matchers...

My use case might be slightly unusual as I'm instrumenting third-party websites for a research project instead of writing tests, but I found myself repeatedly writing code like this:

page.on("frameattached", frame => {
    await frame.waitForSelector(".message-button");
    const buttons = frame.locator(".message-button");
    const buttonTexts = await buttons.allTextContents();
    // ...
});

It would be nice if I could write something like this instead:

page.on("frameattached", frame => {
    const buttons = frame.locator(".message-button");
    await buttons.waitFor();
    const buttonTexts = await buttons.allTextContents();
});

I think that's generally a problem with functions that deal with multiple elements (evaluateAll, allTextContents, elementHandles). It was a bit surprising to me that evaluateAll does not block while evaluate does, but it's probably a relatively pragmatic choice. I feel that this particular use case could be improved with a dedicated .waitFor().

pavelfeldman commented 3 years ago

Actually, in my case, I was slightly disappointed that assertions do not auto-wait

@MindaugasMateika our assertions do wait, see https://playwright.dev/docs/test-assertions

@mhils Yes, this makes sense. We expect users to do

await expect(locator).toHaveText(['a', 'b', 'c']);

in the testing mode, but I can see how this does not work when you are automating something. I'll leave this open and schedule it for the next milestone.

kevin940726 commented 3 years ago

Would be nice if all of the methods of Locator automatically wait (retry) for the element. Take this as an example.

// 1. Start from an empty page
await page.setContent('');
// 2. Click on the button that doesn't exist yet
await page.locator('button').click();
// 3. The button is attached later
await page.setContent('<button>Button</button>');

I would expect it to work, so that we don't have explicitly wait for something to happen. Playwright should be smart enough to auto-wait for us, just like how it's done in the assertions.

$ and $$ are APIs that don't wait, which I think is a design flaw. We have the opportunity to improve this in Locator since it's a new API. (It has already been released though so we have to think about backward-compatibility.)

Nav-2d commented 2 years ago

Would like this from reusability perspective. Thank you!

const userId = this.page.locator(homePageSelectors.userId);
const signOutLink = this.page.locator(homepageSelectors.signOutLink); // 1
await this.page.waitForSelector(homePageSelectors.userId);
await userId.click();
await this.page.waitForSelector(homepageSelectors.signOutLink); // 2
await signOutLink.click(); // Need to wait for the selector before the click as it is a dropdown with animation