Sweep three registries, cover the WebRTC and platform backends, and raise the floor to 81 - #486
Open
JE-Chen wants to merge 20 commits into
Open
Sweep three registries, cover the WebRTC and platform backends, and raise the floor to 81#486JE-Chen wants to merge 20 commits into
JE-Chen wants to merge 20 commits into
Conversation
The MCP handler table and the AC_* dispatch table are the same layer twice: a thousand short functions that take a client's arguments, call one headless function, and return something json.dumps can encode. They were the two least-covered files in the package because each is two to eight lines, so the per-feature test that touches one is testing the feature, not the wiring -- and wiring fails only when a client calls it. Sweep both registries with arguments taken from the schemas that describe them, never from the adapter source, so neither sweep can pass by restating the code it checks. The callee is replaced by a stub built from its return annotation, which only became possible once the typing contract's exemption list was emptied: an adapter now runs against exactly what its callee promises, with no mouse, display or network. Three MCP adapters need more than the contract offers; each is named with a reason, and a further test deletes the entry when one starts passing. One convention becomes a rule in passing: a mandatory executor parameter with no Script Builder field must be a shape no scalar field could express, or the editor emits an action that raises on first run.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 1968 |
| Duplication | 16 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
The sweep called the parsed path first-party when it merely was not in the standard library, so a `from PySide6... import` would have been imported and stubbed like a project callee. Requiring the package prefix makes the filter mean what its name says and makes the dynamic import provably local; today all 280 resolved modules already match, so nothing swept changes.
…at all Eleven modules under `utils/remote_desktop` raise ImportError at module level without aiortc/av, so 2,090 statements were a hard 0% in CI no matter what anyone wrote. The number is not the point: the tests covering the WebRTC host's auth, TLS, resume tokens and file transfer were already written and `importorskip`ped straight past on every square, so they ran on developer machines and nowhere else. Measured on this tree with one variable changed: 513 of those statements are covered by tests that exist today (+1.23 points on Windows/3.14 end to end). The extra resolves on every square in the matrix -- aiortc 1.15.0 and av 17.1.0 have wheels for win_amd64, manylinux x86_64 and macos-14 arm64 on both ends of the supported Python range. Deliberately not added to `typing-stable-api`: that gate must not depend on what is installed.
Three changes, all the same idea one step further: an adapter can be run against exactly what its callee promises, and the promise is machine-readable. * A dataclass return annotation is not the end of the contract, it is one more level of it. `_value_for` now builds an instance from the dataclass's own fields, so the 92 adapters whose callee returns `HealOutcome`, `PollResult`, `AXTreeNode` and friends run for real -- and their `.to_dict()` runs with them, against the declared shape rather than a mock that answers everything. * Several dozen adapters import a module-level singleton rather than a function -- `default_observer`, `default_scheduler`, `registry` -- and call one method on it. That is the same wiring shape one indirection along, so the callee is the method and its annotation is the contract. Two different methods mean the adapter orchestrates the object, and it stays out. * The REST route table is the third registry of this shape, and the one whose handlers nothing exercised: the existing tests go through the HTTP layer, so on a headless runner they reach a handler only to watch it fall into its own `except`. `test_rest_route_sweep.py` calls all 31 routes with the arguments their own OpenAPI document declares, and asserts what the dispatcher relies on -- (status, dict), a real HTTP code, a payload json.dumps accepts -- for a documented request, an empty one, and one of the wrong shape. The machinery the three sweeps share now lives in `_contract_sweep.py`; each keeps its own argument source, which is what stops a sweep from passing by restating the code it checks. `ac_rrule_next` and its neighbours declare `"format": "date-time"` on the properties they parse, which their descriptions already said in prose. Without it a client generating values from the schema alone gets a ValueError out of `datetime.fromisoformat`, and the sweep got one too.
`trust_list`, `fingerprint`, `address_book` and `adaptive_bitrate` decide who may drive this machine unattended, whether the host answering is the one that answered last time, what the viewer offers to reconnect to, and how hard the encoder is pushed when the link degrades. All four are ordinary Python -- a JSON file, a lock and some arithmetic -- and none of them was imported by any test on any CI square, because the subsystem raised ImportError at module level without the WebRTC extra. 61 tests over the decisions they make in the operator's absence: a trust entry that must not lose its label on re-add, a store that must open empty rather than throw on a truncated file, a fingerprint comparison that must survive an SDP spelling the same certificate in a different case, and a downscale that must need two bad samples rather than one. One is a fix rather than a guard. `AddressBook.set_tags()` cleaned its input with `str(t).strip()`, and `str(None)` is the non-empty string "None", so a JSON `null` -- what a client sends for an omitted tag -- was stored as a tag and then listed by `all_tags()` next to the real ones.
…o 74 `ViewerAuthMixin` decides whether a peer that just connected may drive this machine, and `StatsPoller` produces the numbers the adaptive controller acts on. Both are reachable only through `webrtc_transport`, which raised ImportError at module level without the extra, so neither was imported by any test on any CI square until this week. 56 tests over what they actually decide: a token that is only accepted when it is an equal string, a trust list that fails closed when it cannot be read, an IP whitelist that matches by network rather than by string, a grace period that closes a peer which never authenticated, and five derived rates that must refuse to invent a number from one sample, a zero interval or a counter that went backwards. The auth host double supplies exactly the attribute list the mixin's own docstring asks for, so a mixin that starts reaching for something else fails here rather than leaning on whatever the real host happens to own. `_sole_imported_name` now requires the package prefix, the same tightening the other matcher got: a delegator standing in front of `json.dumps` is not wiring under test, and it makes both dynamic imports provably local. The floor moves 69 -> 74, read from the nine-way matrix at 74.99% (ubuntu-22.04 / 3.14) to 76.19% (windows-2022). The comment now also says which number that is: `coverage report`'s total includes branches and the `coverage.xml` artifact's `line-rate` does not, so the artifact reads ~1.8 points higher and a floor set from it would be one the suite cannot clear.
…ints are Progress.md's open decision — install the extra in CI, or write those 2,090 statements off as dead weight in the denominator — is closed with what it was waiting on measured, and replaced by the roadmap the decision unblocks: which files are still dark on *every* square, which of those can move the floor (only the portable ones do), and which two must not be touched until CI installs their extra, because tests written there would skip and move nothing. It also corrects a trap of its own making. The entry said the floor "had to be dug out of the XML artifact"; `coverage report` is what enforces `fail_under` and it includes branches, while Cobertura's `line-rate` does not — about 1.8 points apart on the same square. A floor set from the artifact is one the suite cannot clear.
…cked `webrtc_files` is a filesystem write driven by remote input — the peer names the file — and its own docstring states the defence: "incoming filenames are stripped of any directory components to defeat path traversal". Nothing tested it. 39 tests do now, including the round trip, where the sender's output is fed straight into the receiver: neither half's view of the protocol is asserted from the other's code. `webrtc_host_media` exists because aiortc has no `removeTransceiver`, so enabling and disabling a viewer stream are not symmetric and each reaches for its slot *by position* — the first video transceiver is the host's own outbound screen track, so the viewer's is the second. 24 tests, one of which exists purely to fail if that off-by-one ever mutes the screen share. Local: 77.04% -> the run before this was 76.60%.
Measured on the last run for this PR: 75.79% (ubuntu-22.04 / 3.14) to 76.99% (windows-2022, 3.11 and 3.12), so the floor follows the lowest square as usual. Progress.md's subsystem table and portable-gap figures are re-measured from the same run rather than carried forward, and the WebRTC follow-up list now names what each remaining module costs — `webrtc_host` and `webrtc_viewer` are the two that need a bigger double than the four already done, and the two mixins that came out of them are the precedent for splitting first.
JE-Chen
changed the title
Progress.md had webrtc_host and webrtc_viewer down as "first see whether a testable half can be split out", on the precedent of the two mixins that had already been carved off them. Going and looking says that was the wrong question: neither constructor touches aiortc, and every collaborator arrives as a keyword argument or a module-level name. What held them at 19.83% and 14.08% was that reaching line one meant standing up an RTCPeerConnection, a screen grabber and a background event loop. 364 tests against doubles instead, and the classes stay where they are. webrtc_host, webrtc_viewer and multi_viewer reach 100%, webrtc_audio 96.27% from zero, webrtc_transport 88.84%; 77.04% -> 79.40% on one machine. The floor stays at 75 until the matrix says otherwise. Most of it is refusal, because that is what these classes do with what arrives from the wire: each of the host's four channels against a peer that never authenticated, one the operator has since put in read-only, and one that is flooding; the three inbox verbs against the _safe_basename that stands between "../secret.txt" and the host's own files; the viewer's slot arithmetic, which has to find the host's recvonly slot by m-line position because aiortc gives every answerer transceiver the same direction. The doubles found a real one. _consume_video caught (OSError, RuntimeError), and aiortc ends a track by raising MediaStreamError, which derives straight from Exception. Nothing awaits that task, so the ordinary end of a session reached the console as an un-retrieved task exception instead of the line the host's own drain loop has always logged. What did have to be split was the test files, against the 750-line limit the project applies to every new one.
Progress.md priced x11_backend and its neighbours as reachable by "only the
two Linux squares", which is work that lifts two of nine when the floor is
the lowest one. One command says otherwise:
python -c "import je_auto_control.wrapper.window_backends.x11_backend"
That passes on Windows with no python-Xlib installed. Every import of Xlib,
Quartz, AppKit, ApplicationServices and comtypes under window_backends/ is
inside a method -- the seam was built that way so a platform without a
backend still imports. What kept them at 0% was that nobody had written the
double.
Stubbed into sys.modules, the package goes 14.24% -> 100% on all nine
squares: x11 and macos from 0%, the selector from 46.34%, the base from
63.64%, the Win32 adapter from 89.19%. 184 tests, whole project 79.40% ->
80.22% on one machine.
The doubles are pinned differently because their constants differ in kind.
Xlib's carry their real X.h values, since those numbers go on the wire, and
test_xlib_stub_values.py compares all twelve against the installed library
wherever there is one -- both Linux squares on CI, and a throwaway
pip --target here, twelve for twelve. pyobjc's are sentinels: each is a key
into a dict the stub builds or a token handed back to a function the stub
provides, so no value reaches any arithmetic, and the names are pinned
against the real frameworks on the macOS squares instead.
Worth knowing for the next one: `from package import name` resolves by
attribute on the package before it consults sys.modules, so registering a
double under its own dotted name does nothing where the real module is
importable. The package has to be the double.
Same fact as the window backends: every comtypes, pyobjc and D-Bus import in utils/accessibility is inside a method, so doubles in sys.modules drive all three platform backends from every square rather than from the one they were written for. windows_backend goes 17.72% -> 99.42%, linux_backend 42.41% -> 100%, macos_backend 0% -> 100%, windows_query 24.75% -> 99.01%, windows_state 18.75% -> 100%, base and the selector to 100%. Whole project 80.22% -> 81.89% on one machine. Two of the doubles earn their keep by refusing things. The UIA one models the indirection every control pattern goes through -- a pattern id, then an interface name queried off the generated module -- and rejects a mismatched pair, because those are two independent constants with nothing checking them and the failure is a None that reads exactly like "no such control". It also raises AttributeError for any Current* property a test did not supply, so `str(pattern.CurrentValue or "")` cannot pass against the repr of a function. The AT-SPI one replaces SessionBus rather than _AtspiConnection. The existing Linux test replaces the connection, which is right for the walk and leaves the whole D-Bus call layer beneath it unexecuted -- and that is where the protocol is: the accessibility bus is not the session bus, an accessible is a (sender, path) pair, and the state bitfield arrives as two 32-bit words, so reading only the first drops every state above bit 31. Writing them turned up a real one, the same shape as the WebRTC viewer's last commit: a catch tuple missing the type that actually gets raised. comtypes reports a provider failure as COMError, which derives straight from Exception. windows_query._uia_errors exists to say so and its docstring names the case, but only the two tree-walking guards in windows_backend used it -- the other 37, every control pattern in the file, named (OSError, AttributeError, ...) and contained none of them. A window closing between the search that found an element and the call that read it raised COMError out of the ac_* tool, past the executor's AutoControlException boundary. All 37 now share one tuple, which only widens what is caught. That fix costs 5 lines in a file already on the over-750 list, so Progress.md carries the reason rather than a split: the right seam there is the UIA COM lifetime management, which has nothing to do with this.
utils/hotkey/backends was the last backend seam with a real hole -- ocr, vision, llm and agent were already within single digits, and accessibility and window management landed in the last two commits. Three platforms, three unrelated mechanisms: RegisterHotKey plus a message pump, XGrabKey on the root window, a CGEventTap on a run loop. 12.64%, 24.46% and 40.94% all go to 100%, and the selector with them. Whole project 81.89% -> 82.40% on one machine. None of them needed a desktop. The Windows backend takes user32 as an argument to the three methods that do the work, so a recorder drives them anywhere; only the prologue that builds it needs ctypes.wintypes, and those two tests say so. The other two import their libraries inside their loops. The X11 one was leaking. `_sync_one` dropped a changed binding's previous registration from its own table and never called ungrab_key, so the old combo stayed grabbed on the server: swallowed from every application, firing nothing, and impossible for _ungrab_all to release at shutdown because it no longer knew about it. Rebind ctrl+alt+k and ctrl+alt+k is dead system-wide until the process exits. The module already knew the shape of that mistake -- the rollback in _grab_masked carries a comment about leaving grabs held with _registered never updated -- and the Windows backend unregisters at exactly this point. Only the rebinding path was missed. These tests grew the Xlib stub from 12 constants to 18 plus a keysym table. All of them are still compared against the installed library on the Linux squares, and against python-Xlib 0.33 here: 31 for 31.
`Quartz.kAXValueCGPointType` does not exist. pyobjc builds ApplicationServices on top of HIServices, which declares the AXValue type constants, and Quartz has no such parent — so the attribute lookup raised AttributeError on the one platform this backend runs on. It took out `move()` and `_point()`, and through `_point` the frame match that picks which accessibility window a Quartz window is, which is the first thing every other action does. The stub answered for those names on Quartz, which is why all nine squares were green. `test_pyobjc_stub_names` exists to stop precisely that, and it did: the two macOS squares failed the moment the stub was measured against the real frameworks. The names move to `AX_NAMES`, so they are now checked against ApplicationServices, where the wheel's `HIServices/_metadata.py` defines them.
`raise state["walk_error"]` reads as `raise None` to an analyser that infers the slot from the dict literal that declared it, and the `is not None` guard above does not narrow that inference — Codacy's pylint reported it and held the PR red while every Actions job was green. Passing the error through a parameter says what it is at the point it is raised.
The sweep stopped at `Optional[X]`, containers, scalars and dataclasses, so
134 adapters whose callee is a class sat out — the biggest remaining group.
The contract is readable there too: a constructor's parameters carry
annotations, so an instance can be built from them exactly as a dataclass is
built from its fields. Measured, 112 of the 134 are reachable that way.
Two rules keep it honest rather than merely running:
A callee that *returns* a class gets a stub returning a real instance, so
`get_egress_policy().is_allowed(url)` and `parse_baggage(header).to_dict()`
run for real against the declared shape.
A callee that *is* a class is left alone entirely. Replacing it would throw
away the client's arguments — the very thing under test — and take its
alternative constructors with it, which is how a stub standing in for a class
first showed up as `'function' object has no attribute 'from_dict'`.
Three consequences fall out of running the genuine objects:
Sample values now follow a declared object's own `properties` instead of
flattening it to `{}`. A tool that names `kind` as required is describing a
payload no client would send empty, and `{}` tests the sample, not the wiring.
A class defined beside a third-party import stays out. `S3ArtifactStore`
builds from its annotations perfectly well and then reaches for `boto3` on
the first method call — the existing "the adapter picks its own backend" rule,
one level down, and eight entries that would all have read "boto3" kept off
the documented-exception list.
Both sweeps run in a directory of their own. A checkpoint store handed the
sample path creates a SQLite database where it stands; unisolated it left
`sample.txt` and `value-for-db` in the repository and made one case depend on
whether another had run first.
586 MCP adapters (was 554) and 537 executor adapters (was 471) now run.
Locally 82.40% to 82.75%. Four adapters need more than any annotation
promises and are named with a reason each, as before.
Running the genuine object means an adapter whose job is to start something really starts it. `AC_usb_watch_start` left a hotplug poller running for the rest of the session, and on Windows that poller shells out to PowerShell every interval — so `test_wayland_libei`, which patches `subprocess.run` across the process and reads the first argv it recorded, read the poller's instead of its own. One square of the matrix, and nowhere else. Every `_start` in both registries has a `_stop` sibling — all eight of them — so the sweep runs it, which also covers the stop adapter from the only state where stopping means anything. The autouse guard is the part that matters beyond these two: the next adapter to grow a thread fails here, by name, instead of becoming somebody else's flake three files away. Also cache the module scan behind the class-callee rule; the sweeps drop from 17.9s to 12.4s.
`send_file` hands the work to `FileTransferSender`, which reads `get_bridge` out of `webrtc_files`. Both fixtures patched it on the viewer and the host instead, so that one path kept the real bridge: the chunks were queued onto a background event loop while the assertion read `sent[0]` straight afterwards. It passed wherever the loop won the race, which was every square until a loaded runner lost it — macos-14/3.10 and ubuntu-22.04/3.10 in the same round, one on the host side and one on the viewer side. `test_webrtc_file_transfer` already patched the right module, which is why it never flaked.
The nine-way matrix runs 81.40% (ubuntu-22.04 / 3.14) to 82.73% (windows-2022 / 3.12), read from `coverage report` — the number `fail_under` is compared against — and not from the XML artifact, which omits branches and reads about two points high. Floored to the integer below the lowest square, as every step of this ratchet has been. Five batches took it from 75: the WebRTC family, the window backends, the accessibility backends, the hotkey backends, and the class-callee half of the adapter sweep. Progress.md closes the coverage item and keeps the section for the traps that will be stepped in again — where the measurement starts, that the floor can only be read from the report, and that only code reachable on all nine squares moves it.
|
JE-Chen
changed the title
JE-Chen
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three registries of short wiring functions are now swept, a subsystem that was measured at 0% is measured at all, and the floor moves 69 → 75.
1.
quality.ymlinstalls the[webrtc]extraEleven modules under
utils/remote_desktopraiseImportErrorat module level withoutaiortc/av— 2,090 statements that were a hard 0% on every square no matter what anyone wrote. The number was the smaller half of the reason. The tests covering the WebRTC host's auth, TLS, resume tokens and file transfer were already written; theyimportorskipped straight past on all nine squares, so they ran on developer machines and nowhere else.Measured with one variable changed: 513 of those statements are covered by tests that exist today (+1.23 points end to end). The extra resolves on every square —
aiortc1.15.0 andav17.1.0 have wheels for win_amd64, manylinux x86_64 and macos-14 arm64 on both ends of the supported Python range.typing-stable-apideliberately does not get it: that gate's verdict must not depend on what is installed.This closes the
DECIDEProgress.mdhad been holding.2. The stub grows one level deeper, and a third registry gets swept
The sweep replaces each adapter's callee with a value built from its return annotation. Three things it could not reach, and now does:
.to_dict()runs too — against the declared shape rather than a mock that answers everything. 92 adapters.default_observer,default_scheduler,registry) and call one method on it. Same wiring shape one indirection along, so the callee is the method and its annotation is the contract. 101 adapters. Calling two methods is orchestration, and stays out.exceptand answer 500. All 31 routes are now called with the arguments their own OpenAPI document declares, and the route table is compared against that document.Shared machinery moved to
test/unit_test/headless/_contract_sweep.py; each sweep keeps its own argument source, which is what stops one from passing by restating the code it checks.3. The WebRTC subsystem gets its first tests
119 tests over four modules that decide things while nobody is watching: who may drive this machine unattended, whether the host answering is the one that answered last time, what a remote peer may write to this disk, and how hard the encoder is pushed when the link degrades.
webrtc_host_auth— a token accepted only when it is an equal string, a trust list that fails closed when unreadable, an IP whitelist that matches by network rather than by string, a grace period that closes a peer which never authenticated.webrtc_files— the peer names the file, so the module's own "stripped of any directory components to defeat path traversal" is now asserted, plus a round trip where the sender's output is fed straight into the receiver.webrtc_stats/adaptive_bitrate— five derived rates that must refuse to invent a number from one sample, a zero interval, or a counter that went backwards.webrtc_host_media— enabling and disabling a viewer stream are asymmetric (aiortc has noremoveTransceiver) and each reaches for its slot by position; one test exists purely to fail if that off-by-one ever mutes the host's own screen share.Fixes that fell out
AddressBook.set_tags()cleaned its input withstr(t).strip(), so a JSONnull— what a client sends for an omitted tag — became a tag literally named"None", whichall_tags()then listed next to the real ones.ac_rrule_next,ac_rrule_occurrencesandac_format_datenow declare the string format they parse. Their descriptions said "ISO" in prose; the schema did not, so a client generating values from the schema alone got aValueErrorout ofdatetime.fromisoformat.The floor: 69 → 75
Nine-way matrix, measured on the last run here: 75.79% (ubuntu-22.04 / 3.14) to 76.99% (windows-2022, 3.11 / 3.12), up from 69.67–70.97.
Progress.mdhad recorded that the floor "had to be dug out of the XML artifact". That is wrong and following it sets a floor the suite cannot clear:coverage report— the step that enforcesfail_under— includes branch coverage becausebranch = true, and Cobertura'sline-ratedoes not. About 2 points apart on the same square (75.79% against 77.78%). Corrected in bothProgress.mdand thepyproject.tomlcomment.Tests: 6,426 → 6,898 passing.
Still open, recorded rather than left in the diff
Progress.mdnow names what is left and what each piece costs: 9,343 statements are dark on every square, and only those can move the floor. The two biggest remaining WebRTC modules need a bigger double than the four already done; the adapter sweep's next step needs a decision, because reaching the "callee is a class" cohort means growing a fake object from the class's methods, and that is where "it ran" and "it verified something" can come apart.utils/officeandsignaling_serverare explicitly marked do-not-touch until CI installs their extras — tests there would skip and move nothing.