feat: add share functionality tests and refactor test infrastructure#39
Conversation
📝 WalkthroughWalkthroughAdds environment and config templates, new share-focused end-to-end tests, introduces reported issue types to test data and translations, and refactors Playwright test fixtures (conftest) to remove webpack-script interception and simplify mobile/page setup; also small test, Makefile, and gitignore edits. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as rgba(66,133,244,0.5) Test Runner
participant Page as rgba(15,157,88,0.5) Browser Page
participant App as rgba(251,188,5,0.5) Web App (server)
participant Clipboard as rgba(219,68,55,0.5) Clipboard / OS
Test->>Page: open base URL / expand cluster / open marker popup
Page->>App: request popup data (locationId)
App-->>Page: respond with popup content
Test->>Page: click "Share"
Page->>Clipboard: write URL (contains ?locationId=)
Clipboard-->>Test: confirm copied value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/conftest.py (1)
4-9:⚠️ Potential issue | 🟡 MinorStale docstring reference to removed functionality.
Line 5 still mentions "Webpack script caching and interception" but this functionality was removed in this PR. Update the docstring to reflect the current fixture capabilities:
-Provides custom fixtures for: -- Webpack script caching and interception +Provides fixtures for: +- Page setup with HMR blocking - Window.open() stub tracking - Geolocation mocking - Performance tracking for stress tests +- Mobile device emulation
🤖 Fix all issues with AI agents
In `@e2e_test_data_template.json`:
- Around line 50-52: The JSON is invalid because there's a missing comma after
the array that precedes "reported_issue_types"; add the missing comma so the
"reported_issue_types" property (and the following "categories" object) are
properly separated—update the JSON around the "reported_issue_types" key to
ensure a comma separates it from the previous element, verifying
"reported_issue_types" and "categories" are valid sibling properties.
In `@Makefile`:
- Line 34: The command in the Makefile uses '$(CURDIR)/$(CONFIG_PATH)' which
breaks when CONFIG_PATH is already absolute; change it to use make's abspath
function so both relative and absolute CONFIG_PATH values work (replace the
concatenation with $(abspath $(CONFIG_PATH))) while keeping the existing
variables GOODMAP_PATH and CURDIR and preserving the quoted --app argument.
🧹 Nitpick comments (5)
tests/basic/test_location_buttons.py (1)
44-92: Clean refactoring to remove webpack_script dependency.The approach of using
add_init_scriptat the context level to mock geolocation denial before any page script runs is correct.Consider extracting the HMR blocking logic (lines 77-81) to reuse the
_block_hmrhelper fromconftest.pyfor consistency:from tests.conftest import _block_hmr # ... page = context.new_page() _block_hmr(page)This would reduce duplication, though the current inline approach works fine.
tests/basic/test_share.py (4)
38-58: Consider reusingget_rightmost_markerhelper.This logic to find and click the rightmost marker duplicates the existing
get_rightmost_markerhelper intests/helpers.py. Consider importing and using it:from tests.helpers import get_rightmost_marker # Then use: rightmost_marker = get_rightmost_marker(page) if rightmost_marker: rightmost_marker.click()This would reduce duplication and improve maintainability.
99-100: Remove unuseddevice_nameparameter.The
device_nameparameter is not used in the test method body. Themobile_pagefixture already handles device selection via the parametrization. You can either:
- Switch to indirect parametrization (recommended by conftest):
- `@pytest.mark.parametrize`("device_name", ALL_MOBILE_DEVICES) - def test_share_button_triggers_native_share(self, mobile_page: Page, device_name: str): + `@pytest.mark.parametrize`("mobile_page", ALL_MOBILE_DEVICES, indirect=True) + def test_share_button_triggers_native_share(self, mobile_page: Page):
- Or use
device_namein the test (e.g., for logging/assertions) to justify keeping it.
128-148: Same duplication: consider reusingget_rightmost_markerhelper.This is the same rightmost marker logic duplicated again. Extract to a shared helper or reuse the existing one from
tests/helpers.py.
164-165: Same issue: unuseddevice_nameparameter.Apply the same fix as suggested for line 100 - either switch to indirect parametrization or use the parameter.
🧪 E2E Test Results📊 View full workflow run
|
🧪 E2E Test Results📊 View full workflow run 📊 E2E Stress Test Performance✅ Status: PASSED (12738.03ms max < 25000ms limit)
📈 Individual Run Times
|
🧪 E2E Test Results📊 View full workflow run 📊 E2E Stress Test Performance✅ Status: PASSED (11809.38ms max < 25000ms limit)
📈 Individual Run Times
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/stress/test_stress.py`:
- Around line 103-119: The cluster selector is wrong and the click is unguarded:
update the clusters locator from ".marker-cluster" to ".leaflet-marker-cluster"
and also change the individual_markers selector from
".leaflet-marker-icon:not(.marker-cluster)" to
".leaflet-marker-icon:not(.leaflet-marker-cluster)"; before calling
clusters.first.click() (in the loop that uses clusters, individual_markers,
max_clicks) add a defensive check that clusters.count() > 0 (and raise/assert or
break with a clear message if none) so you never call .first.click() on an empty
locator.
🧪 E2E Test Results📊 View full workflow run 📊 E2E Stress Test Performance✅ Status: PASSED (11805.64ms max < 25000ms limit)
📈 Individual Run Times
|
raven-wing
changed the title
🧪 E2E Test Results📊 View full workflow run 📊 E2E Stress Test Performance✅ Status: PASSED (12680.47ms max < 25000ms limit)
📈 Individual Run Times
|
1 participant
Summary by CodeRabbit
New Features
Translations
Tests
Chores