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.

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

e2e-test: more notebook fixes #5154

Merged
merged 8 commits into from
Oct 24, 2024
Merged

e2e-test: more notebook fixes #5154

merged 8 commits into from
Oct 24, 2024

Conversation

midleman
Copy link
Contributor

@midleman midleman commented Oct 24, 2024

Intent

Make the notebookCreate.test.ts more stable / less flakey.

Approach

The main issue I noticed with the notebook creation test was that we weren’t properly asserting the text content of locators. We were fetching the locator and then immediately verifying its text. This works fine for non-async code, but since we are dealing with asynchronous behavior, this approach isn’t reliable. Instead, we should leverage Playwright’s built-in retry mechanism in the expect methods, which will automatically keep checking the locator until its text matches the expected value or the timeout is reached.

I'm hoping some of the fixes help other tests that rely on executing code, etc. 🤞

Examples

Don't Do This

This approach fails immediately if the element’s text is not ready.

const textContent = await page.locator('.element').textContent();
expect(textContent).toBe('Expected Text');  // Immediate assertion

Do This

Playwright will retry until the element contains the expected text, making it more resilient.

await expect(page.locator('.element')).toHaveText('Expected Text');  // Waits for the condition to be met

Tip

Your await should always precede the expect and never be "inside" it (like in the first example).

  • Use await on the expect statement when you are testing dynamic or asynchronous content where the state of the page may change over time.
  • Use await on the locator or property directly when you are certain that the content is ready and available immediately, and you want to perform the assertion right away.

@midleman midleman marked this pull request as ready for review October 24, 2024 14:33
Copy link
Contributor

@testlabauto testlabauto left a comment

Choose a reason for hiding this comment

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

LGTM!

// basic CSS selection doesn't support frames (or nested frames)
const frame = this.code.driver.getFrame(OUTER_FRAME).frameLocator(INNER_FRAME);
const element = frame.locator(`${MARKDOWN_TEXT} ${tag}`);
const text = await element.textContent();
Copy link
Contributor Author

@midleman midleman Oct 24, 2024

Choose a reason for hiding this comment

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

just an example, but this is what was problematic. the text is being retrieved immediately, which means it might grab the content before it’s fully rendered or updated, causing the assertion to happen before the text is actually correct/ready... which would def lead to flakes.

Copy link
Contributor

@jonvanausdeln jonvanausdeln left a comment

Choose a reason for hiding this comment

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

LGTM! And thanks for the tip on how to assert the text!

@midleman midleman merged commit e58dd99 into main Oct 24, 2024
3 of 4 checks passed
@midleman midleman deleted the mi/more-notebook-fixes branch October 24, 2024 16:54
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants