diff --git a/tests/conftest.py b/tests/conftest.py index a2e6aba2..6b0608ee 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,6 +2,7 @@ import importlib.metadata import os import pathlib import re +import socket import subprocess import sys import tempfile @@ -32,17 +33,31 @@ UNDOCUMENTED_PERMISSIONS = { } -def wait_until_responds(url, timeout=5.0, client=httpx, **kwargs): +def wait_until_responds(url, timeout=5.0, client=httpx, process=None, **kwargs): start = time.time() while time.time() - start < timeout: + # If the server died there is no point waiting out the timeout - fail + # now, with its output, instead of after `timeout` seconds of silence + if process is not None and process.poll() is not None: + raise AssertionError( + "Server exited early with returncode {}\n{}".format( + process.returncode, process.stdout.read().decode("utf-8") + ) + ) try: client.get(url, **kwargs) return - except httpx.ConnectError: + except httpx.TransportError: time.sleep(0.1) raise AssertionError(f"Timed out waiting for {url} to respond") +def find_free_port(): + with socket.socket() as sock: + sock.bind(("127.0.0.1", 0)) + return sock.getsockname()[1] + + @pytest.fixture def bare_ds(): """ @@ -301,6 +316,67 @@ def ds_unix_domain_socket_server(tmp_path_factory): pass +@pytest.fixture +def serve_with_plugins(tmp_path): + """Factory fixture for starting ``datasette serve`` in a subprocess with + plugins written to a temporary ``--plugins-dir``. + + Unlike ``ds_localhost_http_server`` this is function-scoped and takes a + fresh port each time, because each test needs its own plugins. Call it as:: + + proc, port = serve_with_plugins({"my_plugin": PLUGIN_SOURCE}) + + ``plugins`` maps module name to Python source. Pass + ``wait_for_startup=False`` when the server is expected to fail during + startup rather than begin serving. Extra CLI arguments are passed through. + Every process started is terminated when the test ends. + """ + processes = [] + + def start(plugins, *extra_args, wait_for_startup=True): + plugins_dir = tmp_path / "plugins" + plugins_dir.mkdir(exist_ok=True) + for module_name, source in plugins.items(): + (plugins_dir / "{}.py".format(module_name)).write_text(source, "utf-8") + port = find_free_port() + proc = subprocess.Popen( + [ + sys.executable, + "-m", + "datasette", + "--memory", + "--plugins-dir", + str(plugins_dir), + "-h", + "127.0.0.1", + "-p", + str(port), + *extra_args, + ], + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + # Avoid FileNotFoundError: [Errno 2] No such file or directory: + cwd=tempfile.gettempdir(), + ) + processes.append(proc) + if wait_for_startup: + wait_until_responds( + "http://127.0.0.1:{}/-/versions.json".format(port), process=proc + ) + return proc, port + + yield start + + for proc in processes: + if proc.poll() is None: + proc.terminate() + try: + proc.wait(timeout=5) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait() + + # Import fixtures from fixtures.py to make them available from .fixtures import ( # noqa: F401 TEMP_PLUGIN_SECRET_FILE, diff --git a/tests/test_cli_serve_server.py b/tests/test_cli_serve_server.py index 19fc9198..5f74cd9b 100644 --- a/tests/test_cli_serve_server.py +++ b/tests/test_cli_serve_server.py @@ -1,7 +1,4 @@ import socket -import subprocess -import sys -import tempfile import time import httpx @@ -34,12 +31,6 @@ def test_serve_unix_domain_socket(ds_unix_domain_socket_server): }.items() <= response.json().items() -def _find_free_port(): - with socket.socket() as sock: - sock.bind(("127.0.0.1", 0)) - return sock.getsockname()[1] - - # Shaped after datasette-litestream's (sync) startup hook, which schedules a # background task with asyncio.get_running_loop().create_task(...): # https://github.com/simonw/datasette-litestream/blob/main/datasette_litestream/__init__.py @@ -47,7 +38,7 @@ def _find_free_port(): # same event loop that goes on to serve requests - if it runs on a throwaway # loop that gets closed straight after (as on unmodified main), the task is # scheduled but never gets a turn before the loop is torn down. -MARKER_TASK_PLUGIN = ''' +MARKER_TASK_PLUGIN = """ import asyncio from datasette import hookimpl from datasette.utils.asgi import Response @@ -55,6 +46,8 @@ from datasette.utils.asgi import Response @hookimpl def startup(datasette): + datasette._startup_calls = getattr(datasette, "_startup_calls", 0) + 1 + async def _mark(): # The await is essential to the regression: a task with no # internal await point can complete during the brief window @@ -74,14 +67,17 @@ def startup(datasette): def register_routes(): async def marker_status(datasette): return Response.json( - {"marker_task_ran": getattr(datasette, "_marker_task_ran", False)} + { + "marker_task_ran": getattr(datasette, "_marker_task_ran", False), + "startup_calls": getattr(datasette, "_startup_calls", 0), + } ) return [(r"^/-/marker-task-ran$", marker_status)] -''' +""" -STARTUP_ERROR_PLUGIN = ''' +STARTUP_ERROR_PLUGIN = """ from datasette import hookimpl from datasette.utils import StartupError @@ -89,11 +85,11 @@ from datasette.utils import StartupError @hookimpl def startup(datasette): raise StartupError("boom from plugin") -''' +""" @pytest.mark.serial -def test_startup_hook_background_task_runs_on_serving_loop(tmp_path): +def test_startup_hook_background_task_runs_on_serving_loop(serve_with_plugins): """ Litestream-shaped regression test: a startup hook that does asyncio.get_running_loop().create_task(...) must have that task @@ -103,109 +99,61 @@ def test_startup_hook_background_task_runs_on_serving_loop(tmp_path): invoke_startup() runs on a throwaway loop that is closed before uvicorn opens its own loop to serve. """ - plugins_dir = tmp_path / "plugins" - plugins_dir.mkdir() - (plugins_dir / "marker_task_plugin.py").write_text(MARKER_TASK_PLUGIN, "utf-8") - - port = _find_free_port() - ds_proc = subprocess.Popen( - [ - sys.executable, - "-m", - "datasette", - "--memory", - "--plugins-dir", - str(plugins_dir), - "-p", - str(port), - ], - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - cwd=tempfile.gettempdir(), + _, port = serve_with_plugins({"marker_task_plugin": MARKER_TASK_PLUGIN}) + # The fixture has already waited for the server to answer requests. The + # marker task deliberately awaits before setting its flag, so poll for a + # moment rather than assuming it landed before the first request arrived. + deadline = time.time() + 3.0 + payload = {} + while time.time() < deadline: + payload = httpx.get( + f"http://127.0.0.1:{port}/-/marker-task-ran", timeout=1.0 + ).json() + if payload["marker_task_ran"]: + break + time.sleep(0.05) + assert payload.get("marker_task_ran"), ( + "The startup hook's asyncio.create_task(...) never ran - " + "invoke_startup() and the server are not sharing an event loop" ) - try: - url = f"http://localhost:{port}/-/marker-task-ran" - deadline = time.time() + 15.0 - marker_task_ran = False - while time.time() < deadline: - if ds_proc.poll() is not None: - raise AssertionError( - "datasette serve exited early\n" - + ds_proc.stdout.read().decode("utf-8") - ) - try: - response = httpx.get(url, timeout=1.0) - except httpx.TransportError: - time.sleep(0.1) - continue - if response.status_code == 200 and response.json().get( - "marker_task_ran" - ): - marker_task_ran = True - break - time.sleep(0.1) - assert marker_task_ran, ( - "The startup hook's asyncio.create_task(...) never ran - " - "invoke_startup() and the server are not sharing an event loop" + # Polling above means this test would also pass if the startup hook were + # re-run on the serving loop by the first-request fallback - which would + # hide exactly the bug being tested. invoke_startup() is idempotent today + # so that cannot happen; assert it explicitly so that if the idempotency + # guard is ever removed this test fails loudly instead of silently + # becoming a no-op. + assert payload["startup_calls"] == 1, ( + "startup hook ran {} times - the marker may have been set by a " + "re-run on the serving loop rather than by the original task".format( + payload["startup_calls"] ) - finally: - ds_proc.terminate() - try: - ds_proc.wait(timeout=5) - except subprocess.TimeoutExpired: - ds_proc.kill() - ds_proc.wait() + ) @pytest.mark.serial -def test_startup_error_fails_fast_before_port_binds(tmp_path): +def test_startup_error_fails_fast_before_port_binds(serve_with_plugins): """ A "startup" plugin hook that raises StartupError must fail fast: print the message, exit non-zero, and never accept a connection on the port - the failure must happen before uvicorn.Server binds the socket. + + Note this is a characterization test, not a regression test: it also + passes on unmodified main, where startup already ran ahead of + uvicorn.run(). It earns its keep once startup moves into the ASGI + lifespan, where fail-fast is genuinely at risk. """ - plugins_dir = tmp_path / "plugins" - plugins_dir.mkdir() - (plugins_dir / "startup_error_plugin.py").write_text( - STARTUP_ERROR_PLUGIN, "utf-8" + proc, port = serve_with_plugins( + {"startup_error_plugin": STARTUP_ERROR_PLUGIN}, wait_for_startup=False ) + stdout, _ = proc.communicate(timeout=15) + output = stdout.decode("utf-8") + assert proc.returncode not in (0, None), output + assert "boom from plugin" in output, output - port = _find_free_port() - ds_proc = subprocess.Popen( - [ - sys.executable, - "-m", - "datasette", - "--memory", - "--plugins-dir", - str(plugins_dir), - "-p", - str(port), - ], - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - cwd=tempfile.gettempdir(), - ) - try: - deadline = time.time() + 15.0 - # While the process is still alive (it should crash almost - # immediately) repeatedly confirm nothing is listening on the port - while ds_proc.poll() is None and time.time() < deadline: - with pytest.raises(OSError): - with socket.create_connection(("127.0.0.1", port), timeout=0.2): - pass - time.sleep(0.05) - - stdout, _ = ds_proc.communicate(timeout=5) - output = stdout.decode("utf-8") - assert ds_proc.returncode not in (0, None), output - assert "boom from plugin" in output, output - - # And confirm it never accepted a connection even now it has exited - with pytest.raises(OSError): - with socket.create_connection(("127.0.0.1", port), timeout=0.2): - pass - finally: - if ds_proc.poll() is None: - ds_proc.kill() - ds_proc.wait() + # Nothing is listening on the port now the process has exited. This + # confirms the socket was not left bound; on its own it cannot prove the + # failure preceded the bind, since a port nothing ever touched also + # refuses connections. + with pytest.raises(OSError): + with socket.create_connection(("127.0.0.1", port), timeout=0.2): + pass