Refactor to BrowserClient. Unify tests with spies and fakes

ocrdbrowser/__init__.py

@@ -3,6 +3,7 @@
     Channel,
     ChannelClosed,
     OcrdBrowser,
+    OcrdBrowserClient,
     OcrdBrowserFactory,
     filter_owned,
     in_other_workspaces,
@@ -14,13 +15,16 @@
 from ._docker import DockerOcrdBrowserFactory
 from ._port import NoPortsAvailableError
 from ._subprocess import SubProcessOcrdBrowserFactory
+from ._client import HttpBrowserClient
 
 __all__ = [
     "Channel",
     "ChannelClosed",
     "DockerOcrdBrowserFactory",
+    "HttpBrowserClient",
     "NoPortsAvailableError",
     "OcrdBrowser",
+    "OcrdBrowserClient",
     "OcrdBrowserFactory",
     "SubProcessOcrdBrowserFactory",
     "filter_owned",

ocrdbrowser/_browser.py

@@ -5,18 +5,6 @@
 from typing import AsyncContextManager, Protocol
 
 
-class ChannelClosed(RuntimeError):
-    pass
-
-
-class Channel(Protocol):
-    async def receive_bytes(self) -> bytes:
-        ...
-
-    async def send_bytes(self, data: bytes) -> None:
-        ...
-
-
 class OcrdBrowser(Protocol):
     def address(self) -> str:
         ...
@@ -27,12 +15,32 @@ def owner(self) -> str:
     def workspace(self) -> str:
         ...
 
+    def client(self) -> OcrdBrowserClient:
+        ...
+
     async def start(self) -> None:
         ...
 
     async def stop(self) -> None:
         ...
 
+
+class ChannelClosed(RuntimeError):
+    ...
+
+
+class Channel(Protocol):
+    async def receive_bytes(self) -> bytes:
+        ...
+
+    async def send_bytes(self, data: bytes) -> None:
+        ...
+
+
+class OcrdBrowserClient(Protocol):
+    async def get(self, resource: str) -> bytes:
+        ...
+
     def open_channel(self) -> AsyncContextManager[Channel]:
         ...
 

ocrdbrowser/_websocketchannel.py → ocrdbrowser/_client.py

@@ -1,14 +1,15 @@
 from __future__ import annotations
 
 from types import TracebackType
-from typing import Type, cast
+from typing import AsyncContextManager, Type, cast
 
+import httpx
 from websockets import client
 from websockets.exceptions import ConnectionClosed
 from websockets.legacy.client import WebSocketClientProtocol
 from websockets.typing import Subprotocol
 
-from ._browser import ChannelClosed
+from ._browser import Channel, ChannelClosed
 
 
 class WebSocketChannel:
@@ -58,3 +59,16 @@ async def send_bytes(self, data: bytes) -> None:
             await self._open_connection.send(data)
         except ConnectionClosed:
             raise ChannelClosed()
+
+
+class HttpBrowserClient:
+    def __init__(self, address: str) -> None:
+        self.address = address
+
+    async def get(self, resource: str) -> bytes:
+        async with httpx.AsyncClient(base_url=self.address) as client:
+            response = await client.get(resource)
+            return response.content
+
+    def open_channel(self) -> AsyncContextManager[Channel]:
+        return WebSocketChannel(self.address + "/socket")

ocrdbrowser/_docker.py

@@ -3,11 +3,11 @@
 import asyncio
 import logging
 import os.path as path
-from typing import Any, AsyncContextManager
+from typing import Any
 
-from ._browser import Channel, OcrdBrowser
+from ._browser import OcrdBrowser, OcrdBrowserClient
 from ._port import Port
-from ._websocketchannel import WebSocketChannel
+from ._client import HttpBrowserClient
 
 _docker_run = "docker run --rm -d --name {} -v {}:/data -p {}:8085 ocrd-browser:latest"
 _docker_stop = "docker stop {}"
@@ -58,8 +58,8 @@ async def stop(self) -> None:
         self._port.release()
         self.id = None
 
-    def open_channel(self) -> AsyncContextManager[Channel]:
-        return WebSocketChannel(self.address() + "/socket")
+    def client(self) -> OcrdBrowserClient:
+        return HttpBrowserClient(self.address())
 
     def _container_name(self) -> str:
         workspace = path.basename(self.workspace())

ocrdbrowser/_subprocess.py

@@ -4,11 +4,11 @@
 import logging
 import os
 from shutil import which
-from typing import AsyncContextManager, Optional
+from typing import Optional
 
-from ._browser import Channel, OcrdBrowser
+from ._browser import OcrdBrowser, OcrdBrowserClient
 from ._port import Port
-from ._websocketchannel import WebSocketChannel
+from ._client import HttpBrowserClient
 
 BROADWAY_BASE_PORT = 8080
 
@@ -72,8 +72,8 @@ async def stop(self) -> None:
             finally:
                 self._localport.release()
 
-    def open_channel(self) -> AsyncContextManager[Channel]:
-        return WebSocketChannel(self.address() + "/socket")
+    def client(self) -> OcrdBrowserClient:
+        return HttpBrowserClient(self.address())
 
 
 class SubProcessOcrdBrowserFactory:

ocrdmonitor/server/proxy.py

@@ -1,7 +1,6 @@
 from __future__ import annotations
 
 import asyncio
-import httpx
 
 from fastapi import Response
 from ocrdbrowser import Channel
@@ -11,14 +10,8 @@
 
 async def forward(redirect: BrowserRedirect, url: str) -> Response:
     redirect_url = redirect.redirect_url(url)
-    async with httpx.AsyncClient() as client:
-        response = await client.get(redirect_url)
-
-    return Response(
-        content=response.content,
-        status_code=response.status_code,
-        headers=response.headers,
-    )
+    resource = await redirect.browser.client().get(redirect_url)
+    return Response(content=resource)
 
 
 async def tunnel(

ocrdmonitor/server/workspaces.py

@@ -3,7 +3,6 @@
 import uuid
 from pathlib import Path
 
-import httpx
 
 import ocrdbrowser
 import ocrdmonitor.server.proxy as proxy
@@ -33,15 +32,14 @@ def list_workspaces(request: Request) -> Response:
         )
 
     @router.get("/browse/{workspace:path}", name="workspaces.browse")
-    async def browser(request: Request, workspace: str) -> Response:
+    async def browser(request: Request, workspace: Path) -> Response:
         session_id = request.cookies.setdefault("session_id", str(uuid.uuid4()))
         response = Response()
         response.set_cookie("session_id", session_id)
 
-        if (session_id, Path(workspace)) not in redirects:
-            workspace_path = workspace_dir / workspace
-            browser = await launch_browser(session_id, workspace_path)
-            redirects.add(session_id, Path(workspace), browser)
+        if (session_id, workspace) not in redirects:
+            browser = await launch_browser(session_id, workspace)
+            redirects.add(session_id, workspace, browser)
 
         return response
 
@@ -54,28 +52,26 @@ def open_workspace(request: Request, workspace: str) -> Response:
 
     @router.get("/ping/{workspace:path}", name="workspaces.ping")
     async def ping_workspace(
-        request: Request, workspace: str, session_id: str = Cookie(default=None)
+        request: Request, workspace: Path, session_id: str = Cookie(default=None)
     ) -> Response:
-        workspace_path = Path(workspace)
-        redirect = redirects.get(session_id, workspace_path)
+        redirect = redirects.get(session_id, workspace)
         try:
-            await proxy.forward(redirect, str(workspace_path))
+            await proxy.forward(redirect, str(workspace))
             return Response(status_code=200)
-        except httpx.ConnectError:
+        except ConnectionError:
             return Response(status_code=502)
 
     # NOTE: It is important that the route path here ends with a slash, otherwise
     #       the reverse routing will not work as broadway.js uses window.location
     #       which points to the last component with a trailing slash.
     @router.get("/view/{workspace:path}/", name="workspaces.view")
     async def workspace_reverse_proxy(
-        request: Request, workspace: str, session_id: str = Cookie(default=None)
+        request: Request, workspace: Path, session_id: str = Cookie(default=None)
     ) -> Response:
-        workspace_path = Path(workspace)
-        redirect = redirects.get(session_id, workspace_path)
+        redirect = redirects.get(session_id, workspace)
         try:
-            return await proxy.forward(redirect, str(workspace_path))
-        except httpx.ConnectError:
+            return await proxy.forward(redirect, str(workspace))
+        except ConnectionError:
             return templates.TemplateResponse(
                 "view_workspace_failed.html.j2",
                 {"request": request, "workspace": workspace},
@@ -92,7 +88,7 @@ async def workspace_socket_proxy(
     async def communicate_with_browser_until_closed(
         websocket: WebSocket, browser: OcrdBrowser
     ) -> None:
-        async with browser.open_channel() as channel:
+        async with browser.client().open_channel() as channel:
             try:
                 while True:
                     await proxy.tunnel(channel, websocket)
@@ -102,7 +98,8 @@ async def communicate_with_browser_until_closed(
                 pass
 
     async def launch_browser(session_id: str, workspace: Path) -> OcrdBrowser:
-        return await ocrdbrowser.launch(str(workspace), session_id, factory)
+        full_workspace_path = workspace_dir / workspace
+        return await ocrdbrowser.launch(str(full_workspace_path), session_id, factory)
 
     async def stop_browser(browser: OcrdBrowser) -> None:
         await browser.stop()

pyproject.toml

@@ -43,6 +43,10 @@ markers = [
     "integration: mark test as integration test",
 ]
 
+[tool.pdm.scripts]
+test = "pytest tests -m 'not integration'"
+test-integration = "pytest tests"
+
 [build-system]
 requires = ["pdm-pep517>=1.0.0"]
 build-backend = "pdm.pep517.api"

tests/ocrdbrowser/test_launch.py

@@ -3,32 +3,34 @@
 import pytest
 
 import ocrdbrowser
-from tests.ocrdbrowser.browserdoubles import BrowserSpy, BrowserTestDoubleFactory
+from tests.testdoubles import BrowserSpy, IteratingBrowserTestDoubleFactory
 
 
 @pytest.mark.asyncio
 async def test__workspace__launch__spawns_new_ocrd_browser() -> None:
     owner = "the-owner"
     workspace = "path/to/workspace"
-    process = await ocrdbrowser.launch(workspace, owner, BrowserTestDoubleFactory())
+    process = await ocrdbrowser.launch(
+        workspace, owner, IteratingBrowserTestDoubleFactory()
+    )
 
     process = cast(BrowserSpy, process)
-    assert process.running is True
+    assert process.is_running is True
     assert process.owner() == owner
     assert process.workspace() == workspace
 
 
 @pytest.mark.asyncio
 async def test__workspace__launch_for_different_owners__both_processes_running() -> None:
-    factory = BrowserTestDoubleFactory()
+    factory = IteratingBrowserTestDoubleFactory()
 
     first_process = await ocrdbrowser.launch("first-path", "first-owner", factory)
     second_process = await ocrdbrowser.launch(
         "second-path", "second-owner", factory, {first_process}
     )
 
     processes = {first_process, second_process}
-    assert all(cast(BrowserSpy, process).running for process in processes)
+    assert all(cast(BrowserSpy, process).is_running for process in processes)
     assert {p.owner() for p in processes} == {"first-owner", "second-owner"}
     assert {p.workspace() for p in processes} == {"first-path", "second-path"}
 
@@ -39,7 +41,7 @@ async def test__workspace__launch_for_same_owner_and_workspace__does_not_start_n
 ):
     owner = "the-owner"
     workspace = "the-workspace"
-    factory = BrowserTestDoubleFactory()
+    factory = IteratingBrowserTestDoubleFactory()
 
     first_process = await ocrdbrowser.launch(workspace, owner, factory)
     second_process = await ocrdbrowser.launch(

tests/ocrdbrowser/test_websocketchannel.py

@@ -2,10 +2,9 @@
 from typing import Any, Coroutine, Protocol
 
 import pytest
-from ocrdbrowser import Channel, ChannelClosed
-from ocrdbrowser._websocketchannel import WebSocketChannel
 
-from tests.fakes import BackgroundProcess, broadway_fake
+from ocrdbrowser import Channel, ChannelClosed, HttpBrowserClient
+from tests.testdoubles import BackgroundProcess, broadway_fake
 
 
 async def send(channel: Channel) -> None:
@@ -22,15 +21,17 @@ def __call__(self, channel: Channel) -> Coroutine[Any, Any, Any]:
 
 
 @pytest.mark.asyncio
+@pytest.mark.integration
 @pytest.mark.parametrize("comm_function", [send, receive])
 async def test__channel__losing_connection_while_communicating__raises_channel_closed(
     comm_function: CommunicationFunction,
 ) -> None:
     server = broadway_fake("")
     await asyncio.to_thread(server.launch)
 
+    sut = HttpBrowserClient("http://localhost:7000")
     with pytest.raises(ChannelClosed):
-        async with WebSocketChannel("http://localhost:7000/socket") as channel:
+        async with sut.open_channel() as channel:
             await shutdown_server(server)
             await comm_function(channel)