Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/integration/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def poll_for_navigation(
"""
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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 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.

Suggested change
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!



def n_expected_events(exp_event_order: Sequence[str | set[str]]) -> int:
Expand Down
50 changes: 50 additions & 0 deletions tests/units/integration/test_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
"""Unit tests for integration test utilities."""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 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.


from __future__ import annotations

from collections.abc import Callable

import pytest

from tests.integration import utils


class _FakePage:
"""Minimal page stub for testing poll_for_navigation."""

def __init__(self) -> None:
"""Initialize fake page state."""
self.url = "http://localhost:3000/"
self.wait_calls: list[tuple[Callable[[str], bool], float]] = []

def wait_for_url(self, predicate: Callable[[str], bool], timeout: float) -> None:
"""Record wait_for_url calls and validate the URL-change predicate.

Args:
predicate: URL-matcher callback passed by poll_for_navigation.
timeout: Timeout in milliseconds.
"""
self.wait_calls.append((predicate, timeout))
assert not predicate("http://localhost:3000/")
assert predicate("http://localhost:3000/next")


def test_poll_for_navigation_uses_playwright_wait_for_url(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""poll_for_navigation should delegate URL waiting to Playwright."""

def _unexpected_expect(*_args: object, **_kwargs: object) -> None:
msg = "poll_for_navigation should not call AppHarness.expect"
raise AssertionError(msg)

monkeypatch.setattr(utils.AppHarness, "expect", _unexpected_expect)

page = _FakePage()
with utils.poll_for_navigation(page, timeout=2.5):
# The helper snapshots the current URL before yielding.
pass

assert len(page.wait_calls) == 1
_, timeout_ms = page.wait_calls[0]
assert timeout_ms == pytest.approx(2500.0)