test(integration): use Playwright wait_for_url in poll_for_navigation#6385
Conversation
Greptile SummaryThis PR replaces custom
Confidence Score: 4/5Safe to merge after adding the missing init.py to the new test package directory. A single P1 finding (missing init.py) caps the score at 4. The production code change is minimal and correct; the test logic is sound. tests/units/integration/ — needs init.py to match the package layout of all other tests/units/ subdirectories. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant poll_for_navigation
participant Page
Caller->>poll_for_navigation: enter context (page, timeout=N)
poll_for_navigation->>Page: read page.url → prev_url
poll_for_navigation-->>Caller: yield
Note over Caller: performs navigation action
Caller->>poll_for_navigation: exit context
poll_for_navigation->>Page: wait_for_url(λ url: url != prev_url, timeout=N*1000ms)
Page-->>poll_for_navigation: URL changed (or timeout)
poll_for_navigation-->>Caller: return
Reviews (1): Last reviewed commit: "test(integration): use Playwright wait_f..." | Re-trigger Greptile |
| @@ -0,0 +1,50 @@ | |||
| """Unit tests for integration test utilities.""" | |||
There was a problem hiding this comment.
Missing
__init__.py for new test package
All sibling subdirectories under tests/units/ (e.g., compiler/, components/, utils/, etc.) include an __init__.py to declare a proper Python package. The new tests/units/integration/ directory is missing one. When pytest collects the full tests/units/ suite using package-mode discovery (triggered by the existing tests/units/__init__.py), this inconsistency can cause ImportError or silent test-collection failures for this module.
| prev_url = page.url | ||
| yield | ||
| AppHarness.expect(lambda: prev_url != page.url, timeout=timeout) | ||
| page.wait_for_url(lambda url: url != prev_url, timeout=timeout * 1000) |
There was a problem hiding this comment.
Missing comment explaining seconds-to-milliseconds conversion
The * 1000 factor converts the caller-facing timeout (seconds) to the milliseconds unit that Playwright's wait_for_url expects, but there is no inline comment to document this. Per the team's convention for time-based calculations, a brief note should accompany the conversion.
| page.wait_for_url(lambda url: url != prev_url, timeout=timeout * 1000) | |
| page.wait_for_url(lambda url: url != prev_url, timeout=timeout * 1000) # seconds → milliseconds |
Rule Used: When using time-based calculations in code, includ... (source)
Learned From
reflex-dev/flexgen#2190
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
16ff9dd
into
reflex-dev:claude/selenium-to-playwright-migration-7qluV
Summary\nFollow-up to #6378 to remove custom URL polling from integration helpers.\n\npoll_for_navigation now uses Playwright's native page.wait_for_url(...) with a URL-change predicate instead of AppHarness.expect(...) polling.\n\n## Changes\n- Updated ests/integration/utils.py::poll_for_navigation to delegate to page.wait_for_url.\n- Added regression unit test in ests/units/integration/test_utils.py that fails if AppHarness.expect is called and verifies timeout conversion to milliseconds.\n\n## Validation\n- uv run pytest tests/units/integration/test_utils.py -q\n- uv run ruff check tests/integration/utils.py tests/units/integration/test_utils.py\n\nCloses #6384\n