diff --git a/Makefile b/Makefile index 4b42da35..0e6c8865 100644 --- a/Makefile +++ b/Makefile @@ -11,6 +11,7 @@ help: @echo "servers-down: stop the docker VNC test servers" @echo "test-servers: run functional tests against the VNC test servers" @echo "test-os-server: run functional tests against this OS's VNC server" + @echo "test-api: run the in-process vncdotool.api lifecycle suite" @echo "screenshots: screenshot each running VNC test server into a gallery" @echo "docs: build documentation" @echo "release: tag and push current version to trigger PyPI release" diff --git a/docs/testing-framework-design.md b/docs/testing-framework-design.md index 17007be8..2ff29ff7 100644 --- a/docs/testing-framework-design.md +++ b/docs/testing-framework-design.md @@ -86,9 +86,12 @@ questions. **In-process API suite** (small, separate): the library API needs live coverage of its *lifecycle*, not of server compatibility — `api.connect`, error propagation, timeouts, `api.shutdown` cleanliness — against a single -known-good container. One reactor per process means this suite runs in its -own process invocation. It does not fan out across the fleet: server -compatibility is already proven by the subprocess grid. +known-good container. One reactor per process means exactly one module +(`test_api_lifecycle.py`) may ever touch `vncdotool.api` in-process; it is +safe inside the shared functional discover because every other module is +subprocess-only and never touches that reactor, regardless of run order — +`make test-api` also runs it alone. It does not fan out across the fleet: +server compatibility is already proven by the subprocess grid. ### 3. Capture kit (discovery for unhosted servers) diff --git a/tests/functional/test_api_lifecycle.py b/tests/functional/test_api_lifecycle.py new file mode 100644 index 00000000..1695f33c --- /dev/null +++ b/tests/functional/test_api_lifecycle.py @@ -0,0 +1,187 @@ +"""In-process coverage of ``vncdotool.api``'s *lifecycle* -- context-manager +use, reconnection, error propagation, timeouts, ``shutdown()`` cleanliness. +Server compatibility belongs to the subprocess grid in test_servers.py. + +``api.connect()`` starts the Twisted reactor in a daemon thread, and a +reactor cannot be restarted once stopped, so one process gets one reactor +for its whole lifetime: this is the only module allowed to call it. A +second in-process module would either race this one for it or leave its +own reactor thread wedged on exit -- new in-process tests belong in +*this* file, sharing *this* reactor. + +``setUpModule`` makes the first connection itself, so the reactor is +running before any test method does -- no test depends on being first. +""" + +import queue +import socket +import threading +import time +import unittest + +from twisted.internet import reactor +from twisted.internet.error import ConnectError +from twisted.internet.protocol import Factory, Protocol + +from vncdotool import api + +from .vncservers import DOCKER_SERVERS, HOST, SUBPROCESS_TIMEOUT_HEADROOM, connect, port_open + +LIBVNC = next(s for s in DOCKER_SERVERS if s.name == "libvncserver-example") + +# Same headroom the subprocess grid adds: the bare 5s server budget was +# observed to flake under load. +HAPPY_TIMEOUT = LIBVNC.timeout + SUBPROCESS_TIMEOUT_HEADROOM + +SHORT_TIMEOUT = 2.0 + +# join() budget for the closed-port helper thread: long enough to fail +# promptly, short enough that a real hang doesn't stall the suite. +HANG_GUARD_TIMEOUT = 10.0 + +# Bound for a call dispatched onto the reactor thread via callFromThread -- +# a local listenTCP/stopListening, not network I/O, so this is generous +# only relative to how fast that actually is. +REACTOR_CALL_TIMEOUT = 2.0 + + +def setUpModule() -> None: + if not port_open(HOST, LIBVNC.port): + raise RuntimeError( + f"libvncserver-example not reachable on {HOST}:{LIBVNC.port} -- " + "start the servers first with `make servers-up`" + ) + with connect(LIBVNC, timeout=HAPPY_TIMEOUT) as client: + client.refreshScreen() + + +def tearDownModule() -> None: + # Exactly once per process: without it the interpreter hangs on the + # non-daemon worker threads Twisted runs under the reactor. + api.shutdown() + + +class TestApiLifecycle(unittest.TestCase): + """Lifecycle cases against the libvncserver-example:5935 container. + + Independent of each other and of run order: the one case that could + wedge the shared reactor (the closed-port case) is bounded by its own + timeout and helper thread, not by being run last. + """ + + def test_context_manager_connect(self) -> None: + """The documented ``with api.connect(...) as client:`` pattern works.""" + with connect(LIBVNC, timeout=HAPPY_TIMEOUT) as client: + client.keyPress("x") + + def test_sequential_reconnects(self) -> None: + """``shutdown()`` is what's terminal, not ``disconnect()``: otherwise no + long-running application could reconnect after a single drop. + """ + for _ in range(2): + with connect(LIBVNC, timeout=HAPPY_TIMEOUT) as client: + client.refreshScreen() + + def test_timeout_raises_timeout_error(self) -> None: + """``api.connect()`` is fire-and-forget and never raises here; the + timeout comes from the first call that has to wait on the + never-completing handshake. + """ + listener = _SilentListener() + listener.start() + self.addCleanup(listener.stop) + + client = api.connect(f"{HOST}::{listener.port}") + client.timeout = SHORT_TIMEOUT + self.addCleanup(client.disconnect) + + start = time.monotonic() + with self.assertRaises(TimeoutError): + client.refreshScreen() + elapsed = time.monotonic() - start + + # Tight bound: the timeout path costs mild thread-wakeup slack at + # most. Padding this further would hide a real regression instead + # of catching one. + self.assertLess(elapsed, SHORT_TIMEOUT + 1.0) + + def test_closed_port_raises_promptly(self) -> None: + """Bounded twice over -- a per-client timeout, and a helper thread + joined with a deadline -- so even a wedge fails only this test, + not the suite. + + Twisted's ``ConnectionRefusedError`` is a ``ConnectError``, not the + stdlib ``OSError`` of the same name, so assert on that base class. + """ + closed_port = _closed_port() + + outcome: dict = {} + + def attempt() -> None: + try: + client = api.connect(f"{HOST}::{closed_port}") + client.timeout = SHORT_TIMEOUT + try: + client.refreshScreen() + outcome["result"] = "no exception" + except Exception as exc: # noqa: BLE001 - captured for the assertion below + outcome["exception"] = exc + finally: + client.disconnect() + except Exception as exc: # noqa: BLE001 - connect() itself is not expected to raise + outcome["exception"] = exc + + worker = threading.Thread(target=attempt, name="closed-port-attempt", daemon=True) + worker.start() + worker.join(timeout=HANG_GUARD_TIMEOUT) + + if worker.is_alive(): + self.fail( + f"connect()/refreshScreen() against a closed port did not " + f"return within {HANG_GUARD_TIMEOUT}s -- this would have " + "hung the process" + ) + + self.assertIn("exception", outcome, f"expected an exception, got: {outcome}") + # The family, not the exact class: any prompt, clear connection + # failure satisfies the lifecycle guarantee. + self.assertIsInstance(outcome["exception"], ConnectError) + + +class _SilentProtocol(Protocol): + """Accepts the connection and never sends or reads anything.""" + + +class _SilentListener: + """A TCP listener on api.connect()'s own reactor that accepts and then + never speaks, forcing a protocol-level timeout rather than a + connection-refused. This process gets exactly one reactor, so this + reuses it rather than starting a second one via raw sockets. + """ + + def start(self) -> None: + ready: "queue.Queue[None]" = queue.Queue() + + def _listen() -> None: + self._port = reactor.listenTCP(0, Factory.forProtocol(_SilentProtocol), interface=HOST) + ready.put(None) + + reactor.callFromThread(_listen) + ready.get(timeout=REACTOR_CALL_TIMEOUT) + self.port = self._port.getHost().port + + def stop(self) -> None: + reactor.callFromThread(self._port.stopListening) + + +def _closed_port() -> int: + """A port nothing listens on, found by binding and releasing it.""" + s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + s.bind((HOST, 0)) + port = s.getsockname()[1] + s.close() + return port + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/functional/vncservers.py b/tests/functional/vncservers.py index c60889dc..7cb1735e 100644 --- a/tests/functional/vncservers.py +++ b/tests/functional/vncservers.py @@ -169,6 +169,10 @@ def connect(server: VNCServer, timeout: Optional[float] = None) -> api.ThreadedV Remember that the returned client is a context manager, and that ``api.shutdown()`` still has to be called once before the process exits. + And that shutdown is terminal for the whole process: after it, no + further ``connect()`` can ever work again (the reactor cannot restart), + which is why only ONE test module -- test_api_lifecycle.py -- may use + this in-process; everything else shells out via run_vncdo(). """ client = api.connect( f"{HOST}::{server.port}", diff --git a/tests/servers/servers.mk b/tests/servers/servers.mk index 02db01ad..13243e45 100644 --- a/tests/servers/servers.mk +++ b/tests/servers/servers.mk @@ -25,6 +25,13 @@ test-servers: test-os-server: $(PYTHON) -m unittest discover $(UNITTEST_ARGS) -s tests/functional -t . -p 'test_os_servers.py' +# The in-process vncdotool.api lifecycle suite, against libvncserver-example +# alone. Runs on its own because one process gets one Twisted reactor -- see +# tests/functional/test_api_lifecycle.py. +.PHONY: test-api +test-api: + $(PYTHON) -m unittest discover $(UNITTEST_ARGS) -s tests/functional -t . -p 'test_api_lifecycle.py' + # Screenshot every running test server into $(SCREENSHOT_DIR), including an # index.html gallery of them all, for eyeballing what the servers render. .PHONY: screenshots