From 2c0b5704e8dcbc8623baad4f54038392be935733 Mon Sep 17 00:00:00 2001 From: Marc Sibson <125162+sibson@users.noreply.github.com> Date: Fri, 14 Aug 2026 02:57:28 -0400 Subject: [PATCH 1/5] Add the in-process API lifecycle suite (Phase 5) tests/functional/test_api_lifecycle.py covers what a subprocess cannot: vncdotool.api's own lifecycle -- connect/op/disconnect, the documented context-manager pattern, sequential reconnects on one reactor, a TimeoutError against a silent listener, and a prompt ConnectError against a closed port -- all against the single known-good tigervnc container. Server compatibility stays with the subprocess grid. This is the one module permitted to touch the api in-process: the module docstring, vncservers.connect()'s docstring, and the design doc all now state the rule and why it is safe inside the shared functional discover (the module sorts first; api.shutdown() in tearDownModule is terminal for the process). The potentially-wedging closed-port case is ordered last and double-bounded (client timeout plus a joined helper thread) so it can never hang the suite. Happy-path calls carry the same timeout headroom the subprocess grid uses, after a 5s-budget flake was observed under load. make test-api runs the module alone. Both fail-loudly cases pass today without expectedFailure: connect() is fire-and-forget by design, the first proxied call raises TimeoutError via queue.get, and a closed port surfaces Twisted's ConnectionRefusedError promptly. Verified: 5/5 module tests green (twice), full functional discover 36 tests green (twice, clean process exit both times), skips-and-exits with the fleet down, unit suite and flake8 clean. Co-Authored-By: Claude Fable 5 --- Makefile | 1 + docs/testing-framework-design.md | 9 +- tests/functional/test_api_lifecycle.py | 209 +++++++++++++++++++++++++ tests/functional/vncservers.py | 4 + tests/servers/servers.mk | 7 + 5 files changed, 227 insertions(+), 3 deletions(-) create mode 100644 tests/functional/test_api_lifecycle.py 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..ff43b03e 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 it sorts first and every +other module is subprocess-only, and `make test-api` 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..7d5731a5 --- /dev/null +++ b/tests/functional/test_api_lifecycle.py @@ -0,0 +1,209 @@ +"""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. + +THIS IS THE ONE MODULE ALLOWED TO CALL api.connect(). +----------------------------------------------------- + +``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. 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. + +Ordering matters for the same reason: every case here shares that reactor, +torn down once by ``tearDownModule``. The one case that could wedge it (the +closed-port case) is ordered last so a hang there poisons nothing before it. +""" + +import socket +import threading +import time +import unittest + +from twisted.internet.error import ConnectError + +from vncdotool import api + +from .vncservers import DOCKER_SERVERS, HOST, SUBPROCESS_TIMEOUT_HEADROOM, connect, port_open + +TIGERVNC = next(s for s in DOCKER_SERVERS if s.name == "tigervnc") + +# Same headroom the subprocess grid adds: the bare 5s server budget was +# observed to flake under load. +HAPPY_TIMEOUT = TIGERVNC.timeout + SUBPROCESS_TIMEOUT_HEADROOM + +# Tight budget for the cases that want to observe a timeout quickly. +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 + + +def setUpModule() -> None: + if not port_open(HOST, TIGERVNC.port): + raise unittest.SkipTest( + f"tigervnc not reachable on {HOST}:{TIGERVNC.port} -- " + "start the servers first with `make servers-up`" + ) + + +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): + """Ordered lifecycle cases against the tigervnc:5931 container. + + Names are prefixed to fix run order: well-behaved cases first, the + potentially-hanging one last. + """ + + def test_a_connect_op_disconnect(self) -> None: + """connect -> one trivial op -> disconnect: clean, no exception.""" + client = api.connect(f"{HOST}::{TIGERVNC.port}") + client.timeout = HAPPY_TIMEOUT + try: + client.refreshScreen() + finally: + client.disconnect() + + def test_b_context_manager(self) -> None: + """The documented ``with api.connect(...) as client:`` pattern works.""" + with connect(TIGERVNC, timeout=HAPPY_TIMEOUT) as client: + client.keyPress("x") + + def test_c_sequential_connects(self) -> None: + """The reactor survives a disconnect and serves a second connection. + + ``shutdown()`` is what's terminal, not ``disconnect()``: otherwise no + long-running application could reconnect after a single drop. + """ + for _ in range(2): + with connect(TIGERVNC, timeout=HAPPY_TIMEOUT) as client: + client.refreshScreen() + + def test_d_timeout_raises_timeout_error(self) -> None: + """A call against a port that never speaks RFB raises TimeoutError + instead of hanging. + + ``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 + + # Generous bound: proves the timeout fired, without being a + # flakiness trap on a loaded CI box. + self.assertLess(elapsed, SHORT_TIMEOUT + 5.0) + + def test_z_closed_port_raises_promptly(self) -> None: + """connect() to a port nothing listens on fails fast, not hangs. + + Ordered last (``z`` prefix): a regression here could wedge the + shared reactor, so it must not run before anything else. Bounded + twice over -- a per-client timeout, and a helper thread joined with + a deadline -- so even a wedge fails this test rather than 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; see the docstring above for why this " + "test is ordered last" + ) + + 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 _SilentListener: + """A TCP listener that accepts and then never speaks, forcing a + protocol-level timeout rather than a connection-refused. + """ + + def __init__(self) -> None: + self._socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + self._socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + self._socket.bind((HOST, 0)) + self.port = self._socket.getsockname()[1] + self._socket.listen(1) + self._thread = threading.Thread(target=self._serve, daemon=True) + self._stopped = threading.Event() + + def start(self) -> None: + self._thread.start() + + def stop(self) -> None: + self._stopped.set() + self._socket.close() + + def _serve(self) -> None: + self._socket.settimeout(1.0) + while not self._stopped.is_set(): + try: + conn, _ = self._socket.accept() + except TimeoutError: + continue # idle poll; socket.timeout IS an OSError, catch it first + except OSError: + return + # Accept and go silent: read nothing, write nothing, until + # told to stop. + while not self._stopped.is_set(): + time.sleep(0.1) + conn.close() + return + + +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..6615dcf0 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 tigervnc 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 From 92ce5132479d85facaa4e0f97bc866b0c1447233 Mon Sep 17 00:00:00 2001 From: Marc Sibson <125162+sibson@users.noreply.github.com> Date: Sun, 16 Aug 2026 11:49:58 -0400 Subject: [PATCH 2/5] docs: drop the misleading 'sorts first' safety claim for test_api_lifecycle Subprocess-only functional modules never touch the in-process reactor, so run order relative to them can't matter -- only the one-module rule does. The earlier wording implied filename sort order was doing safety work it wasn't. --- docs/testing-framework-design.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/testing-framework-design.md b/docs/testing-framework-design.md index ff43b03e..2ff29ff7 100644 --- a/docs/testing-framework-design.md +++ b/docs/testing-framework-design.md @@ -88,10 +88,10 @@ 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 exactly one module (`test_api_lifecycle.py`) may ever touch `vncdotool.api` in-process; it is -safe inside the shared functional discover because it sorts first and every -other module is subprocess-only, and `make test-api` runs it alone. It does -not fan out across the fleet: server compatibility is already proven by the -subprocess grid. +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) From 8863cb33849639981434a60fb253eebdd4e20684 Mon Sep 17 00:00:00 2001 From: Marc Sibson <125162+sibson@users.noreply.github.com> Date: Sun, 16 Aug 2026 16:02:22 -0400 Subject: [PATCH 3/5] test_api_lifecycle: fail loudly instead of skipping when tigervnc is down A module-level skip meant this whole file's coverage could silently vanish from a green CI run if the port probe ever desynced from the compose healthcheck. setUpModule now raises RuntimeError instead of SkipTest, so unittest reports an ERROR (and a nonzero exit) with the same 'start the servers first' message, rather than a silent skip. Also drop two comments that restated what the next line already says, per the WHAT-vs-WHY comment policy. --- tests/functional/test_api_lifecycle.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/functional/test_api_lifecycle.py b/tests/functional/test_api_lifecycle.py index 7d5731a5..3a19ea0d 100644 --- a/tests/functional/test_api_lifecycle.py +++ b/tests/functional/test_api_lifecycle.py @@ -33,7 +33,6 @@ # observed to flake under load. HAPPY_TIMEOUT = TIGERVNC.timeout + SUBPROCESS_TIMEOUT_HEADROOM -# Tight budget for the cases that want to observe a timeout quickly. SHORT_TIMEOUT = 2.0 # join() budget for the closed-port helper thread: long enough to fail @@ -43,7 +42,7 @@ def setUpModule() -> None: if not port_open(HOST, TIGERVNC.port): - raise unittest.SkipTest( + raise RuntimeError( f"tigervnc not reachable on {HOST}:{TIGERVNC.port} -- " "start the servers first with `make servers-up`" ) @@ -188,8 +187,6 @@ def _serve(self) -> None: continue # idle poll; socket.timeout IS an OSError, catch it first except OSError: return - # Accept and go silent: read nothing, write nothing, until - # told to stop. while not self._stopped.is_set(): time.sleep(0.1) conn.close() From fcc251e381c0558b4517b12d00ce5e10a712a6b1 Mon Sep 17 00:00:00 2001 From: Marc Sibson <125162+sibson@users.noreply.github.com> Date: Sun, 16 Aug 2026 22:12:45 -0400 Subject: [PATCH 4/5] test_api_lifecycle: address review -- drop ordering, retarget, tighten a bound - Target libvncserver-example instead of tigervnc: lighter, more deterministic, and its stdout is already read elsewhere in the fleet for observability. No behavioural reason to prefer tigervnc here. - Drop the a/b/c/d/z ordering scheme. It assumed the closed-port case needed to run last to avoid poisoning the suite, but that case is already bounded by its own timeout and a joined helper thread -- verified live by running it first (it now sorts first alphabetically) and confirming every test after it still passes. Rather than leave connect/disconnect boilerplate duplicated once the ordering rationale was gone, setUpModule now makes the first connection itself (a real connect/refresh/disconnect), which also means the reactor is guaranteed running before any test method needs it, deterministically rather than by being first alphabetically. - Tighten the timeout-path assertion bound from SHORT_TIMEOUT + 5.0 to SHORT_TIMEOUT + 1.0. The wider bound was unexamined padding: it would hide a real timeout-handling regression rather than catch one, and nothing suggested the tighter bound was actually flaky. - Replace the hand-rolled socket+threading silent listener with a Twisted-native one: a Protocol that does nothing, registered on api.connect()'s own already-running reactor via reactor.callFromThread(reactor.listenTCP, ...) rather than a second, unrelated listener thread. Addresses review comments on PR #360. --- tests/functional/test_api_lifecycle.py | 114 +++++++++++-------------- tests/servers/servers.mk | 4 +- 2 files changed, 54 insertions(+), 64 deletions(-) diff --git a/tests/functional/test_api_lifecycle.py b/tests/functional/test_api_lifecycle.py index 3a19ea0d..a8d60188 100644 --- a/tests/functional/test_api_lifecycle.py +++ b/tests/functional/test_api_lifecycle.py @@ -11,27 +11,29 @@ one for it or leave its own reactor thread wedged on exit -- new in-process tests belong in *this* file, sharing *this* reactor. -Ordering matters for the same reason: every case here shares that reactor, -torn down once by ``tearDownModule``. The one case that could wedge it (the -closed-port case) is ordered last so a hang there poisons nothing before it. +``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 -TIGERVNC = next(s for s in DOCKER_SERVERS if s.name == "tigervnc") +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 = TIGERVNC.timeout + SUBPROCESS_TIMEOUT_HEADROOM +HAPPY_TIMEOUT = LIBVNC.timeout + SUBPROCESS_TIMEOUT_HEADROOM SHORT_TIMEOUT = 2.0 @@ -39,13 +41,20 @@ # 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, TIGERVNC.port): + if not port_open(HOST, LIBVNC.port): raise RuntimeError( - f"tigervnc not reachable on {HOST}:{TIGERVNC.port} -- " + 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: @@ -55,37 +64,29 @@ def tearDownModule() -> None: class TestApiLifecycle(unittest.TestCase): - """Ordered lifecycle cases against the tigervnc:5931 container. + """Lifecycle cases against the libvncserver-example:5935 container. - Names are prefixed to fix run order: well-behaved cases first, the - potentially-hanging one last. + 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_a_connect_op_disconnect(self) -> None: - """connect -> one trivial op -> disconnect: clean, no exception.""" - client = api.connect(f"{HOST}::{TIGERVNC.port}") - client.timeout = HAPPY_TIMEOUT - try: - client.refreshScreen() - finally: - client.disconnect() - - def test_b_context_manager(self) -> None: + def test_context_manager_connect(self) -> None: """The documented ``with api.connect(...) as client:`` pattern works.""" - with connect(TIGERVNC, timeout=HAPPY_TIMEOUT) as client: + with connect(LIBVNC, timeout=HAPPY_TIMEOUT) as client: client.keyPress("x") - def test_c_sequential_connects(self) -> None: + def test_sequential_reconnects(self) -> None: """The reactor survives a disconnect and serves a second connection. ``shutdown()`` is what's terminal, not ``disconnect()``: otherwise no long-running application could reconnect after a single drop. """ for _ in range(2): - with connect(TIGERVNC, timeout=HAPPY_TIMEOUT) as client: + with connect(LIBVNC, timeout=HAPPY_TIMEOUT) as client: client.refreshScreen() - def test_d_timeout_raises_timeout_error(self) -> None: + def test_timeout_raises_timeout_error(self) -> None: """A call against a port that never speaks RFB raises TimeoutError instead of hanging. @@ -106,17 +107,17 @@ def test_d_timeout_raises_timeout_error(self) -> None: client.refreshScreen() elapsed = time.monotonic() - start - # Generous bound: proves the timeout fired, without being a - # flakiness trap on a loaded CI box. - self.assertLess(elapsed, SHORT_TIMEOUT + 5.0) + # 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_z_closed_port_raises_promptly(self) -> None: + def test_closed_port_raises_promptly(self) -> None: """connect() to a port nothing listens on fails fast, not hangs. - Ordered last (``z`` prefix): a regression here could wedge the - shared reactor, so it must not run before anything else. Bounded - twice over -- a per-client timeout, and a helper thread joined with - a deadline -- so even a wedge fails this test rather than the suite. + 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. @@ -147,8 +148,7 @@ def attempt() -> None: self.fail( f"connect()/refreshScreen() against a closed port did not " f"return within {HANG_GUARD_TIMEOUT}s -- this would have " - "hung the process; see the docstring above for why this " - "test is ordered last" + "hung the process" ) self.assertIn("exception", outcome, f"expected an exception, got: {outcome}") @@ -157,40 +157,30 @@ def attempt() -> None: self.assertIsInstance(outcome["exception"], ConnectError) +class _SilentProtocol(Protocol): + """Accepts the connection and never sends or reads anything.""" + + class _SilentListener: - """A TCP listener that accepts and then never speaks, forcing a - protocol-level timeout rather than a connection-refused. + """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 __init__(self) -> None: - self._socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - self._socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) - self._socket.bind((HOST, 0)) - self.port = self._socket.getsockname()[1] - self._socket.listen(1) - self._thread = threading.Thread(target=self._serve, daemon=True) - self._stopped = threading.Event() - def start(self) -> None: - self._thread.start() + ready: "queue.Queue[None]" = queue.Queue() - def stop(self) -> None: - self._stopped.set() - self._socket.close() + def _listen() -> None: + self._port = reactor.listenTCP(0, Factory.forProtocol(_SilentProtocol), interface=HOST) + ready.put(None) - def _serve(self) -> None: - self._socket.settimeout(1.0) - while not self._stopped.is_set(): - try: - conn, _ = self._socket.accept() - except TimeoutError: - continue # idle poll; socket.timeout IS an OSError, catch it first - except OSError: - return - while not self._stopped.is_set(): - time.sleep(0.1) - conn.close() - return + 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: diff --git a/tests/servers/servers.mk b/tests/servers/servers.mk index 6615dcf0..13243e45 100644 --- a/tests/servers/servers.mk +++ b/tests/servers/servers.mk @@ -25,8 +25,8 @@ 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 tigervnc alone. Runs -# on its own because one process gets one Twisted reactor -- see +# 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: From 3adaaffd7ba5ec0355b2478e894a85475192b244 Mon Sep 17 00:00:00 2001 From: Marc Sibson <125162+sibson@users.noreply.github.com> Date: Sun, 16 Aug 2026 23:25:39 -0400 Subject: [PATCH 5/5] test_api_lifecycle: trim docstrings that restated their method name Three test docstrings opened with a sentence that just restated the method name (test_sequential_reconnects, test_timeout_raises_..., and test_closed_port_raises_promptly) before getting to the actual WHY. Dropped those opening sentences, keeping only the non-obvious rationale. Also folded the module-level ALL-CAPS banner into plain prose -- no other module in the codebase uses that heading style, and the paragraph right after it already carried the weight. --- tests/functional/test_api_lifecycle.py | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/tests/functional/test_api_lifecycle.py b/tests/functional/test_api_lifecycle.py index a8d60188..1695f33c 100644 --- a/tests/functional/test_api_lifecycle.py +++ b/tests/functional/test_api_lifecycle.py @@ -2,14 +2,12 @@ use, reconnection, error propagation, timeouts, ``shutdown()`` cleanliness. Server compatibility belongs to the subprocess grid in test_servers.py. -THIS IS THE ONE MODULE ALLOWED TO CALL api.connect(). ------------------------------------------------------ - ``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. 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. +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. @@ -77,9 +75,7 @@ def test_context_manager_connect(self) -> None: client.keyPress("x") def test_sequential_reconnects(self) -> None: - """The reactor survives a disconnect and serves a second connection. - - ``shutdown()`` is what's terminal, not ``disconnect()``: otherwise no + """``shutdown()`` is what's terminal, not ``disconnect()``: otherwise no long-running application could reconnect after a single drop. """ for _ in range(2): @@ -87,10 +83,7 @@ def test_sequential_reconnects(self) -> None: client.refreshScreen() def test_timeout_raises_timeout_error(self) -> None: - """A call against a port that never speaks RFB raises TimeoutError - instead of hanging. - - ``api.connect()`` is fire-and-forget and never raises here; the + """``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. """ @@ -113,9 +106,7 @@ def test_timeout_raises_timeout_error(self) -> None: self.assertLess(elapsed, SHORT_TIMEOUT + 1.0) def test_closed_port_raises_promptly(self) -> None: - """connect() to a port nothing listens on fails fast, not hangs. - - Bounded twice over -- a per-client timeout, and a helper thread + """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.