Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: page.pause() timeout reset #2157

Merged
merged 1 commit into from
Nov 13, 2023
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
7 changes: 5 additions & 2 deletions playwright/_impl/_browser_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -248,15 +248,18 @@ def set_default_navigation_timeout(self, timeout: float) -> None:
def _set_default_navigation_timeout_impl(self, timeout: Optional[float]) -> None:
self._timeout_settings.set_default_navigation_timeout(timeout)
self._channel.send_no_reply(
"setDefaultNavigationTimeoutNoReply", dict(timeout=timeout)
"setDefaultNavigationTimeoutNoReply",
{} if timeout is None else {"timeout": timeout},
Copy link
Member Author

Choose a reason for hiding this comment

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

The issue was that on protocol level we don't allow timeout to be null.

Copy link
Member

Choose a reason for hiding this comment

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

Should we consider serializing None as undefined or too many things break?

Copy link
Member Author

Choose a reason for hiding this comment

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

too many things would break I believe!

)

def set_default_timeout(self, timeout: float) -> None:
return self._set_default_timeout_impl(timeout)

def _set_default_timeout_impl(self, timeout: Optional[float]) -> None:
self._timeout_settings.set_default_timeout(timeout)
self._channel.send_no_reply("setDefaultTimeoutNoReply", dict(timeout=timeout))
self._channel.send_no_reply(
"setDefaultTimeoutNoReply", {} if timeout is None else {"timeout": timeout}
)

@property
def pages(self) -> List[Page]:
Expand Down
30 changes: 30 additions & 0 deletions tests/async/test_page.py
Original file line number Diff line number Diff line change
Expand Up @@ -1294,3 +1294,33 @@ def binding(source, o):
await page.expose_binding("log", lambda source, o: binding(source, o))
await page.evaluate("const a = {}; a.b = a; window.log(a)")
assert binding_values[0]["b"] == binding_values[0]


async def test_page_pause_should_reset_default_timeouts(
page: Page, headless: bool, server: Server
) -> None:
if not headless:
pytest.skip()

await page.goto(server.EMPTY_PAGE)
page.pause()
with pytest.raises(Error, match="Timeout 30000ms exceeded."):
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to slow down all tests by 30s?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think no other option instead of hacking around the impl for testing.

await page.get_by_text("foo").click()


async def test_page_pause_should_reset_custom_timeouts(
page: Page, headless: bool, server: Server
) -> None:
if not headless:
pytest.skip()

page.set_default_timeout(123)
page.set_default_navigation_timeout(456)
await page.goto(server.EMPTY_PAGE)
page.pause()
with pytest.raises(Error, match="Timeout 123ms exceeded."):
await page.get_by_text("foo").click()

server.set_route("/empty.html", lambda route: None)
with pytest.raises(Error, match="Timeout 456ms exceeded."):
await page.goto(server.EMPTY_PAGE)
19 changes: 11 additions & 8 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
_dirname = get_file_dirname()


def pytest_generate_tests(metafunc: Any) -> None:
def pytest_generate_tests(metafunc: pytest.Metafunc) -> None:
if "browser_name" in metafunc.fixturenames:
browsers = metafunc.config.option.browser or ["chromium", "firefox", "webkit"]
metafunc.parametrize("browser_name", browsers, scope="session")
Expand All @@ -54,11 +54,14 @@ def assetdir() -> Path:


@pytest.fixture(scope="session")
def launch_arguments(pytestconfig: Any) -> Dict:
def headless(pytestconfig: pytest.Config) -> bool:
return not (pytestconfig.getoption("--headed") or os.getenv("HEADFUL", False))


@pytest.fixture(scope="session")
def launch_arguments(pytestconfig: pytest.Config, headless: bool) -> Dict:
return {
"headless": not (
pytestconfig.getoption("--headed") or os.getenv("HEADFUL", False)
),
"headless": headless,
"channel": pytestconfig.getoption("--browser-channel"),
}

Expand Down Expand Up @@ -92,12 +95,12 @@ def after_each_hook() -> Generator[None, None, None]:


@pytest.fixture(scope="session")
def browser_name(pytestconfig: Any) -> None:
def browser_name(pytestconfig: pytest.Config) -> None:
return pytestconfig.getoption("browser")


@pytest.fixture(scope="session")
def browser_channel(pytestconfig: Any) -> None:
def browser_channel(pytestconfig: pytest.Config) -> None:
return pytestconfig.getoption("--browser-channel")


Expand Down Expand Up @@ -169,7 +172,7 @@ def skip_by_platform(request: pytest.FixtureRequest) -> None:
pytest.skip(f"skipped on this platform: {sys.platform}")


def pytest_addoption(parser: Any) -> None:
def pytest_addoption(parser: pytest.Parser) -> None:
group = parser.getgroup("playwright", "Playwright")
group.addoption(
"--browser",
Expand Down
30 changes: 30 additions & 0 deletions tests/sync/test_page.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,33 @@ def test_emitted_for_domcontentloaded_and_load(page: Page, server: Server) -> No
page.goto(server.EMPTY_PAGE)
assert isinstance(dom_info.value, Page)
assert isinstance(load_info.value, Page)


def test_page_pause_should_reset_default_timeouts(
page: Page, headless: bool, server: Server
) -> None:
if not headless:
pytest.skip()

page.goto(server.EMPTY_PAGE)
page.pause()
with pytest.raises(Error, match="Timeout 30000ms exceeded."):
page.get_by_text("foo").click()


def test_page_pause_should_reset_custom_timeouts(
page: Page, headless: bool, server: Server
) -> None:
if not headless:
pytest.skip()

page.set_default_timeout(123)
page.set_default_navigation_timeout(456)
page.goto(server.EMPTY_PAGE)
page.pause()
with pytest.raises(Error, match="Timeout 123ms exceeded."):
page.get_by_text("foo").click()

server.set_route("/empty.html", lambda route: None)
with pytest.raises(Error, match="Timeout 456ms exceeded."):
page.goto(server.EMPTY_PAGE)
Loading