Skip to content

test: stub Slack name resolution instead of relying on a dead port - #252

Open
Dhevenddra wants to merge 1 commit into
andrewyng:mainfrom
Dhevenddra:fix/relay-tests-hermetic-name-lookup
Open

test: stub Slack name resolution instead of relying on a dead port#252
Dhevenddra wants to merge 1 commit into
andrewyng:mainfrom
Dhevenddra:fix/relay-tests-hermetic-name-lookup

Conversation

@Dhevenddra

@Dhevenddra Dhevenddra commented Jul 27, 2026

Copy link
Copy Markdown

Problem

Five tests fail deterministically on Windows because of an assumption written into the test
fixture itself:

# tests/test_slack_relay.py
@pytest.fixture(autouse=True)
def _no_slack_network(monkeypatch):
    """... unstubbed lookups must fail instantly at a dead loopback port ..."""
    monkeypatch.setenv("SLACK_API_URL", "http://127.0.0.1:9/")

tests/test_github_installs.py:218 uses the same pattern inline.

"Fails instantly" is true on POSIX. It is not true on Windows. Measured on this host, 5 trials
each:

Target Median Result
socket.create_connection(("127.0.0.1", 9)) 2.033s ConnectionRefusedError
urllib.urlopen("http://127.0.0.1:9/...") 2.032s URLError
socket.create_connection(("localhost", 9)) 4.064s tries ::1, then IPv4

The budget is wait_dispatched(at_least, timeout: float = 2.0).

It compounds, because RelayHub._run only counts a frame after awaiting the handler:

# coworker/connectors/relay_client.py:137-146
if frame is not None:
    handler = self._handlers.get(frame.get("provider") or "slack")
    if handler is not None:
        await handler(frame)        # blocks here
    self._dispatched += 1           # so this never runs in time
    self._progress.set()

and _dispatch_slack_event awaits two lookups per frame (relay_client.py:335 and :339),
each funnelling into _slack_get. So one frame costs roughly 4s against a 2.0s budget.

That is why the diagnostic is only 0 frames dispatched (< 1). Zero rather than partial is what
distinguishes this from ordinary timing jitter.

This is not flakiness

Three consecutive runs of both modules produced the identical five failures each time:

test_relay_dispatches_team_qualified_event
test_relay_two_workspace_fan_in
test_relay_watchdog_reconnects
test_relay_nudge_pulls_history
test_one_hub_fans_out_to_both_adapters

I mention this because #209, #210 and #216 are addressing genuinely flaky tests, and this is a
different problem that happens to share a symptom.

Fix

Stub _slack_get, the single chokepoint both _display_name and _channel_name route through,
so the tests are hermetic by construction rather than by depending on how quickly the host OS
refuses a connection. The module docstring already claims hermeticity ("no live WebSocket ... no
network"); name resolution was the one path still escaping it.

SLACK_API_URL stays pinned at the dead port as a backstop, so any path I have not stubbed still
cannot reach slack.com.

One detail worth flagging for review: the stub is installed on the class, while
test_relay_resolves_names_and_mentions and test_relay_name_cache_is_per_workspace set _slack_get on the
instance. Instance attributes shadow class attributes, so those two tests still exercise real
name mapping and still assert user_name == "Rohit" / chat_name == "ocw-test". Their stubs take
(team_id, method, params) because they are instance-bound; the class-level one takes self.

Verification

Before After
Windows 11, Py 3.11.9, both modules 5 failed, 29 passed, 21.2s 34 passed, 10.2s
Windows 11, full suite 9 failed, 880 passed 3 failed, 886 passed
Linux, Py 3.12, full suite (CI parity, Docker) 889 passed 889 passed, 1 skipped

The two-fold speedup on Windows is just the removed connect stalls.

The 3 remaining Windows failures are unrelated to this PR: two are the user-only file
permission tests covered by #250, and one is
test_workspace_command_trust_controls_live_engine failing with WinError 32 on a directory
rename, which I have not investigated and am not claiming to fix.

Alternative considered

The repo already has a fake_slack fixture in tests/conftest.py that boots an in-process fake
Slack and points SLACK_API_URL at it. That would be more realistic than a stub, but it adds
async startup cost to tests that never assert anything about name resolution, and the dead-port
approach suggests speed was the original priority. Happy to switch to fake_slack if you would
rather these go through the real Web API surface.

Note

Found by running the suite on Windows. CI is ubuntu-latest only, so this cannot surface
upstream today.

Both relay test modules pinned SLACK_API_URL at http://127.0.0.1:9/ on the
stated assumption that lookups "fail instantly at a dead loopback port". That
holds on POSIX, where a refused connection returns in microseconds. It does
not hold on Windows, where refusing a loopback connect takes about 2.0s:

    socket.create_connection(("127.0.0.1", 9))   median 2.033s over 5 trials
    urllib.urlopen("http://127.0.0.1:9/...")     median 2.032s over 3 trials

RelayHub._run only increments _dispatched after awaiting the frame handler,
and _dispatch_slack_event awaits both _display_name and _channel_name, so a
single frame cost roughly 4s against the 2.0s wait_dispatched budget. The
result was a deterministic "only 0 frames dispatched" across five tests,
reproduced identically on three consecutive runs.

Stub _slack_get, the single chokepoint both lookups go through, so the tests
are hermetic by construction rather than by depending on how fast the host OS
refuses a connection. SLACK_API_URL still points at the dead port as a
backstop so an unstubbed path can never reach slack.com.

The stub goes on the class, so the two tests that exercise real name mapping
keep working: they set _slack_get on the instance, which shadows it.

On Windows this takes the two modules from 5 failed / 29 passed in 21.2s to
34 passed in 10.2s. Linux is unaffected: 889 passed, 1 skipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant