Feature/auto waiting

[jeppester]

🔗 Linked issue

#1

❓ Type of change

• 🐞 Bug fix (a non-breaking change that fixes an issue)
• 👌 Enhancement (improving an existing functionality like performance)
• ✨ New feature (a non-breaking change that adds functionality)
• ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

Implements waiting behaviour for the assertions added to the playwright page.
Resolves #1.

Let me know if this approach seems good to you, so that I can confidently implement the remaining assertions and update the documentation.

📝 Checklist

• I have linked an issue or discussion.
• I have updated the documentation accordingly.

[thetutlage]

This looks fine to me at the first glance. Have to still pull the code and try it locally.

Can you please add some notes around

• If the old assertions will continue to work as it is.
• I see, you have introduced a config object and how that will be passed from the user-land.
• Are are exposing the playwright expect to the end-user?

[jeppester]

Can you please add some notes around

* If the old assertions will continue to work as it is.

* I see, you have introduced a config object and how that will be passed from the user-land.

* Are are exposing the playwright `expect` to the end-user?

I'm not 100% sure were you'd like those notes, but to begin with I can answer the questions here:

If the old assertions will continue to work as it is.

The old assertions will assert the same things as before, but when an expectation is not met, they will keep checking until they succeed or time out, at which point they will raise the same assertion errors as before.

Since this change only adds an extra opportunity for an assertion to succeed I don't believe it will cause existing specs to fail. That is as long as the spec does not anticipate an assertion error, which would be bad practice (unless for testing the assertion itself).

In regards to failing specs, failures will take a bit longer, because they will need to time out before they fail.

I see, you have introduced a config object and how that will be passed from the user-land.

It will be passed as part of the PluginConfig when setting up the browser test plugin, e.g.:

{
  ...existing plugin config
  expect: { timeout: 1000 }
}

The above will make the assertions wait for up-to 1 second before failiing. The config object is the same as for expect.configure (but without the other options as they don't make sense in the context of japa):
https://playwright.dev/docs/test-assertions#expectconfigure

Are are exposing the playwright expect to the end-user?

I expose it as a part of the test context, so that the end-user can easily get hold of playwright's expect if they need it. It also has the timeout configured according to the PluginConfig, which a freshly imported expect would not.

I am however a bit worried that "expect" is too generic a property name to have added directly to the test context. I'd understand if you prefer it to have it nested, f.i. under playwright.

[jeppester]

Btw.: I will go ahead with implementing the remaining assertions

[thetutlage]

Hello!

Yeah, sorry for not being clear. I meant adding notes in the PR description or comment.

I am however a bit worried that "expect" is too generic a property name to have added directly to the test context. I'd understand if you prefer it to have it nested, f.i. under playwright.

Yeah, we already have expect in the context when you use the https://japa.dev/docs/plugins/expect plugin. So we will have to go with another name here. How about pwExpect?

[jeppester]

Yeah, we already have expect in the context when you use the https://japa.dev/docs/plugins/expect plugin. So we will have to go with another name here. How about pwExpect?

No problem. I would however prefer if it could be destructured directly to expect because that's the word you'll see in the playwright documentation, on stack overflow or when you ask copilet/chatgpt for help.

As I see it we should pick one of the following solutions:

Namespace expect under pw (or playwright)

  test('assert page title', async ({ browser, pw: { expect } }) => {
    ...
    await expect(page.locator('a')).toBeVisible()
  })

Pros:

• Expect will have the same name as you'll see elsewhere (as mentioned above)
• It would perhaps be reasonable to namespace properties added by plugins, as it avoids conflicts like the one we are running into here.

Cons:

• It raises the question if browser and browserContext should also be moved as they are playwright related as well. Moving them would be a breaking change - unless if we added aliases.
• You'd have to rename the import if you'd also like to use @japa/expect in the same test.

Use pwExpect

  test('assert page title', async ({ browser, pwExpect } }) => {
    ...
    await pwExpect(page.locator('a')).toBeVisible()
  })

Pros:

• No conflict

Cons:

• Expect will be named differently than what you'll see elsewhere.
• Prefixing one playwright property with pw while leaving the others nonprefixed seems inconsistent - a similar issue as with name spacing the property.

Use pwExpect, but (in examples) give it another name when destructuring

  test('assert page title', async ({ browser, pwExpect: expect } }) => {
    ...
    await expect(page.locator('a')).toBeVisible()
  })

Pros:

• No conflict
• Expect will have the same name as you'll see elsewhere

Cons:

