Test the search URLs, and delete the harness that hid them

The Playwright tests drove a real browser through a wrapper that replaced
`page.goto` with a stand-in accepting the URL and discarding it. So every
scraper's navigation target was untested, and invisibly so: mutation
testing showed that replacing the Bing URL with a constant, dropping
`&num=` from the Scholar URL, and making the Google Patents URL ignore the
query all left the full suite green. A scraper searching for the wrong
thing entirely passed.

Finish the fetch/parse split that scrap.py and _extract_bing_results had
already started:

- Each scraper's URL construction becomes a named pure builder
(bing_search_url and friends), asserted directly in tests/test_serp_urls.py
- query present, query encoded, result count honoured, expected host.
22 tests, no browser, 0.11s. All four URL mutants now fail the suite.
- The extraction tests take an already-loaded page via a `page_factory`
fixture using `set_content`, exactly as the bing test already did. This
removes the need for the goto-patching Browser and Context wrappers, so
they are deleted (~45 lines of test infrastructure).
- Result-limit coverage added for all four extractors, and the shared
resource-blocking route handler is now tested - blocking the main
document instead of just stylesheets and images would break every
Playwright scrape.

Also cover the /serp/search aggregation, which had no endpoint test:
merging across queries, partial failure reported alongside results,
all-fail, and per-query fallthrough. Discarding every per-query error now
fails the suite.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MSNSYnceqvdVz7Csis4K9e

serp.py

