fix: improve E2E test stability with proper element waits and video recording#37
Conversation
…ests (Problematy#36)" This reverts commit 91e881e.
📝 WalkthroughWalkthroughSwitches frontend test assets to minified bundles, refines Playwright mobile contexts to record per-test videos with conditional retention, adds a frontend startup delay and failure-video artifact upload in CI, replaces force-clicks with wait-then-click in mobile tests, and adds a comprehensive left-panel test suite across viewports. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI runner
participant Frontend as Frontend server
participant Playwright as Playwright runner
participant Browser as Browser/device
participant Artifacts as Artifact storage
CI->>Frontend: run `make serve-prod`
CI->>CI: sleep 3s
CI->>Playwright: start tests (uses index.min.js)
Playwright->>Browser: create context (mobile? include record_video_dir & size)
Browser->>Playwright: execute tests (recording per-test)
alt test failed
Playwright->>CI: emit failure report (pytest_runtest_makereport)
CI->>Artifacts: upload `e2e-tests/test-results/**/*.webm`
else test passed
Playwright->>Playwright: delete per-test video if retain-on-failure
end
CI->>Frontend: stop server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧪 E2E Test Results📊 View full workflow run
|
🧪 E2E Test Results📊 View full workflow run
|
🧪 E2E Test Results📊 View full workflow run
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/basic/test_left_panel.py`:
- Around line 60-78: The test_no_page_scrollbar_on_desktop currently uses
document.body.scrollHeight > document.body.clientHeight which can be true even
with overflow:hidden; change the assertion to attempt an actual scroll and
verify it fails: in the test_no_page_scrollbar_on_desktop function, after
waiting for "#filter-form", run a page.evaluate that records initial scroll
position (window.scrollY or document.documentElement.scrollTop), attempts to
scroll (e.g. window.scrollTo(0, 100) or set document.documentElement.scrollTop),
then read the scroll position again and assert it did not change; this ensures
the page cannot be scrolled rather than relying on scrollHeight comparisons.
- Line 145: The unused parameter device_name in test functions (e.g.,
test_panel_hidden_by_default_on_mobile) is triggering ARG002; at the top of each
affected test body (lines indicated: tests named
test_panel_hidden_by_default_on_mobile and the other parametrized tests at the
given positions) add a no-op assignment like _ = device_name # required by
parametrization; intentionally unused to silence the lint warning; update the
same pattern in all listed tests (lines 145, 162, 182, 200, 226, 256) so the
pytest parametrization remains intact while ARG002 is resolved.
🧹 Nitpick comments (1)
tests/basic/test_left_panel.py (1)
80-114: Ensure the scroll test actually exercises overflow.If content no longer overflows, this test passes without validating scrolling. Add an explicit assertion so failures are actionable.
♻️ Suggested tweak
scroll_info = panel_body.evaluate( """el => ({ clientHeight: el.clientHeight, scrollHeight: el.scrollHeight, hasOverflow: el.scrollHeight > el.clientHeight })""" ) - # If there's overflow, verify scrolling works - if scroll_info["hasOverflow"]: - # Get initial scroll position - initial_scroll = panel_body.evaluate("el => el.scrollTop") - - # Scroll down - panel_body.evaluate("el => el.scrollBy(0, 200)") - - # Verify scroll position changed - new_scroll = panel_body.evaluate("el => el.scrollTop") - assert new_scroll > initial_scroll, "Panel body should be scrollable" + assert scroll_info["hasOverflow"], ( + "Expected panel content to overflow for the scroll test; " + "reduce the viewport height or add fixtures if needed." + ) + + # Verify scrolling works + initial_scroll = panel_body.evaluate("el => el.scrollTop") + panel_body.evaluate("el => el.scrollBy(0, 200)") + new_scroll = panel_body.evaluate("el => el.scrollTop") + assert new_scroll > initial_scroll, "Panel body should be scrollable"
🧪 E2E Test Results📊 View full workflow run
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/basic/test_location_buttons.py`:
- Around line 248-260: The 200ms timeouts used for tooltip visibility checks are
too short and inconsistent with the rest of the file; update the two expect(...)
calls that use timeout=200 (the visibility and not_to_be_visible assertions
referencing the tooltip locator in tests/basic/test_location_buttons.py using
mobile_page and tooltip) to use a larger timeout (at minimum 1000ms, preferably
5000ms to match surrounding patterns and CI reliability).
In `@tests/conftest.py`:
- Around line 276-294: Replace request.node.name with request.node.nodeid when
constructing the test-specific video directory to avoid name collisions; update
the assignment that builds test_name (currently using request.node.name and
replace("[", "-").replace("]", "")) to use request.node.nodeid and sanitize path
separators and parameter brackets (e.g., replace path separators and brackets)
before creating record_video_dir in the context creation block so
record_video_dir remains unique per test run.
🧹 Nitpick comments (1)
tests/basic/test_location_buttons.py (1)
220-232: Gate the 2s debug pause behind an environment variable to avoid slowing every mobile test run.The unconditional
wait_for_timeout(2000)will add noticeable runtime across all mobile tests. Since the code is explicitly marked for removal after debugging, it should be conditional. The fixture pattern already uses configuration-based conditionals (e.g., video recording), so the approach is consistent with the codebase.🛠️ Proposed adjustment
+import os import pytest from playwright.sync_api import Page, expect @@ - # DEBUG: Keep tooltip visible for video capture - remove after debugging - mobile_page.wait_for_timeout(2000) + # DEBUG: Keep tooltip visible for video capture - remove after debugging + if os.getenv("DEBUG_VIDEO_PAUSE") == "1": + mobile_page.wait_for_timeout(2000)
🧪 E2E Test Results📊 View full workflow run
|
🧪 E2E Test Results📊 View full workflow run
|
raven-wing
changed the title
🧪 E2E Test Results📊 View full workflow run
|
🧪 E2E Test Results📊 View full workflow run 📊 E2E Stress Test Performance✅ Status: PASSED (11732.11ms max < 25000ms limit)
📈 Individual Run Times
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/conftest.py`:
- Around line 123-132: The pytest hook function pytest_runtest_makereport
currently declares an unused parameter call causing Ruff ARG001; rename the
parameter to _call (i.e., def pytest_runtest_makereport(item, _call):) so
linters recognize it as intentionally unused, or alternatively add a targeted
noqa comment for ARG001 on the function signature—update the function
declaration and any internal references to the parameter name accordingly.
- Around line 307-316: The retain-on-failure logic only checks
request.node.rep_call for failures and should instead consider all test phases;
update the block that inspects video_path/video_option to determine test_failed
by checking request.node.rep_setup, request.node.rep_call, and
request.node.rep_teardown (each may or may not exist) and treat the test as
failed if any of those report .failed; keep the existing deletion/unlink and
empty-directory removal behavior but skip deletion when any phase failed so
videos are preserved.
♻️ Duplicate comments (1)
tests/conftest.py (1)
255-266: Avoid video dir collisions by using nodeid (not name).
request.node.namecan collide across parametrized or same-named tests, which risks overwriting videos.🛠️ Proposed fix
- test_name = request.node.name.replace("[", "-").replace("]", "") - record_video_dir = str(Path(output_dir) / test_name) + nodeid = request.node.nodeid + safe_nodeid = ( + nodeid.replace("::", "__") + .replace("/", "__") + .replace("[", "-") + .replace("]", "") + ) + record_video_dir = str(Path(output_dir) / safe_nodeid)Also applies to: 272-273
🧪 E2E Test Results📊 View full workflow run 📊 E2E Stress Test Performance✅ Status: PASSED (10816.12ms max < 25000ms limit)
📈 Individual Run Times
|
🧪 E2E Test Results📊 View full workflow run 📊 E2E Stress Test Performance✅ Status: PASSED (11138.4ms max < 25000ms limit)
📈 Individual Run Times
|
🧪 E2E Test Results📊 View full workflow run
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/basic/test_left_panel.py`:
- Around line 308-309: The test clicks the toggle button immediately which can
be flaky; before calling toggle_button.click() in
tests/basic/test_left_panel.py, wait for the element to be visible/attached
using toggle_button.wait_for(...) (e.g., state="visible" or "attached") to
mirror the mobile tests pattern, then perform toggle_button.click(); update the
code around the toggle_button locator and click to include this wait to ensure
stability.
- Around line 236-237: The test uses
mobile_page.locator('button[aria-label="Toggle left panel"]') assigned to
toggle_button and calls toggle_button.click() directly; add a wait for
visibility to match other tests by calling
toggle_button.wait_for(state="visible") before toggle_button.click() so the
button is guaranteed ready and flakiness is avoided.
- Around line 266-267: The click on the left-panel toggle is missing an explicit
wait which can cause flakiness; before calling toggle_button.click() (where
toggle_button is created via mobile_page.locator('button[aria-label="Toggle left
panel"]')), wait for the locator to be visible/enabled using the
locator.wait_for(...) API (or mobile_page.wait_for_selector with the same
selector), then perform the click to ensure stability.
🧹 Nitpick comments (1)
tests/basic/test_left_panel.py (1)
370-375: Test assertion may always pass in Chromium — consider browser-specific handling.Allowing empty string means this test passes even when no custom scrollbar styling is applied in Chromium. Since
scrollbar-widthis Firefox-only, you might consider:
- Skip this test on Chromium, or
- Add a separate Chromium-specific check using CSS selector inspection for
::-webkit-scrollbarrulesThis is a minor observation — the current approach is documented and the test still provides value for Firefox.
🧪 E2E Test Results📊 View full workflow run 📊 E2E Stress Test Performance✅ Status: PASSED (13194.93ms max < 25000ms limit)
📈 Individual Run Times
|
🧪 E2E Test Results📊 View full workflow run 📊 E2E Stress Test Performance✅ Status: PASSED (11211.12ms max < 25000ms limit)
📈 Individual Run Times
|
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.