• Prefixing one playwright property with pw while leaving the others nonprefixed still seems inconsistent.
• The ExpectProxy (for when the suite hasn't been set up to use the browser) will have to either show "expect" or "pwExpect" both of which will be confusing under some circumstances

After having given all the solutions some thought - while writing this comment - I'm still leaning towards name spacing. I feel that the benefit of being able to write more idiomatic playwright code outweighs the negatives.
But I will leave the choice up to you - obviously.

[jeppester]

@thetutlage
I realized that it would not work well to use expect().toPass as an escape hook for all the tests, because it swallows the errors. After much digging around I eventually came to the conclusion that it would work better to build a custom wrapper - reusing some of the inner logic from expect().toPass.

So instead of reimplementing all specs using playwright's expect, I'm now trying an approach where all the previous assertions are just wrapped in this new wrapper.

The advantages are:

• The assertion code itself has not changed at all.
• Since we are not using playwright's expect, we can completely remove that from the PR and we avoid making a decision about whether to let the user access it or not. (I still think we should have an option for setting the timeout)

The disadvantage is:
The functions I'm pulling in from playwright (pollAgainstDeadline and monotonicTime) are not part of PlayWright's user facing API, to there's a risk that they cloud change in ways that will break browser-client.

Those functions are however completely self-contained and could easily be pulled into browser-client (and maybe adjusted a bit).

Could you take a look at src/decorators/assertions.ts and let me know what you think about this approach?

[thetutlage]

I realized that it would not work well to use expect().toPass as an escape hook for all the tests, because it swallows the errors

Is it something inherent with Playwright that their expectations do not throw error and instead mark the test as failed internally?

[thetutlage]

If you ask on the options for 3 API's. I will go with the 3rd one. I think its the balanced in many ways.

• No tested destructuring needed.
• No breaking changes
• If you are not using the expect plugin, then you can re-map it.
• If you are using the expect plugin, you can use pwExpect.

[jeppester]

Is it something inherent with Playwright that their expectations do not throw error and instead mark the test as failed internally?

You can see what it does here:
https://github.com/microsoft/playwright/blob/5b28d2a84c0fa5232ab029ff1e6c7c73c10f9e42/packages/playwright/src/matchers/matchers.ts#L424

Basically they grab the message from the error/assertion object, then append some timeout info to that message and return an object in their own internal format.

If you ask on the options for 3 API's. I will go with the 3rd one. I think its the balanced in many ways.

Fine with me 👍

[jeppester]

I believe this PR is ready for review now.

I've reduced the scope of the PR so that it only implements auto-retry (with optional plugin settings).
I think it would be useful to add @playwright/expect to the test context like we discussed, but since I found a way around using it for the assertions, I don't think it's relevant for this PR.

From a user perspective the changes are:

1. All assertions from browser-client now have built-in waiting behaviour. By default they will wait for up-to 5 seconds.
2. It is possible to configure the timeout and polling intervals with an optional assertions plugin option:

    assertions?: {
      timeout?: number
      pollIntervals?: number[]
    }

To implement auto-retry I've lifted some utilities out of playwright and placed them under a new lib-folder with attribution.

If we want to customize the time-out per test or per assertion, that is something we will need to add later. For now I think this will be enough to greatly improve the DX when using browser-test with SPA front-ends (like inertia)

Let me know what you think..

[RomainLanz]

Hey @jeppester! 👋🏻

Amazing work — thank you for all the effort!

I just had a quick question about the changes: what’s the reason for adding the Apache 2 license and the NOTICE.md file?

[jeppester]

I just had a quick question about the changes: what’s the reason for adding the Apache 2 license and the NOTICE.md file?

I copied two files - verbatim - from playwright and added them to a new lib folder. They implement the retry behaviour that I utilize in the retryTest-utility.

I could have imported the same functions directly from the playwright-core package, but since they are not part of their user facing API (and so are not typed) I thought it would be safer to clone the functionality. Now playwright can change it's internal workings without breaking browser-client.

Playwright is using the Apache 2 license, so figured that I'd have to add attribution. (Just like playwright itself has attribution for puppeteer).

If you're not fond of this, I can do my own implementation. It does however seem like some thoughts went into the playwright code. My naive implementation would for instance not have been using monotonic time 🤷😄

UPDATE
I spent some time tonight on re-implementing the retry logic, so that we don't need the attribution. Please have a look.

[thetutlage]

I spent some time tonight on re-implementing the retry logic, so that we don't need the attribution. Please have a look.

I think, I will have no issues with attribution at all. So you can decide either we want to keep the retry logic you have implemented or copy over the playwright files. Its upto you :)

[thetutlage]

@jeppester Seems like tests and the lint workflow are failing. Can you please look into it?

[jeppester]

I think, I will have no issues with attribution at all. So you can decide either we want to keep the retry logic you have implemented or copy over the playwright files. Its upto you :)

The downside is that my implementation is obviously not as battle tested as playwright's and is therefore more likely to contain bugs/oversights.

But if you're happy with my implementation, let's keep that.
It has less indirection and is simpler for not having to support old nodejs versions (it can use timers/promises)

@jeppester Seems like tests and the lint workflow are failing. Can you please look into it?

Sure.

I was running all the tests through npm run quick:test, so I didn't catch a minor eslint warning.
Running npm run test now succeeds on my setup.

[thetutlage]

Can we please have some tests that uses the pollIntervals, or maybe tests for the retry helper will be even better.

Otherwise the PR looks great. Thanks for taking out time 👍

[jeppester]

Can we please have some tests that uses the pollIntervals, or maybe tests for the retry helper will be even better.

I'll see what I can do 👍

Otherwise the PR looks great. Thanks for taking out time 👍

No problem. I'm just trying to make adonis look as good as possible now that I've convinced my team to try it out 😄

[jeppester]

@thetutlage I added tests for retryTest. Let me know what you think

[thetutlage]

@jeppester Can you please check why the tests are failing? They are failing just on Windows in CI

[jeppester]

@jeppester Can you please check why the tests are failing? They are failing just on Windows in CI

Apparently setTimeout is less precise on windows (or just on that specific machine, or due to load), so the allowed deviation is too small.

I'll make it 25 or maybe 50 instead of 5, so that the current failure is far within the limit.

[jeppester]

@thetutlage I updated the spec to allow more deviation.

I also improved the coverage and fixed a problem where the timeout was not correctly added to the error message. Please retry the specs.

[thetutlage]

Thanks @jeppester . Let's get it out in a couple of days

[jeppester]

Thanks @jeppester . Let's get it out in a couple of days

Should I update the documentation on japa.dev?

[thetutlage]

@jeppester Yeah, it will be nice if you can open a PR for docs too