Skip to content

fix(tests): the Device Connect swap puts back every module it displaced - #3855

Merged
cagataycali merged 4 commits into
strands-labs:mainfrom
cagataycali:tests/device-connect-swap-restores-modules
Sep 18, 2026
Merged

cagataycali merged 4 commits into
strands-labs:mainfrom
cagataycali:tests/device-connect-swap-restores-modules

Conversation

@cagataycali

@cagataycali cagataycali commented Sep 18, 2026

Copy link
Copy Markdown
Member

What — one owner for the Device Connect swap (tests/_device_connect_real.py) hands back both bindings an import makes, the sys.modules entry and the same-named attribute on the parent package, restored by an autouse conftest fixture. Three copies and their nine cross-test imports collapse onto it. +437/-128 over 19 files.

Why — dropping strands_robots.device_connect.* from sys.modules is not an undo: it orphans every reference a sibling bound at collection time, so monkeypatch.setattr patches a module the code under test no longer reads, and one unit test resolved a hostname (reachy-a.local:8000).

selection before after
test_device_connect_hardening.py then the reachy driver files 4 failed, 234 passed 238 passed
the reverse order (control) 82 passed 82 passed
the 14 files that swap 1882 passed 1882 passed

Teststest_sys_modules_removal_leaves_no_orphan.py now claims the prefix-purge idiom (a variable key filtered through a literal startswith): 5 offenders on main, 0 here. 6 cells, both outcomes planted. Roadmap #3818, 0.8–0.9.


§13 Review rounds

Round Concern Fix commit Pin test
R1 test_insecure_transport_posture_has_one_owner fails: source_device kwarg not accepted by step() under mock @rpc(), and get_rpc_source_device() returns None when sim_driver is imported after conftest fixture e7dabe3 N/A (base-branch test fix, not a regression introduced by this PR)
R2 Same file, [True] cell still red on e7dabe3: the SELF-ASSERTED advisory never fired. Two sibling modules install a MagicMock edge at collection time, and the sim_driver bound under it subclasses a fake whose set_device is a no-op, so the attached runtime was dropped. On main this cell passed only because the hardening file's purge leaked the real-edge module for the rest of the session; with the swap undone per test, the collection-time binding is what a body-level import receives. The two cells that construct a driver now call use_the_real_edge() first, as the estop and port-domain files already do. Reproduced serially with the 23 device_connect files ahead of it: 1 failed -> 2037 passed. 2ccd5d3 N/A (the failing cell is the pin)

Thirteen test modules run against the real device_connect_edge by dropping
strands_robots.device_connect.* from sys.modules so the integration re-imports
against the genuine @rpc / DeviceDriver. Dropping an entry is not an undo: it
orphans every reference a sibling module bound at collection time, and the next
import hands out a different object - so monkeypatch.setattr on the sibling's
binding lands on a module the code under test no longer reads.

With test_device_connect_hardening.py ahead of the reachy driver files, four
cells in tests/drivers/test_reachy_wireless_daemon_protocol.py resolved
reachy-a.local for real and failed; the same four pass in the opposite order.

The swap now has one owner (tests/_device_connect_real.py) that hands back both
bindings an import makes - the sys.modules entry and the attribute of the same
name on the parent package - and restoration has one owner too, an autouse
conftest fixture, so none of the thirteen callers can forget it. Three copies of
the swap and their nine private importers collapse onto it.

tests/test_sys_modules_removal_leaves_no_orphan.py grades the prefix-purge idiom
its literal-key half could not see: a dynamic key filtered through a literal
startswith is now claimed, and reports the five sites plus the sibling each
orphans.
@cagataycali
cagataycali enabled auto-merge (squash) September 18, 2026 07:25
yinsong1986
yinsong1986 previously approved these changes Sep 18, 2026

@yinsong1986 yinsong1986 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR fixes a real cross-file test-ordering bug: five copies of the "swap in the real device_connect_edge" idiom purged strands_robots.device_connect.* from sys.modules without putting the displaced modules back, orphaning bindings that sibling test modules had captured at collection time (so their monkeypatch.setattr calls landed on dead objects and four reachy driver cells resolved a real hostname). The fix gives the swap one owner (tests/_device_connect_real.py) whose restore() puts back both bindings an import makes — the sys.modules entry and the same-named attribute on the parent package — and one owner for the undo (an autouse conftest fixture), collapsing three duplicated helpers and nine private cross-test imports onto it. The existing test_sys_modules_removal_leaves_no_orphan.py guard is extended to grade the prefix-purge idiom it previously could not see, with planted-source tests covering both outcomes (reported / accepted) so the rule itself is verified rather than assumed.

What's good

  • The restore() helper re-binds the parent-package attribute alongside the sys.modules entry, which is the half the old copies missed; the docstring explains why restoring only one of the two is worse than neither.
  • The new lint rule is deliberately conservative (literal startswith prefixes only; parameterized prefixes like purge(prefix) are documented as out of reach) and does not false-positive on the refactored teardowns or on _device_connect_real itself.
  • Import-time behavior of the mock-installing modules is preserved against the base — only the teardown changed from purge to restore, so the blast radius is exactly the stated bug.
  • No host paths, no non-ASCII additions, changelog entry included per repo convention.

@cagataycali cagataycali added P2 Soon process Process and repo hygiene labels Sep 18, 2026
@cagataycali

Copy link
Copy Markdown
Member Author

Required check red on dac6058: test_insecure_transport_posture_has_one_owner.py::...[True] - TypeError: SimulationDeviceDriver.step() got an unexpected keyword argument source_device. The cell calls driver.step(n_steps=3, source_device="ctrl") at lines 164 and 324, while device_connect/sim_driver.py:282 declares step(self, n_steps: int = 1).

