feat(testUtils): add navigation option to grantAccessIfNeeded and imp…#29
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Playwright helper utilities to support “grant access” flows without forcing a re-navigation (preserving in-test state), and introduces a new helper for dismissing a OneTrust cookie dialog.
Changes:
- Add an optional
{ shouldNavigate }option tograntAccessIfNeeded(defaulting totrue) so callers can skippage.goto(...). - Add
dismissCookieDialog(page, timeoutMs)helper to accept cookies if the OneTrust banner/dialog appears. - Document the new
grantAccessIfNeeded(..., { shouldNavigate: false })usage in the README.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/testUtils.js | Adds optional navigation behavior to grantAccessIfNeeded and introduces dismissCookieDialog. |
| README.md | Documents skipping navigation when calling grantAccessIfNeeded. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sebastianchristopher
requested review from
buckett,
lardo-de-arnaud and
nicholaswilson100
| const { shouldNavigate = true } = options | ||
| if (shouldNavigate) { | ||
| await page.goto(toolUrl) | ||
| } |
There was a problem hiding this comment.
I probably would have made toolUrl optional, if it's not supplied then it doesn't navigate, if it's supplied then do. Generally I think it would be clearer if almost all uses of this method just did the navigate themselves as there's not much benefit of having it in this method.
| /** | ||
| * Dismiss the OneTrust cookie dialog by accepting cookies if the banner appears. | ||
| * @param {import('@playwright/test').Page} page - Playwright page | ||
| * @param {number} [timeoutMs=3000] - Max wait for the dialog accept button | ||
| * @returns {Promise<void>} | ||
| */ |
There was a problem hiding this comment.
I wonder if this would be better handled by https://playwright.dev/docs/api/class-page#page-add-locator-handler which allows us to just accept the dialog if it ever appears and not explicitly wait for it?
There was a problem hiding this comment.
Is this the case for the beta banner as well?
There was a problem hiding this comment.
Yeah, I think so, not essential, I was just commenting because it was new code.
…lement cookie dialog dismissal
The original code was written for the pre-test auth stage, so assumed that the
grantAccessIfNeededshould also navigate to the page. This doesn't work with this tool, which has some steps before Grant Access which get wiped if you re-navigate to the page. This PR adds support for cases where the flow is already on the correct page, but maintains backwards compatibility (don't need to touch existing tests).Also adds another method to dismiss the new(?) cookie dialog.