test: handle "Cookie Preferences and Opt-Out Rights" modal#1276
test: handle "Cookie Preferences and Opt-Out Rights" modal#1276rsoaresd merged 1 commit intocodeready-toolchain:masterfrom
Conversation
WalkthroughModified the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
rsoaresd
changed the title
coderabbitai
Bot
added
the
test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testsupport/devsandbox-dashboard/setup.go`:
- Around line 134-146: The OR locator is non-deterministic and can throw a
strict-mode error if both buttons exist; replace consentButton :=
agreeProceed.Or(acceptDefault).Click() with an explicit try-then-fallback: first
attempt agreeProceed.Click(), if that returns an error (or a
strict-mode/multiple-matches error), then attempt acceptDefault.Click(), and
assert no error for whichever click succeeds (use require.NoError on each
attempted click). Use the existing agreeProceed, acceptDefault, contentFrame and
require.NoError symbols to locate and implement the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4ceaeeae-417e-489e-bbfb-9ad07eae41a7
📒 Files selected for processing (1)
testsupport/devsandbox-dashboard/setup.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: govulncheck
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
testsupport/devsandbox-dashboard/setup.go
| // TrustArc can show different modals (e.g. full consent vs. simplified); accept whichever button is present. | ||
| agreeProceed := contentFrame.GetByRole("button", playwright.FrameLocatorGetByRoleOptions{ | ||
| Name: "Agree and proceed with", | ||
| }) | ||
|
|
||
| err = agreeButton.Click() | ||
| // to some us states, like texas, it appears the "Cookie Preferences and Opt-Out Rights" modal | ||
| acceptDefault := contentFrame.GetByRole("button", playwright.FrameLocatorGetByRoleOptions{ | ||
| Name: "Accept default", | ||
| }) | ||
| consentButton := agreeProceed.Or(acceptDefault) | ||
|
|
||
| err = consentButton.Click() | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Playwright (especially playwright-go), if Locator.Or(otherLocator) matches two elements, can Locator.Click() fail due to strict mode? What is the recommended pattern to try locator A first and fall back to locator B?
💡 Result:
Yes. In Playwright, locators are strict: if you call Locator.click (including in playwright-go) and that locator resolves to more than one element, Playwright can throw a “strict mode violation”. Using locator.or (or Locator.Or in playwright-go) creates a locator that matches one or both alternatives; if both alternatives are present at the same time, the combined locator may match multiple elements and then click will fail due to strictness. Recommended pattern (try A, fall back to B): avoid calling click on an OR-locator that may match multiple elements. Instead, explicitly choose which one to click by checking visibility (or count) on each alternative and then clicking the single matching locator. Example (Node/Playwright JS pattern, same idea applies to playwright-go): const a = page.getByRole('button', { name: 'A' }); const b = page.getByRole('button', { name: 'B' }); if (await a.isVisible) await a.click; else await b.click; If you specifically use an OR locator, Playwright’s own docs show using .first to prevent the strict mode issue when the OR matches multiple elements, but they caution that .first is not recommended because page changes can make it click an unintended element. Example with OR: await expect(a.or(b).first).toBeVisible; // then decide/click based on which one is visible, as shown in the docs.
Citations:
- 1: https://playwright.dev/docs/locators
- 2: https://github.com/playwright-community/playwright-go/blob/main/locator.go
- 3: https://playwright.dev/docs/api/class-locator
🏁 Script executed:
#!/bin/bash
# Verify the code snippet exists at the specified lines
if [ -f "testsupport/devsandbox-dashboard/setup.go" ]; then
sed -n '130,150p' testsupport/devsandbox-dashboard/setup.go | cat -n
else
echo "File not found"
fiRepository: codeready-toolchain/toolchain-e2e
Length of output: 1065
Make consent-button fallback deterministic to avoid flaky modal handling.
The agreeProceed.Or(acceptDefault).Click() pattern at line 145 can fail in strict mode if both buttons are present simultaneously, causing test flakiness. Playwright does not guarantee which locator in an OR combination is clicked, and when multiple elements match, Click() throws a strict mode violation. Use explicit try-then-fallback instead.
🔧 Proposed fix
- consentButton := agreeProceed.Or(acceptDefault)
-
- err = consentButton.Click()
+ err = agreeProceed.Click(playwright.LocatorClickOptions{
+ Timeout: playwright.Float(2000),
+ })
+ if err != nil {
+ err = acceptDefault.Click(playwright.LocatorClickOptions{
+ Timeout: playwright.Float(2000),
+ })
+ }
require.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testsupport/devsandbox-dashboard/setup.go` around lines 134 - 146, The OR
locator is non-deterministic and can throw a strict-mode error if both buttons
exist; replace consentButton := agreeProceed.Or(acceptDefault).Click() with an
explicit try-then-fallback: first attempt agreeProceed.Click(), if that returns
an error (or a strict-mode/multiple-matches error), then attempt
acceptDefault.Click(), and assert no error for whichever click succeeds (use
require.NoError on each attempted click). Use the existing agreeProceed,
acceptDefault, contentFrame and require.NoError symbols to locate and implement
the change.
There was a problem hiding this comment.
As far as I know, either appears one or another
There was a problem hiding this comment.
@rsoaresd, that's a good clarification — if the two TrustArc modal variants are mutually exclusive (only one appears at a time, never both), then the .Or() locator will always resolve to exactly one element, and the strict mode concern doesn't apply. The current code is safe as-is. Thanks for the context!
📝 Learnings were identified and not saved (knowledge base disabled). Enable
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, metlos, mfrancisc, MikelAlejoBR, rsoaresd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
The "Cookie Preferences and Opt-Out Rights Your Choices About Cookies on this Site" modal appears depending on the country/state (for example, it appears for us texas), which breaks the current Developer Sandbox E2E tests against prod (pls, check this example). So, we should handle it.
Issue ticket number and link
SANDBOX-1630
Assisted-by: Cursor
Summary by CodeRabbit