…tests

The insecure-transport posture test calls driver.step() directly (not
through the RPC wire protocol), so the @rpc() decorator does not inject
source_device into the context variable. Two fixes:

1. Remove source_device="ctrl" kwarg from driver.step() calls -- the
   mock @rpc() decorator is a passthrough that does not strip framework
   kwargs, so the kwarg reached the method signature and raised TypeError.

2. Patch get_rpc_source_device on the sim_driver module so the
   authorization check sees a named caller ("ctrl") instead of None.
   The conftest autouse fixture patches the symbol on modules already in
   sys.modules at fixture time, but sim_driver is imported inside the
   test body -- after the fixture ran -- binding a fresh reference the
   fixture never saw.
yinsong1986
yinsong1986 previously approved these changes Sep 18, 2026

@yinsong1986 yinsong1986 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Test-only PR that fixes a real cross-file ordering bug: five copies of the "swap in the real device_connect_edge" idiom purged strands_robots.device_connect.* from sys.modules without putting the displaced modules back, orphaning bindings sibling test modules captured at collection time (their monkeypatch.setattr calls landed on dead objects, and four reachy wireless daemon cells resolved reachy-a.local for real). The swap now has one owner (tests/_device_connect_real.py) whose restore() puts back both bindings an import makes -- the sys.modules entry and the same-named attribute on the parent package -- backed by an autouse conftest fixture as the session-level undo, and the two mock-installing files' teardowns switch from purge to restore. The test_sys_modules_removal_leaves_no_orphan.py guard is extended with a prefix-purge rule (variable key filtered through a literal startswith), with planted sources covering both outcomes. I verified the fix locally at head: the reported ordering (test_device_connect_hardening.py then tests/drivers/test_reachy_wireless_daemon_protocol.py) fails the 4 orphan cells on the base commit and passes on this branch; a stress ordering that runs both mock-installing files first drops from 7 failures on base to the pre-existing subset on this branch (a collection-order variant where the wireless file binds a mock-era module reproduces identically on base -- pre-existing, not a regression, and it does not occur under CI's natural alphabetical collection where tests/drivers/ imports before tests/test_device_connect_*.py). No public API surface is touched; ruff check and ruff format --check are clean on all 19 Python files; changelog fragment present; no host paths or non-ASCII additions.

What's good

  • The restore() docstring and module docstring state the two-bindings contract precisely, and the conftest fixture only restores when the snapshot actually changed, so the per-test overhead is two dict scans.
  • The new lint rule is conservative by design (literal startswith prefixes only, parameterized prefixes documented as out of reach) and its planted-source tests cover reported, accepted, comprehension-spelling, and setup/teardown-pair cases -- the rule itself is verified rather than assumed.
  • The R1 fix (patching get_rpc_source_device on sim_driver instead of passing source_device= through a mock @rpc) keeps test_insecure_transport_posture_has_one_owner.py green after the conftest fixture re-imports the module.

Verification suggestions

  • pytest tests/test_device_connect_hardening.py tests/drivers/test_reachy_wireless_daemon_protocol.py -- the 4 previously-orphaned cells pass at head and fail on the merge base.
  • pytest tests/test_sys_modules_removal_leaves_no_orphan.py -- 37 passed, including the new prefix-purge rule and its planted sources.

The two cells in tests/test_insecure_transport_posture_has_one_owner.py
that construct a SimulationDeviceDriver grade the real DeviceDriver's
plumbing: set_device binds _device, and __init__ starts it at None. Two
sibling modules install a MagicMock device_connect_edge at collection
time, and the strands_robots.device_connect.* modules bound under it
subclass a fake whose set_device is a no-op. Once the swap is undone per
test rather than left leaked, that collection-time binding is what a
body-level import receives, so the attached runtime was dropped and the
SELF-ASSERTED advisory never fired (required check red on e7dabe3, the
[True] cell).

Call use_the_real_edge() before the import, as the estop and port-domain
files already do; the session fixture puts the displaced modules back.

@yinsong1986 yinsong1986 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Test-only PR that consolidates five duplicated copies of the "swap in the real device_connect_edge" idiom onto a single owner (tests/_device_connect_real.py) and fixes the real ordering bug those copies shared: purging strands_robots.device_connect.* from sys.modules orphaned every binding a sibling test module captured at collection time, so monkeypatch.setattr patches landed on dead module objects and four reachy wireless daemon cells resolved reachy-a.local for real under --dist loadfile ordering. The new restore() puts back both bindings an import makes (the sys.modules entry and the same-named attribute on the parent package), an autouse conftest fixture is the session-level undo that only fires when the snapshot actually changed, and the two mock-installing files' teardowns switch from purge to restore. The test_sys_modules_removal_leaves_no_orphan.py guard gains a prefix-purge rule (variable key filtered through a literal startswith) with planted sources covering reported, accepted, comprehension-spelling, parameterized-prefix, and setup/teardown-pair cases, so the rule is verified rather than assumed. No src/ changes, no public API surface, changelog fragment follows the <pr#>-slug.md convention, no host paths or non-ASCII additions, and the two review-round concerns (R1 source_device kwarg under a mock @rpc, R2 collection-time MagicMock edge dropping the attached runtime) are both resolved at head, with the previously failing cells now calling use_the_real_edge() before constructing a driver, matching the pattern the estop and port-domain files already use.

@cagataycali
cagataycali merged commit c6fb39d into strands-labs:main Sep 18, 2026
3 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Strands Labs - Robots Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Soon process Process and repo hygiene

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants