Add the in-process API lifecycle suite (Phase 5) by sibson · Pull Request #360 · sibson/vncdotool

sibson · 2026-08-16T16:35:37Z

Summary

tests/functional/test_api_lifecycle.py covers vncdotool.api's lifecycle in-process (connect/op/disconnect, context-manager use, sequential reconnects, TimeoutError, prompt ConnectError) against the tigervnc container -- the one module permitted to call api.connect(), since a process gets exactly one Twisted reactor. make test-api runs it alone; it also sorts first in the shared functional discover.
make test-api target added; design doc and vncservers.connect() docstring updated to state the one-module rule and why it's safe.

Test plan

make test-api -- 5/5 green
full functional discover (39 tests) -- green, 4 skipped
unit suite + flake8 -- clean

Last phase of the server testing framework (#347, #348, #350, #351, #352 already merged).

🤖 Generated with Claude Code

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 <noreply@anthropic.com>

…ecycle 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.

…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.

sibson · 2026-08-17T01:55:48Z

+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().


gratuitiious, Extraneous comments

🤖 Claude Code: Trimmed. Dropped the now-stale "ordering matters for the same reason" paragraph (the ordering claim itself is gone, see below) and kept only the one-reactor-per-process rationale, which is still load-bearing.

sibson · 2026-08-17T01:57:17Z

+
+
+def setUpModule() -> None:
+    if not port_open(HOST, TIGERVNC.port):


better to run against libvncserver rather than tiger?

🤖 Claude Code: Switched to libvncserver-example -- lighter, more deterministic, and no behavioural reason to prefer tigervnc for a lifecycle-only suite.

sibson · 2026-08-17T02:00:05Z

+class TestApiLifecycle(unittest.TestCase):
+    """Ordered lifecycle cases against the tigervnc:5931 container.
+
+    Names are prefixed to fix run order: well-behaved cases first, the


Dependent ordering is a code smell. We should be reslient to failures. If not let's dig into why to establish this is required with evidence rather than assumed.

🤖 Claude Code: Checked rather than assumed: the closed-port case is already bounded by its own timeout + a joined helper thread, so a wedge there fails only that test. Verified live by running it first (it now sorts first alphabetically) and confirming the tests after it still passed. Dropped the a/b/c/d/z ordering scheme entirely -- it was redundant with protection already in place.

sibson · 2026-08-17T02:00:17Z

+    """
+
+    def test_a_connect_op_disconnect(self) -> None:
+        """connect -> one trivial op -> disconnect: clean, no exception."""


🤖 Claude Code: Removed as a separate test. setUpModule now makes this exact connect/refresh/disconnect itself, which also fixes the reactor-startup ordering dependency (see the ordering thread below) rather than relying on this test happening to run first.

sibson · 2026-08-17T02:00:57Z

+        client = api.connect(f"{HOST}::{TIGERVNC.port}")
+        client.timeout = HAPPY_TIMEOUT
+        try:
+            client.refreshScreen()


rather than op let's call it refresh. op feels more like a keypress

🤖 Claude Code: That exact docstring text is gone now (test_a folded into setUpModule, see above), but noting for the record: agreed, 'refresh' is the clearer word for what refreshScreen() does.

sibson · 2026-08-17T02:01:31Z

+
+    def test_a_connect_op_disconnect(self) -> None:
+        """connect -> one trivial op -> disconnect: clean, no exception."""
+        client = api.connect(f"{HOST}::{TIGERVNC.port}")


should connect/disco be part of setup/teardown? Avoids duplicating code?

🤖 Claude Code: Per-test setUp/tearDown doesn't actually fit here -- test_context_manager_connect, test_sequential_reconnects, and the timeout/closed-port cases each need a different connection pattern or target on purpose, so a shared setUp would just add an unused connection to most of them. Moved the one case that did want a plain connect/refresh/disconnect into setUpModule instead, which removes the duplication AND fixes the reactor-startup-ordering issue in the other thread.

sibson · 2026-08-17T02:01:54Z

+            with connect(TIGERVNC, timeout=HAPPY_TIMEOUT) as client:
+                client.refreshScreen()
+
+    def test_d_timeout_raises_timeout_error(self) -> None:


what is the d for here?

🤖 Claude Code: Letter prefixes dropped entirely -- tests are now named for what they check (test_context_manager_connect, test_sequential_reconnects, test_timeout_raises_timeout_error, test_closed_port_raises_promptly), no ordering scheme.

sibson · 2026-08-17T02:02:18Z

+        elapsed = time.monotonic() - start
+
+        # Generous bound: proves the timeout fired, without being a
+        # flakiness trap on a loaded CI box.


pointless comment?

🤖 Claude Code: Agreed on reflection -- the +5.0 was unexamined padding, and nothing established the tighter bound as actually flaky. Tightened to SHORT_TIMEOUT + 1.0 and rewrote the comment to state that reasoning instead of hand-waving 'generous.'

sibson · 2026-08-17T02:02:49Z

+    protocol-level timeout rather than a connection-refused.
+    """
+
+    def __init__(self) -> None:


Is there not a more Twisted native version of this?

🤖 Claude Code: Yes -- replaced the hand-rolled socket+threading listener with a Protocol that does nothing, registered on api.connect()'s own already-running reactor via reactor.callFromThread(reactor.listenTCP, ...) instead of a second, unrelated listener thread.

…n 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.

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.

sibson and others added 3 commits August 16, 2026 11:43

sibson commented Aug 17, 2026

View reviewed changes

sibson added 2 commits August 16, 2026 22:12

sibson merged commit 00a4dc8 into main Aug 17, 2026
11 checks passed

sibson deleted the claude/fw-phase5-api-lifecycle branch August 17, 2026 03:31



		def setUpModule() -> None:
		if not port_open(HOST, TIGERVNC.port):

Conversation

sibson commented Aug 16, 2026

Summary

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant