refactor(tests): remove test smells from sync tests

tests/sync/test_assertions.py

@@ -14,6 +14,7 @@
 
 import datetime
 import re
+from typing import Any, Callable, Generator
 
 import pytest
 
@@ -355,7 +356,16 @@ def test_ignore_case_regex(page: Page, server: Server, method: str) -> None:
         )
     assert expected_error_msg in str(excinfo.value)
 
-    # Array Variants
+
+@pytest.mark.parametrize(
+    "method",
+    ["to_have_text", "to_contain_text"],
+)
+def test_ignore_case_regex_array_variants(
+    page: Page, server: Server, method: str
+) -> None:
+    page.goto(server.EMPTY_PAGE)
+    page.set_content("<div id=target>apple BANANA</div><div>orange</div>")
     getattr(expect(page.locator("div")), method)(
         [re.compile("apple BANANA"), re.compile("orange")]
     )
@@ -368,6 +378,7 @@ def test_ignore_case_regex(page: Page, server: Server, method: str) -> None:
             [re.compile("apple banana"), re.compile("ORANGE")],
             timeout=300,
         )
+    expected_error_msg = method.replace("_", " ")
     assert expected_error_msg in str(excinfo.value)
     # overrides regex flag
     with pytest.raises(AssertionError) as excinfo:
@@ -381,7 +392,15 @@ def test_ignore_case_regex(page: Page, server: Server, method: str) -> None:
         )
     assert expected_error_msg in str(excinfo.value)
 
-    # not variant
+
+@pytest.mark.parametrize(
+    "method",
+    ["to_have_text", "to_contain_text"],
+)
+def test_ignore_case_regex_not_variant(page: Page, server: Server, method: str) -> None:
+    page.goto(server.EMPTY_PAGE)
+    page.set_content("<div id=target>apple BANANA</div><div>orange</div>")
+    expected_error_msg = method.replace("_", " ")
     getattr(expect(page.locator("div#target")), f"not_{method}")(
         re.compile("apple banana")
     )
@@ -789,62 +808,61 @@ def test_assertions_response_is_ok_fail(page: Page, server: Server) -> None:
     assert "← 404 Not Found" in error_message
 
 
-def test_should_print_response_with_text_content_type_if_to_be_ok_fails(
-    page: Page, server: Server
-) -> None:
-    server.set_route(
-        "/text-content-type",
-        lambda r: (
-            r.setResponseCode(404),
-            r.setHeader("content-type", "text/plain"),
-            r.write(b"Text error"),
-            r.finish(),
+@pytest.mark.parametrize(
+    "path, handler, body, shows_text",
+    [
+        (
+            "/text-content-type",
+            lambda r: (
+                r.setResponseCode(404),
+                r.setHeader("content-type", "text/plain"),
+                r.write(b"Text error"),
+                r.finish(),
+            ),
+            "Text error",
+            True,
         ),
-    )
-    server.set_route(
-        "/no-content-type",
-        lambda r: (
-            r.setResponseCode(404),
-            r.write(b"No content type error"),
-            r.finish(),
+        (
+            "/no-content-type",
+            lambda r: (
+                r.setResponseCode(404),
+                r.write(b"No content type error"),
+                r.finish(),
+            ),
+            "No content type error",
+            False,
         ),
-    )
-    server.set_route(
-        "/binary-content-type",
-        lambda r: (
-            r.setResponseCode(404),
-            r.setHeader("content-type", "image/bmp"),
-            r.write(b"Image content type error"),
-            r.finish(),
+        (
+            "/binary-content-type",
+            lambda r: (
+                r.setResponseCode(404),
+                r.setHeader("content-type", "image/bmp"),
+                r.write(b"Image content type error"),
+                r.finish(),
+            ),
+            "Image content type error",
+            False,
         ),
-    )
-
-    response = page.request.get(server.PREFIX + "/text-content-type")
-    with pytest.raises(AssertionError) as excinfo:
-        expect(response).to_be_ok()
-    error_message = str(excinfo.value)
-    assert ("→ GET " + server.PREFIX + "/text-content-type") in error_message
-    assert "← 404 Not Found" in error_message
-    assert "Response Text:" in error_message
-    assert "Text error" in error_message
-
-    response = page.request.get(server.PREFIX + "/no-content-type")
-    with pytest.raises(AssertionError) as excinfo:
-        expect(response).to_be_ok()
-    error_message = str(excinfo.value)
-    assert ("→ GET " + server.PREFIX + "/no-content-type") in error_message
-    assert "← 404 Not Found" in error_message
-    assert "Response Text:" not in error_message
-    assert "No content type error" not in error_message
+    ],
+)
+def test_should_print_response_with_text_content_type_if_to_be_ok_fails(
+    page: Page,
+    server: Server,
+    path: str,
+    handler: Callable[[Any], Any],
+    body: str,
+    shows_text: bool,
+) -> None:
+    server.set_route(path, handler)
 
-    response = page.request.get(server.PREFIX + "/binary-content-type")
+    response = page.request.get(server.PREFIX + path)
     with pytest.raises(AssertionError) as excinfo:
         expect(response).to_be_ok()
     error_message = str(excinfo.value)
-    assert ("→ GET " + server.PREFIX + "/binary-content-type") in error_message
+    assert ("→ GET " + server.PREFIX + path) in error_message
     assert "← 404 Not Found" in error_message
-    assert "Response Text:" not in error_message
-    assert "Image content type error" not in error_message
+    assert ("Response Text:" in error_message) == shows_text
+    assert (body in error_message) == shows_text
 
 
 def test_should_print_users_message_for_page_based_assertion(
@@ -972,16 +990,21 @@ def test_should_be_able_to_set_custom_timeout(page: Page) -> None:
     )
 
 
-def test_should_be_able_to_set_custom_global_timeout(page: Page) -> None:
-    try:
-        expect.set_options(timeout=111)
-        with pytest.raises(AssertionError) as exc_info:
-            expect(page.locator("#a1")).to_be_visible()
-        assert 'Expect "to_be_visible" locator("#a1") with timeout 111ms' in str(
-            exc_info.value
-        )
-    finally:
-        expect.set_options(timeout=5_000)
+@pytest.fixture
+def custom_global_timeout() -> Generator[None, None, None]:
+    expect.set_options(timeout=111)
+    yield
+    expect.set_options(timeout=5_000)
+
+
+def test_should_be_able_to_set_custom_global_timeout(
+    page: Page, custom_global_timeout: None
+) -> None:
+    with pytest.raises(AssertionError) as exc_info:
+        expect(page.locator("#a1")).to_be_visible()
+    assert 'Expect "to_be_visible" locator("#a1") with timeout 111ms' in str(
+        exc_info.value
+    )
 
 
 def test_to_have_accessible_name(page: Page) -> None:

tests/sync/test_browsercontext_request_fallback.py

@@ -117,8 +117,7 @@ def test_should_not_chain_abort(
     page: Page,
     context: BrowserContext,
     server: Server,
-    is_webkit: bool,
-    is_firefox: bool,
+    browser_name: str,
 ) -> None:
     failed = [False]
 
@@ -131,12 +130,11 @@ def handler(route: Route) -> None:
 
     with pytest.raises(Error) as excinfo:
         page.goto(server.EMPTY_PAGE)
-    if is_webkit:
-        assert "Blocked by Web Inspector" in excinfo.value.message
-    elif is_firefox:
-        assert "NS_ERROR_FAILURE" in excinfo.value.message
-    else:
-        assert "net::ERR_FAILED" in excinfo.value.message
+    expected_message = {
+        "webkit": "Blocked by Web Inspector",
+        "firefox": "NS_ERROR_FAILURE",
+    }.get(browser_name, "net::ERR_FAILED")
+    assert expected_message in excinfo.value.message
     assert not failed[0]
 
 
@@ -180,6 +178,25 @@ def handler_with_header_mods(route: Route) -> None:
     assert values == ["bar", "bar", "bar"]
 
 
+FETCH_WITH_FOO_AND_BAR_HEADERS_JS = """
+    async url => {
+        const data = await fetch(url, {
+            headers: {
+            foo: 'a',
+            bar: 'b',
+            }
+        });
+        return data.text();
+        }
+    """
+
+
+def delete_foo_header(route: Route, request: Request) -> None:
+    headers = request.all_headers()
+    del headers["foo"]
+    route.fallback(headers=headers)
+
+
 def test_should_delete_header_with_undefined_value(
     page: Page, context: BrowserContext, server: Server
 ) -> None:
@@ -200,27 +217,10 @@ def capture_and_continue(route: Route, request: Request) -> None:
         route.continue_()
 
     context.route("**/*", capture_and_continue)
-
-    def delete_foo_header(route: Route, request: Request) -> None:
-        headers = request.all_headers()
-        del headers["foo"]
-        route.fallback(headers=headers)
-
     context.route(server.PREFIX + "/something", delete_foo_header)
     with server.expect_request("/something") as server_req_info:
         text = page.evaluate(
-            """
-            async url => {
-                const data = await fetch(url, {
-                    headers: {
-                    foo: 'a',
-                    bar: 'b',
-                    }
-                });
-                return data.text();
-                }
-            """,
-            server.PREFIX + "/something",
+            FETCH_WITH_FOO_AND_BAR_HEADERS_JS, server.PREFIX + "/something"
         )
     server_req = server_req_info.value
     assert text == "done"

tests/sync/test_browsercontext_storage_state.py

@@ -14,6 +14,7 @@
 
 import json
 from pathlib import Path
+from typing import Generator
 
 import pytest
 
@@ -43,7 +44,8 @@ def test_should_capture_local_storage(context: BrowserContext) -> None:
     }
 
 
-def test_should_set_local_storage(browser: Browser) -> None:
+@pytest.fixture
+def page_with_storage_state(browser: Browser) -> Generator[Page, None, None]:
     storage_state: StorageState = {
         "origins": [
             {
@@ -72,10 +74,17 @@ def test_should_set_local_storage(browser: Browser) -> None:
     page = context.new_page()
     page.route("**/*", lambda route: route.fulfill(body="<html></html>"))
     page.goto("https://www.example.com")
-    local_storage = page.evaluate("window.localStorage")
+    yield page
+    context.close()
+
+
+def test_should_set_local_storage(page_with_storage_state: Page) -> None:
+    local_storage = page_with_storage_state.evaluate("window.localStorage")
     assert local_storage == {"name1": "value1"}
 
-    indexed_db = page.evaluate(
+
+def test_should_set_indexed_db(page_with_storage_state: Page) -> None:
+    indexed_db = page_with_storage_state.evaluate(
         """async () => {
         return new Promise((resolve, reject) => {
             const openRequest = indexedDB.open('db', 42);
@@ -91,7 +100,6 @@ def test_should_set_local_storage(browser: Browser) -> None:
     }"""
     )
     assert indexed_db == "foo"
-    context.close()
 
 
 def test_should_round_trip_through_the_file(

tests/sync/test_browsertype_connect.py

@@ -247,5 +247,4 @@ def test_set_input_files_should_preserve_last_modified_timestamp(
 
     # On Linux browser sometimes reduces the timestamp by 1ms: 1696272058110.0715  -> 1696272058109 or even
     # rounds it to seconds in WebKit: 1696272058110 -> 1696272058000.
-    for i in range(len(timestamps)):
-        assert abs(timestamps[i] - expected_timestamps[i]) < 1000
+    assert timestamps == pytest.approx(expected_timestamps, abs=1000)

tests/sync/test_console.py

@@ -32,12 +32,11 @@ def test_console_should_work(page: Page, browser_name: str) -> None:
         page.evaluate('() => console.log("hello", 5, {foo: "bar"})')
     assert len(messages) == 1
     message = messages[0]
-    if browser_name != "firefox":
-        assert message.text == "hello 5 {foo: bar}"
-        assert str(message) == "hello 5 {foo: bar}"
-    else:
-        assert message.text == "hello 5 JSHandle@object"
-        assert str(message) == "hello 5 JSHandle@object"
+    expected_text = {"firefox": "hello 5 JSHandle@object"}.get(
+        browser_name, "hello 5 {foo: bar}"
+    )
+    assert message.text == expected_text
+    assert str(message) == expected_text
     assert message.type == "log"
     assert message.args[0].json_value() == "hello"
     assert message.args[1].json_value() == 5
@@ -102,10 +101,8 @@ def test_console_should_not_fail_for_window_object(
     with page.expect_console_message():
         page.evaluate("() => console.error(window)")
     assert len(messages) == 1
-    if browser_name != "firefox":
-        assert messages[0].text == "Window"
-    else:
-        assert messages[0].text == "JSHandle@object"
+    expected_text = {"firefox": "JSHandle@object"}.get(browser_name, "Window")
+    assert messages[0].text == expected_text
 
 
 # Upstream issue https://bugs.webkit.org/show_bug.cgi?id=229515