@@ -124,13 +124,21 @@ async def _extract_google_scholar_results(page: Page, n_results: int) -> list[di
     return results
 
 
+def google_scholar_url(q: str, n_results: int) -> str:
+    """The Google Scholar results URL for a query.
+
+    Split out from the scraper so it can be asserted on directly: the
+    Playwright tests replace `page.goto` and never see the URL.
+    """
+    return f"https://scholar.google.com/scholar?q={quote_plus(q)}&num={n_results}"
+
+
 async def query_google_scholar(browser: Browser, q: str, n_results: int = 10):
     """Queries google scholar for the specified query and number of results. Returns relevant papers"""
     async with playwright_open_page(browser) as page:
         await page.route("**/*", _block_stylesheet_and_image_resources)
 
-        url = f"https://scholar.google.com/scholar?q={quote_plus(q)}&num={n_results}"
-        await page.goto(url)
+        await page.goto(google_scholar_url(q, n_results))
 
         return await _extract_google_scholar_results(page, n_results)
 
@@ -180,13 +188,17 @@ async def _extract_google_patents_results(page: Page, n_results: int) -> list[di
     return results[:n_results]
 
 
+def google_patents_search_url(q: str, n_results: int) -> str:
+    """The Google Patents search URL for a query."""
+    return f"https://patents.google.com/?q={quote_plus(q)}&num={n_results}"
+
+
 async def query_google_patents(browser: Browser, q: str, n_results: int = 10):
     """Queries google patents for the specified query and number of results. Returns relevant patents"""
     async with playwright_open_page(browser) as page:
         await page.route("**/*", _block_stylesheet_and_image_resources)
 
-        url = f"https://patents.google.com/?q={quote_plus(q)}&num={n_results}"
-        await page.goto(url)
+        await page.goto(google_patents_search_url(q, n_results))
 
         return await _extract_google_patents_results(page, n_results)
 
@@ -228,13 +240,22 @@ async def _extract_brave_results(page: Page, n_results: int) -> list[dict]:
     return results
 
 
+def brave_search_url(q: str, n_results: int = 10) -> str:
+    """The Brave Search results URL for a query.
+
+    Brave paginates rather than taking a result count, so `n_results` is
+    applied while extracting rather than in the URL; it is accepted here to
+    keep the builders interchangeable.
+    """
+    return f"https://search.brave.com/search?q={quote_plus(q)}"
+
+
 async def query_brave_search(browser: Browser, q: str, n_results: int = 10):
     """Queries Brave Search for the specified query."""
     async with playwright_open_page(browser) as page:
         await page.route("**/*", _block_stylesheet_and_image_resources)
 
-        url = f"https://search.brave.com/search?q={quote_plus(q)}"
-        await page.goto(url)
+        await page.goto(brave_search_url(q, n_results))
 
         return await _extract_brave_results(page, n_results)
 
@@ -277,13 +298,22 @@ async def _extract_bing_results(page: Page, n_results: int) -> list[dict]:
     return results
 
 
+def bing_search_url(q: str, n_results: int = 10) -> str:
+    """The Bing results URL for a query.
+
+    Bing paginates rather than taking a result count, so `n_results` is
+    applied while extracting rather than in the URL; it is accepted here to
+    keep the builders interchangeable.
+    """
+    return f"https://www.bing.com/search?q={quote_plus(q)}"
+
+
 async def query_bing_search(browser: Browser, q: str, n_results: int = 10):
     """Queries bing search for the specified query"""
     async with playwright_open_page(browser) as page:
         await page.route("**/*", _block_stylesheet_and_image_resources)
 
-        url = f"https://www.bing.com/search?q={quote_plus(q)}"
-        await page.goto(url)
+        await page.goto(bing_search_url(q, n_results))
 
         return await _extract_bing_results(page, n_results)
 

tests/conftest.py

@@ -59,3 +59,29 @@ async def browser():
     yield b
     await b.close()
     await pw.stop()
+
+
+@pytest_asyncio.fixture
+async def page_factory(browser):
+    """Returns a callable producing a real Page with fixture HTML loaded.
+
+    This is the whole seam the Playwright-driven tests need now that every
+    scraper is split into a URL builder, a navigation step and an
+    `_extract_*` function: the extractors take an already-loaded page, so
+    `set_content` reaches them directly. It replaces the goto-patching
+    Browser wrapper that used to be required, which had to accept the
+    navigation URL and throw it away - and so hid every URL bug.
+    """
+    contexts = []
+
+    async def make(html: str):
+        context = await browser.new_context()
+        contexts.append(context)
+        page = await context.new_page()
+        await page.set_content(html)
+        return page
+
+    yield make
+
+    for context in contexts:
+        await context.close()

tests/helpers.py

@@ -1,7 +1,4 @@
-"""Shared test helpers: fixture loading and a fake-navigation Playwright
-Browser wrapper used to characterize the scraper functions in `serp.py`
-without hitting live search engines.
-"""
+"""Shared test helpers: fixture loading."""
 
 from pathlib import Path
 
@@ -11,63 +8,3 @@ FIXTURES_DIR = Path(__file__).resolve().parent / "fixtures"
 def load_fixture(*parts: str) -> str:
     """Read a text fixture from tests/fixtures/<parts...>."""
     return FIXTURES_DIR.joinpath(*parts).read_text(encoding="utf-8")
-
-
-class _GotoPatchingContext:
-    """Wraps a real Playwright BrowserContext so every Page it creates loads
-    fixture HTML via `set_content()` instead of navigating over the network.
-    """
-
-    def __init__(self, real_context, html: str):
-        self._real_context = real_context
-        self._html = html
-
-    async def new_page(self):
-        page = await self._real_context.new_page()
-        html = self._html
-
-        async def fake_goto(url, **kwargs):
-            # The scraper functions don't use the response object `goto`
-            # normally returns, so a plain None is a safe stand-in.
-            await page.set_content(html)
-            return None
-
-        page.goto = fake_goto
-        return page
-
-    async def close(self):
-        await self._real_context.close()
-
-
-class _GotoPatchingBrowser:
-    """Wraps a real Playwright Browser the same way, so it can be passed
-    directly to `serp.py` functions in place of the real `pw_browser`.
-    """
-
-    def __init__(self, real_browser, html: str):
-        self._real_browser = real_browser
-        self._html = html
-
-    async def new_context(self, **kwargs):
-        real_context = await self._real_browser.new_context(**kwargs)
-        return _GotoPatchingContext(real_context, self._html)
-
-
-def make_fixture_browser(real_browser, html: str) -> _GotoPatchingBrowser:
-    """Build a Browser-like object for characterizing `serp.py`'s scraper
-    functions against fixture HTML.
-
-    Why this exists: each scraper (e.g. `query_bing_search`) registers its
-    own catch-all `page.route("**/*", ...)` resource blocker *inside* the
-    function, and that handler calls `route.continue_()` (not
-    `route.fallback()`) for the main document request. Playwright invokes
-    the most-recently-registered matching route first, so any route we
-    register on the page *before* calling the function never runs -
-    verified empirically, it's not a documented guarantee either way.
-    Patching `page.goto` itself, on a page created through this wrapper, is
-    the only reliable seam into these functions without touching their
-    source (see the code review this test suite followed from: the
-    fetch/navigate and parse logic aren't separated, which is exactly what
-    makes this workaround necessary).
-    """
-    return _GotoPatchingBrowser(real_browser, html)

tests/test_app.py

@@ -342,3 +342,80 @@ async def test_search_one_skips_a_backend_whose_circuit_is_open(monkeypatch):
     q, results, err = await app_module._search_one("second query", 5)
     assert ddg_calls == ["first query"]  # unchanged - DDG was skipped
     assert results == [{"title": "from brave"}]
+
+
+# ------------------------------- /serp/search endpoint -------------------------------
+# `_search_one` is covered above; this is the aggregation wrapped around it,
+# which had no coverage - dropping every per-query error left the suite green.
+
+
+async def test_search_endpoint_merges_results_from_every_query(client, monkeypatch):
+    async def fake_ddg(q, n):
+        return [{"title": f"result for {q}"}]
+
+    monkeypatch.setattr(app_module, "query_ddg_search", fake_ddg)
+
+    resp = await client.post("/serp/search", json={"queries": ["a", "b"], "n_results": 5})
+
+    data = resp.json()
+    assert {r["title"] for r in data["results"]} == {"result for a", "result for b"}
+    assert data["error"] is None
+
+
+async def test_search_endpoint_reports_partial_failure_alongside_results(client, monkeypatch):
+    """A query that no backend could answer must be reported even when
+    another query succeeded - otherwise the caller silently receives fewer
+    results than they asked for with no indication why.
+    """
+    async def selective_ddg(q, n):
+        if q == "bad":
+            raise RuntimeError("blocked")
+        return [{"title": f"result for {q}"}]
+
+    async def failing(*args):
+        raise RuntimeError("blocked")
+
+    monkeypatch.setattr(app_module, "query_ddg_search", selective_ddg)
+    monkeypatch.setattr(app_module, "query_brave_search", failing)
+    monkeypatch.setattr(app_module, "query_bing_search", failing)
+
+    resp = await client.post("/serp/search", json={"queries": ["good", "bad"], "n_results": 5})
+
+    data = resp.json()
+    assert [r["title"] for r in data["results"]] == ["result for good"]
+    assert "bad" in data["error"]
+
+
+async def test_search_endpoint_reports_an_error_when_everything_fails(client, monkeypatch):
+    async def failing(*args):
+        raise RuntimeError("blocked")
+
+    monkeypatch.setattr(app_module, "query_ddg_search", failing)
+    monkeypatch.setattr(app_module, "query_brave_search", failing)
+    monkeypatch.setattr(app_module, "query_bing_search", failing)
+
+    resp = await client.post("/serp/search", json={"queries": ["a", "b"], "n_results": 5})
+
+    data = resp.json()
+    assert data["results"] == []
+    # Both failed queries are named, not just the last one.
+    assert "'a'" in data["error"] and "'b'" in data["error"]
+
+
+async def test_search_endpoint_falls_through_backends_per_query(client, monkeypatch):
+    """The fallback chain is per query, so one blocked backend shouldn't
+    cost results for queries the next backend can answer."""
+    async def failing_ddg(q, n):
+        raise RuntimeError("ddg blocked")
+
+    async def fake_brave(browser, q, n):
+        return [{"title": f"brave has {q}"}]
+
+    monkeypatch.setattr(app_module, "query_ddg_search", failing_ddg)
+    monkeypatch.setattr(app_module, "query_brave_search", fake_brave)
+
+    resp = await client.post("/serp/search", json={"queries": ["a"], "n_results": 5})
+
+    data = resp.json()
+    assert data["results"] == [{"title": "brave has a"}]
+    assert data["error"] is None

tests/test_serp_playwright.py

@@ -1,36 +1,36 @@
-"""Characterization tests for the Playwright-driven scrapers in serp.py.
-
-These run against a real headless Chromium (same engine production uses) so
-that DOM/CSS-selector behavior is genuinely exercised, but the target page is
-served locally from a fixture instead of navigating to the live site - see
-`helpers.make_fixture_browser` for how, and why a simpler
-`page.route()`-based approach doesn't work here.
+"""Tests for the DOM-extraction half of the Playwright-driven scrapers.
+
+Each scraper in serp.py is three separable pieces: a URL builder (pure,
+tested in test_serp_urls.py), a navigation step, and an `_extract_*`
+function that reads an already-loaded page. These test the third piece
+against a real headless Chromium - the same engine production uses, so
+selector behaviour is genuinely exercised - with the fixture HTML loaded
+via `set_content`.
+
+This used to require a wrapper that replaced `page.goto` on a
+purpose-built fake Browser, because fetch and parse weren't separated.
+That wrapper accepted the navigation URL and discarded it, which is
+precisely why URL bugs were invisible to the suite; splitting the scrapers
+made it unnecessary and it has been deleted.
 """
 
 import pytest
 
-from helpers import load_fixture, make_fixture_browser
+from helpers import load_fixture
 from serp import (
     BraveSearchBlockedException,
     BrowserUnavailableError,
+    _block_stylesheet_and_image_resources,
     _extract_bing_results,
+    _extract_brave_results,
+    _extract_google_patents_results,
+    _extract_google_scholar_results,
     playwright_open_page,
-    query_bing_search,
-    query_brave_search,
-    query_google_patents,
-    query_google_scholar,
 )
 
 
-async def test_extract_bing_results_works_on_an_already_loaded_page(browser):
-    """Demonstrates the payoff of splitting fetch (navigate) from parse
-    (extract): `_extract_bing_results` takes a Page that's already loaded,
-    so this needs nothing beyond `set_content` - no goto-patching wrapper,
-    no browser-as-a-parameter indirection.
-    """
-    context = await browser.new_context()
-    page = await context.new_page()
-    await page.set_content(load_fixture("bing_results.html"))
+async def test_bing_results_are_extracted(page_factory):
+    page = await page_factory(load_fixture("bing_results.html"))
 
     results = await _extract_bing_results(page, 10)
 
@@ -39,24 +39,19 @@ async def test_extract_bing_results_works_on_an_already_loaded_page(browser):
         {"title": "Result B", "href": "https://example.org/b", "body": "Snippet B"},
     ]
 
-    await context.close()
 
+async def test_bing_extraction_honours_the_result_limit(page_factory):
+    page = await page_factory(load_fixture("bing_results.html"))
 
-async def test_bing_search_parses_results(browser):
-    fixture_browser = make_fixture_browser(browser, load_fixture("bing_results.html"))
+    results = await _extract_bing_results(page, 1)
 
-    results = await query_bing_search(fixture_browser, "test query", 10)
-
-    assert results == [
-        {"title": "Result A", "href": "https://example.org/a", "body": "Snippet A"},
-        {"title": "Result B", "href": "https://example.org/b", "body": "Snippet B"},
-    ]
+    assert [r["title"] for r in results] == ["Result A"]
 
 
-async def test_brave_search_parses_results(browser):
-    fixture_browser = make_fixture_browser(browser, load_fixture("brave_results.html"))
+async def test_brave_results_are_extracted(page_factory):
+    page = await page_factory(load_fixture("brave_results.html"))
 
-    results = await query_brave_search(fixture_browser, "test query", 10)
+    results = await _extract_brave_results(page, 10)
 
     assert results == [
         {"title": "Brave Result A", "body": "Brave snippet A", "href": "https://example.org/brave-a"},
@@ -64,17 +59,30 @@ async def test_brave_search_parses_results(browser):
     ]
 
 
-async def test_brave_search_detects_the_suspicious_block_page(browser):
-    fixture_browser = make_fixture_browser(browser, load_fixture("brave_blocked.html"))
+async def test_brave_extraction_honours_the_result_limit(page_factory):
+    page = await page_factory(load_fixture("brave_results.html"))
+
+    results = await _extract_brave_results(page, 1)
+
+    assert [r["title"] for r in results] == ["Brave Result A"]
+
+
+async def test_brave_block_page_raises(page_factory):
+    """Brave's anti-bot interstitial has no result cards, so it would
+    otherwise look like a legitimate zero-result search - and the
+    `/serp/search` fallback chain needs an exception to move to the next
+    backend.
+    """
+    page = await page_factory(load_fixture("brave_blocked.html"))
 
     with pytest.raises(BraveSearchBlockedException):
-        await query_brave_search(fixture_browser, "test query", 10)
+        await _extract_brave_results(page, 10)
 
 
-async def test_google_scholar_parses_results(browser):
-    fixture_browser = make_fixture_browser(browser, load_fixture("scholar_results.html"))
+async def test_google_scholar_results_are_extracted(page_factory):
+    page = await page_factory(load_fixture("scholar_results.html"))
 
-    results = await query_google_scholar(fixture_browser, "test query", 10)
+    results = await _extract_google_scholar_results(page, 10)
 
     assert results == [
         {"title": "Scholar Paper A", "body": "Scholar snippet A", "href": "https://example.org/scholar-a"},
@@ -82,10 +90,18 @@ async def test_google_scholar_parses_results(browser):
     ]
 
 
-async def test_google_patents_parses_results(browser):
-    fixture_browser = make_fixture_browser(browser, load_fixture("patents", "search_results.html"))
+async def test_google_scholar_extraction_honours_the_result_limit(page_factory):
+    page = await page_factory(load_fixture("scholar_results.html"))
+
+    results = await _extract_google_scholar_results(page, 1)
+
+    assert [r["title"] for r in results] == ["Scholar Paper A"]
 
-    results = await query_google_patents(fixture_browser, "test query", 10)
+
+async def test_google_patents_results_are_extracted(page_factory):
+    page = await page_factory(load_fixture("patents", "search_results.html"))
+
+    results = await _extract_google_patents_results(page, 10)
 
     assert results == [
         {
@@ -103,7 +119,57 @@ async def test_google_patents_parses_results(browser):
     ]
 
 
+async def test_google_patents_extraction_honours_the_result_limit(page_factory):
+    page = await page_factory(load_fixture("patents", "search_results.html"))
+
+    results = await _extract_google_patents_results(page, 1)
+
+    assert [r["id"] for r in results] == ["US11930446B2"]
+
+
+# --------------------------------- shared plumbing ---------------------------------
+
+
 async def test_playwright_open_page_raises_when_browser_unavailable():
     with pytest.raises(BrowserUnavailableError):
         async with playwright_open_page(None):
             pass
+
+
+class _FakeRoute:
+    def __init__(self):
+        self.aborted = False
+        self.continued = False
+
+    async def abort(self):
+        self.aborted = True
+
+    async def continue_(self):
+        self.continued = True
+
+
+class _FakeRequest:
+    def __init__(self, resource_type):
+        self.resource_type = resource_type
+
+
+@pytest.mark.parametrize("resource_type", ["stylesheet", "image"])
+async def test_decorative_resources_are_blocked(resource_type):
+    """Only page text matters to these scrapers, and skipping stylesheets
+    and images meaningfully speeds up navigation."""
+    route = _FakeRoute()
+
+    await _block_stylesheet_and_image_resources(route, _FakeRequest(resource_type))
+
+    assert route.aborted and not route.continued
+
+
+@pytest.mark.parametrize("resource_type", ["document", "script", "xhr", "fetch"])
+async def test_everything_else_is_allowed_through(resource_type):
+    """Blocking the main document (or the XHR that renders results) would
+    break the scrape entirely."""
+    route = _FakeRoute()
+
+    await _block_stylesheet_and_image_resources(route, _FakeRequest(resource_type))
+
+    assert route.continued and not route.aborted

tests/test_serp_urls.py

@@ -0,0 +1,78 @@
+"""The search URLs each scraper navigates to.
+
+These had no coverage at all, and the gap was invisible: the Playwright
+tests drive a real browser but patch `page.goto` with a stand-in that
+accepts the URL and discards it, so everything downstream of navigation
+was verified and everything upstream was not. Mutation testing confirmed
+it - replacing the Bing URL with a constant, dropping `&num=` from the
+Scholar URL, and making the Google Patents URL ignore the query all left
+the full suite green. A scraper that searched for the wrong thing entirely
+passed.
+
+These are pure string assertions: no browser, no fixtures, microseconds.
+"""
+
+import pytest
+
+from serp import (
+    bing_search_url,
+    brave_search_url,
+    google_patents_search_url,
+    google_scholar_url,
+)
+
+
+class TestQueryIsIncluded:
+    """The single most important property: the URL must ask for what the
+    caller asked for."""
+
+    @pytest.mark.parametrize("builder", [
+        bing_search_url, brave_search_url, google_patents_search_url, google_scholar_url])
+    def test_the_query_reaches_the_url(self, builder):
+        assert "widgets" in builder("widgets", 10)
+
+    @pytest.mark.parametrize("builder", [
+        bing_search_url, brave_search_url, google_patents_search_url, google_scholar_url])
+    def test_a_different_query_produces_a_different_url(self, builder):
+        assert builder("widgets", 10) != builder("gadgets", 10)
+
+
+class TestQueryIsEncoded:
+    """A raw query in a URL is both a correctness bug (spaces, ampersands
+    and hashes truncate or corrupt the search) and the kind of thing that
+    turns user input into URL structure."""
+
+    @pytest.mark.parametrize("builder", [
+        bing_search_url, brave_search_url, google_patents_search_url, google_scholar_url])
+    def test_spaces_are_encoded(self, builder):
+        url = builder("agentic ai", 10)
+        assert "agentic+ai" in url or "agentic%20ai" in url
+        assert "agentic ai" not in url
+
+    @pytest.mark.parametrize("builder", [
+        bing_search_url, brave_search_url, google_patents_search_url, google_scholar_url])
+    def test_url_significant_characters_are_escaped(self, builder):
+        url = builder("a&b=c#d", 10)
+        query_part = url.split("?", 1)[1]
+        # Exactly one separator per parameter the builder itself added.
+        assert "a&b=c#d" not in query_part
+        assert "%26" in query_part
+
+
+class TestResultCount:
+    def test_scholar_asks_for_the_requested_number_of_results(self):
+        assert "num=25" in google_scholar_url("widgets", 25)
+
+    def test_google_patents_asks_for_the_requested_number_of_results(self):
+        assert "num=25" in google_patents_search_url("widgets", 25)
+
+
+class TestHosts:
+    @pytest.mark.parametrize("builder, host", [
+        (bing_search_url, "https://www.bing.com/search"),
+        (brave_search_url, "https://search.brave.com/search"),
+        (google_patents_search_url, "https://patents.google.com/"),
+        (google_scholar_url, "https://scholar.google.com/scholar"),
+    ])
+    def test_points_at_the_expected_endpoint(self, builder, host):
+        assert builder("widgets", 10).startswith(host)