Skip to content

test(e2e): wait for the facet panel to settle before asserting - #6636

Open
aditya-786 wants to merge 4 commits into
keephq:mainfrom
aditya-786:fix/flaky-facet-assertion-strict-mode
Open

test(e2e): wait for the facet panel to settle before asserting#6636
aditya-786 wants to merge 4 commits into
keephq:mainfrom
aditya-786:fix/flaky-facet-assertion-strict-mode

Conversation

@aditya-786

@aditya-786 aditya-786 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Why are these changes needed?

test_filter_search_timeframe_combination_with_queryparams currently fails on every run of run-tests (mysql, postgresql and sqlite), so it blocks CI on all open PRs.

I root-caused it from the CI failure artifacts. The failure is a Playwright strict-mode violation, not a product bug:

LocatorAssertions.to_be_visible: Error: strict mode violation:
locator("[data-testid='facet']").filter(has_text="severity") resolved to 2 elements

What happens:

  • The failing assertion is the assert_facet(...) call right after browser.reload(), where the filters are restored from the URL query params.
  • During that restore the facets panel briefly re-renders, so the Severity facet exists twice for a moment.
  • assert_facet locates the facet with a substring has_text="severity" match and immediately calls to_be_visible(). With two matching elements, strict mode raises.
  • The saved DOM dump has only one Severity facet, confirming the duplicate is transient. But it outlives the default 5s expect timeout (there is no global timeout override), which is why to_be_visible() cannot ride it out and the failure is deterministic.

What changed

assert_facet now waits for the facet locator to settle to a single element before asserting visibility:

facet_locator = browser.locator("[data-testid='facet']", has_text=facet_name)
expect(facet_locator).to_have_count(1, timeout=15000)
expect(facet_locator).to_be_visible()

The 15s timeout matches the existing count assertion in this same file (assert_alerts_by_column). Tests where only one facet is ever present are unaffected (the count is already 1, so the wait returns immediately).

The incidents test file (test_filtering_sort_search_on_incidents.py) has an identical assert_facet helper with the same transient-duplicate issue, so it gets the same guard. Verified on this PR's own CI: with only the alerts fix, test_filter_search_timeframe_combination_with_queryparams passed and the failure moved to the incidents twin test_filter_timeframe_combination_with_queryparams, so both copies are fixed here.

Note: this is a browser e2e test that needs the full docker stack, so I could not run it locally. The diagnosis is from the CI artifacts (DOM dump, console log, traceback) and the fix uses the settle-then-assert pattern already used elsewhere in this suite. The verification is this PR's own run-tests run.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests

After a reload or a timeframe change the facets panel briefly re-renders
the same facet twice. assert_facet located the facet with a substring
has_text match and asserted visibility immediately, so the strict locator
resolved to two elements and raised a strict mode violation. The duplicate
outlives the default 5s expect timeout, which is why
test_filter_search_timeframe_combination_with_queryparams fails on every run.

Wait for the facet locator to settle to a single element before asserting,
using the 15s timeout already used for the count assertion in this file.
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jul 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged

The incidents assert_facet is an identical copy with the same transient
double-render issue (test_filter_timeframe_combination_with_queryparams
hits it after reload), so it gets the same settle-to-one-facet guard.
@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.42%. Comparing base (98dbd7e) to head (4889dbc).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6636       +/-   ##
===========================================
+ Coverage   30.49%   46.42%   +15.92%     
===========================================
  Files         101      178       +77     
  Lines       11741    18685     +6944     
===========================================
+ Hits         3581     8674     +5093     
- Misses       8160    10011     +1851     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@shahargl shahargl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please make cicd pass

The settle-wait was not enough. CI showed to_have_count(1) passing and the
very next to_be_visible() failing with a strict mode violation on two
elements, both of them the same facet (get_by_text("Status").first and
.nth(1)), so the panel re-renders between two consecutive assertions and
waiting once cannot hold.

Scope the facet locators to the first match instead. The locator is then
never ambiguous, and Playwright re-queries it on each retry so the value
and counter assertions still converge on the settled panel. The
not_to_be_visible checks after deleting a facet are left alone, since
first would change what they assert.
@aditya-786

Copy link
Copy Markdown
Contributor Author

Thanks, pushed a fix.

The previous approach was not enough and CI showed exactly why. My settle-wait (to_have_count(1)) passed, and the very next to_be_visible() failed with a strict mode violation on two elements, both of them the same facet:

1) <div data-testid="facet" ...> aka get_by_text("Status").first
2) <div data-testid="facet" ...> aka get_by_text("Status").nth(1)

So the facet panel re-renders between two consecutive assertions and waiting once for it to settle cannot hold. The duplicate is also gone by the time the failure artifact is captured, which is why the saved DOM only ever shows one facet.

Now the facet locators are scoped to the first match, so the locator is never ambiguous. Playwright re-queries it on every retry, so the value and counter assertions still converge on the settled panel. I applied it to both the alerts and incidents copies of assert_facet and select_one_facet_option, plus display_all_available_incidents, since the same duplicate can hit any of them. The not_to_be_visible checks after deleting a facet are deliberately left alone, because .first would change what they assert.

Worth noting the underlying behaviour is on the UI side: the facets panel briefly mounts two copies of the same facet while re-rendering after a reload or timeframe change. This PR only makes the tests robust to it. Happy to open a separate issue for the duplicate render if you want it tracked.

@aditya-786

Copy link
Copy Markdown
Contributor Author

CI update on the latest commit (7e59adf):

  • run-sqlite-without-redis passed
  • run-postgresql-without-redis passed
  • run-mysql-with-redis failed, but not in a test

Both of the tests this PR targets now pass on both databases that completed:

PASSED tests/e2e_tests/incidents_alerts_tests/test_filtering_sort_search_on_alerts.py::test_filter_search_timeframe_combination_with_queryparams
PASSED tests/e2e_tests/incidents_alerts_tests/test_filtering_sort_search_on_incidents.py::test_filter_timeframe_combination_with_queryparams

The mysql job failed at step 16, Wait for services to be ready, with the 120 second health check timeout. No test ran in that job at all, the log contains zero PASSED or FAILED lines, so it is the compose stack not coming up rather than anything in this change. A re-run should clear it, and I do not have permission to trigger one on this repo, so could you kick it off when you get a chance?

The only other red check is Validate PR to Issue link. That one is deliberate: this PR references the flaky tests rather than closing an issue, and I did not want to attach a closing keyword to an unrelated issue. Happy to file a short bug issue for the duplicate facet render and link it if you would prefer the check green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants