Repository navigation
refactor(11a-2): package-local fakes at the daemon ports; root-own command-surface composition tests - #3519
refactor(11a-2): package-local fakes at the daemon ports; root-own command-surface composition tests#3519thymikee wants to merge 7 commits into
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Thanks for this PR. At 2c7a8d0 I found two test regressions and one duplicated format, and unit-core and Coverage are red because of them. The shared close fixture in session-close-shutdown.fixtures.ts (line 55) swaps the real platform cleanup for a no-op but keeps the old name through an
Could the sort rule be applied all the way? Teardown suites inject The failing checks look related to this change. All 11 unit-core failures are in suites whose fixtures this PR rewired (session-close-shutdown.fixtures.ts and find-handler-fixture.ts), and Coverage fails with them. Lint & Format flags provider-device-admission-fixture.ts and lease.test.ts in oxfmt. Fallow flags the unused exports I did not run the name-parity inventory, so I have not verified that the lease.test.ts to lease-provider-credentials split keeps every assertion. I also did not read all of the roughly 100 renamed test files. I relied on the rename stats and spot checks of import-only hunks. Since this is a draft with port clusters still in progress, this head may be intermediate. Before merge, please wire |
…y suites root-side Owner review on #3519: - Teardown/close suites inject platformResourceCleanupFixture() and assert the stopSnapshotHelper / closeManagedBrowser port members, with a rejecting override for the cleanup-failure channel. The no-op constant and the as-platformResourceCleanup alias are gone; one fixture per port. The adapter's web-device filter (a composition decision) is pinned root-owned in daemon-runtime-web-close-teardown. - find-read-only and find-ambiguous-match run the selector find/wait route, so they relocate to src/__tests__/daemon-composition/interaction/ behind testCreateCommandSurface, per the PR's own sorting rule. - providerCredentialsFixture drops the copied digest format for an opaque deterministic key over the sorted defined pairs. - fakeCommandSurface becomes module-private (fallow's unused export).
|
Addressed the review in 3916a64 (plus 46fef50, your own #3502 row-retirement cherry-pick — the three test-kit approval rows read stale against the updated merge-base):
|
…y suites root-side Owner review on #3519: - Teardown/close suites inject platformResourceCleanupFixture() and assert the stopSnapshotHelper / closeManagedBrowser port members, with a rejecting override for the cleanup-failure channel. The no-op constant and the as-platformResourceCleanup alias are gone; one fixture per port. The adapter's web-device filter (a composition decision) is pinned root-owned in daemon-runtime-web-close-teardown. - find-read-only and find-ambiguous-match run the selector find/wait route, so they relocate to src/__tests__/daemon-composition/interaction/ behind testCreateCommandSurface, per the PR's own sorting rule. - providerCredentialsFixture drops the copied digest format for an opaque deterministic key over the sorted defined pairs. - fakeCommandSurface becomes module-private (fallow's unused export).
46fef50 to
57b6b9b
Compare
|
Base refreshed onto |
|
The earlier CI failures are fixed in 57b6b9b: the no-op cleanup fixture and the find fixture default are corrected, the 11 failing suites pass, and all 19 checks are green. No conflicts. One test gap remains, and the PR body still needs the final gate results before it leaves draft. The Android teardown case in daemon-runtime-web-close-teardown.test.ts would not catch a wrong close. It installs no managed agent-browser and discards stderr. If someone removes the Is one fake per daemon port the goal here ( I did not run the test-name parity inventory against main or the mutation checks. I did not read all of the roughly 100 renamed files and relied on rename stats and the lease split's name comparison. I treated the upstream commits pulled in by the rebase (mutation tooling, limrun, appstate) as upstream and did not review them. I did not look further at the inline port literal in Nothing else stops this delta. Before taking the PR out of draft, please record the final gate set and the name-parity result in the PR body. Also land or explicitly defer the in-progress clusters. |
|
Follow-up in 773f78b:
|
|
This PR is ready, and I found nothing that needs to change at 773f78b. The earlier unit-core, Lint and Fallow failures were fixed at 57b6b9b, and all 18 checks are green. There are no conflicts. The last delta touches one test file only. Not blocking: the comment at https://github.com/callstack/agent-device/blob/773f78b/packages/contracts/src/snapshot-runtime.test.ts#L117 still points to the old I did not run the mutation (removing the platform guard) or the test locally. I read closeManagedBrowser and the seam, and I rely on your description for the rest. The name-parity inventory against main and the unit-core counts (1,580 files, 13,435 tests) are your numbers, and I did not re-run them. I sampled assertions on the lease split only, not all of the roughly 100 renamed files. Nothing else from review stands in the way. The PR is still a draft, so you decide when to mark it ready, and #3544 carries the deferred zero-edge work. |
thymikee
added
the
ready-for-human
…y suites root-side Owner review on #3519: - Teardown/close suites inject platformResourceCleanupFixture() and assert the stopSnapshotHelper / closeManagedBrowser port members, with a rejecting override for the cleanup-failure channel. The no-op constant and the as-platformResourceCleanup alias are gone; one fixture per port. The adapter's web-device filter (a composition decision) is pinned root-owned in daemon-runtime-web-close-teardown. - find-read-only and find-ambiguous-match run the selector find/wait route, so they relocate to src/__tests__/daemon-composition/interaction/ behind testCreateCommandSurface, per the PR's own sorting rule. - providerCredentialsFixture drops the copied digest format for an opaque deterministic key over the sorted defined pairs. - fakeCommandSurface becomes module-private (fallow's unused export).
773f78b to
8f501eb
Compare
|
Coordinator: rebased onto main c13c1bd, which was in conflict. Two main changes needed follow-up, both folded into the relocation commit (11a-2a):
|
|
This PR is ready at 8f501eb. The rebase conflicts in the test import paths are resolved, and the earlier clean review of 773f78b still holds. All 19 checks pass. I did not run the tests or gates locally, so the 1,538 affected tests and the 13,435 unit-core tests are the author's counts. I did not re-audit the 164-file rename set. Commits 2-5 are unchanged, and commit 1 differs only in the expected rebase hunks. The rebase added no fakes, only a relocation. Not blocking, and you can take or leave it: the section comment at https://github.com/callstack/agent-device/blob/8f501eb/packages/contracts/src/snapshot-runtime.test.ts#L117 still says the poll-deadline behavior is proven in |
|
This still looks good at f13636c. Since the clean review at 8f501eb, the only change is a comment edit in The failing iOS smoke job timed out in |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
This PR is ready. The code in 8e599d3 looks good, and the fixes since the earlier review at f13636c hold: the removed mock and the two test fixtures in the new delta look correct. The PR has no production-code changes, and no conflicts. I did not re-verify the 165-file move from the earlier reviews, but the range-diff shows commits 1-6 unchanged. I did not run the test suite locally. I checked by grep only that no test still reaches the real Android helper stop. The only checks not passing are two Smoke Tests, still queued or running with no failure shown. The earlier failure was an iOS runner prepare timeout, and this change does not touch that route. Wait for the Smoke Tests to finish, re-run them if the timeout comes back, then merge. Not blocking, and fine to take or leave: lease-provider-credentials.test.ts copies |
|
All review items closed at
|
…y suites root-side Owner review on #3519: - Teardown/close suites inject platformResourceCleanupFixture() and assert the stopSnapshotHelper / closeManagedBrowser port members, with a rejecting override for the cleanup-failure channel. The no-op constant and the as-platformResourceCleanup alias are gone; one fixture per port. The adapter's web-device filter (a composition decision) is pinned root-owned in daemon-runtime-web-close-teardown. - find-read-only and find-ambiguous-match run the selector find/wait route, so they relocate to src/__tests__/daemon-composition/interaction/ behind testCreateCommandSurface, per the PR's own sorting rule. - providerCredentialsFixture drops the copied digest format for an opaque deterministic key over the sorted defined pairs. - fakeCommandSurface becomes module-private (fallow's unused export).
8e599d3 to
df0af2a
Compare
|
Coordinator: rebased onto main |
|
The PR is ready to merge at df0af2a. The earlier clean review of 8e599d3 still holds, and the new commits are a rebase plus a trivial conflict resolution. All 19 CI checks pass, and I know of no conflicts. I did not rerun the test gates. I rely on the green CI and on your local check:affected run. Not blocking: the real android leg of Nothing else must happen before merge. |
Relocate the 42 daemon test files whose assertions traverse real root command code (proven: they fail against a canned CreateDaemonCommandSurface and pass against the real factory) into src/__tests__/daemon-composition/, preserving subtrees, and move command-surface-fixture itself there since it wraps the root createCommandSurfaceAgentDevice factory. The remaining 82 daemon suites now inject a package-local fake surface at the same port (support/command-surface-fake): inert capture shapes and loud selector refusals, matching the pilot evidence for every surviving suite. The real-surface interaction runners move to a sibling root-owned module, and invokeFindHandler grows an explicit surface parameter so the relocated find-production-route suite keeps driving the composed find->interaction leg. Dispatch-disclosure driver ownership and the boundary-fault evidence path follow the relocated files.
Daemon suites pass the composed cleanup capability down as a port value and assert which finalization the neutral orchestration requested, never how a platform performs it. They now inject a package-local noop fixture at that port (support/platform-resource-cleanup-fixture, with per-member overrides). snapshot-session-cleanup.test.ts asserts the real adapter's platform branching over mocked platform packages, so it relocates to the root-owned composition subtree instead of being converted.
…n ports Daemon suites treat the credential fingerprint as opaque — they assert refusal/reuse on equality, never digest content — so request-router-open, human-control-router-fixture, and the router fixture default now inject a package-local fixture at DaemonProviderCredentials that digests the given environment. The lease suite's provider-credentials family asserts the real reader's provider/manifest-aware digest semantics (and the plugin.fixtures manifest registry), so it moves to the root-owned composition subtree as lease-provider-credentials.test.ts. Provider ownership arrives through the daemon's own ProviderDeviceAdmission install point: snapshot-settings-handler and session-relaunch-close install a package-local admission over runtime fakes instead of the root ambient provider-runtime scope.
…y suites root-side Owner review on #3519: - Teardown/close suites inject platformResourceCleanupFixture() and assert the stopSnapshotHelper / closeManagedBrowser port members, with a rejecting override for the cleanup-failure channel. The no-op constant and the as-platformResourceCleanup alias are gone; one fixture per port. The adapter's web-device filter (a composition decision) is pinned root-owned in daemon-runtime-web-close-teardown. - find-read-only and find-ambiguous-match run the selector find/wait route, so they relocate to src/__tests__/daemon-composition/interaction/ behind testCreateCommandSurface, per the PR's own sorting rule. - providerCredentialsFixture drops the copied digest format for an opaque deterministic key over the sorted defined pairs. - fakeCommandSurface becomes module-private (fallow's unused export).
Owner follow-up: install the fake agent-browser so a mistaken close reaches the process seam, and assert stderr stays empty (the cleanup-failure channel is how such a close surfaces). Proven against a guard-removed closeManagedBrowser: the test fails on the mockRunCmd call; the production edit was not committed.
Follow-up on the ready review: the contract suite's section comment still named src/daemon/__tests__/wait-runtime.test.ts, which this PR moved to src/__tests__/daemon-composition/wait-runtime.test.ts.
Self-review of the full diff: with every cleanup injection point taking platformResourceCleanupFixture(), the close-shutdown fixture no longer reaches the Android snapshot-helper mechanics entry, so its mock override, vi.mocked wrapper, and exported alias are dead; the admission fixture's reset() had no consumer and re-spelled setActive([]).
|
Coordinator: rebased onto main |
df0af2a to
af6925d
Compare
… the mechanics leaf From #3519's review, landed here so the daemon lane keeps no root mock: one root-owned case calls the REAL platformResourceCleanup.stopSnapshotHelper over the mocked android mechanics leaf and asserts an Android device reaches stopAndroidSnapshotHelperSessionForDevice(device) while a non-Android device does not. Both legs were proven to bite by temporarily inverting and then deleting the platform guard.
|
This PR is ready. The earlier review at df0af2a was clean, and af6925d is a rebase with two import-path edits, so nothing new needs fixing. Not blocking: lease-provider-credentials.test.ts still re-declares a small allocateRequest() that lease.test.ts keeps for its own cases, and you can share it through a daemon support fixture if a third user appears or leave it as is. CI is still pending for Lint & Format, Coverage, Integration Tests and Smoke, with no failures so far. A failure would most likely come from import resolution in the relocated suites. I did not run the test suites myself, and I did not re-check the 13,469-test name-parity count or your local check:quick, check:layering and check:affected results. There are no conflicts. Merge once those four checks finish green. |
… the mechanics leaf From #3519's review, landed here so the daemon lane keeps no root mock: one root-owned case calls the REAL platformResourceCleanup.stopSnapshotHelper over the mocked android mechanics leaf and asserts an Android device reaches stopAndroidSnapshotHelperSessionForDevice(device) while a non-Android device does not. Both legs were proven to bite by temporarily inverting and then deleting the platform guard.
This branch has not been deployed
1 participant
Summary
Second half of the daemon test-boundary prep (after #3498). This PR cuts its own clusters — command surface, platform-resource-cleanup, provider admission/credentials — from
src/daemon/test imports. It does not empty the daemon tree: thecreateRequestHandlertest defaults (real root cleanup and claim recovery behind ~33 router suites) and the runtime-gateway/lifecycle, runtime-hints, host-diagnostics, claim-recovery, and perf-capture clusters are deferred to #3544 (stacked on #3517). The zero-edge target forsrc/daemon/holds only at the top of the 11a stack (#3517 → #3544), not at this PR alone; the remaining residue below #3544 is the 11a-3 data-fixture cluster.CreateDaemonCommandSurfacefake was installed and all 124 fixture users run against it. 42 files failed (266 tests): their assertions traverse real root command code, so they relocate tosrc/__tests__/daemon-composition/(subtrees preserved) with the real factory, which itself moved there (command-surface-fixture.tswraps rootcreateCommandSurfaceAgentDevice). The 82 survivors getsupport/command-surface-fake.ts. Real-surface runners and theinvokeFindHandlersurface parameter keep the relocated suites meaningful. Owner review applied the same rule tofind-read-only/find-ambiguous-match(selector find/wait route → root-owned) and re-swept all 388src/daemonsuites: none reachesselectors.*.platformResourceCleanupFixture()and assert thestopSnapshotHelper/closeManagedBrowserport members (rejecting override for the cleanup-failure channel). The composed adapter's platform filtering (Android stop, web-device close, non-web no-dispatch) is pinned root-owned underdaemon-composition/, with the non-web case armed via the fake agent-browser and stderr.support/provider-credentials-fixture.ts(opaque deterministic fingerprint key, no copied digest format) andsupport/provider-device-admission-fixture.ts; the credential-digest family oflease.test.ts(real fingerprint + manifest behavior) split todaemon-composition/handlers/lease-provider-credentials.test.ts.Validation
Head
8e599d3:check:affected --run,check:quick,check:layering,check:di-seams,check:gate-manifest,check:fallow, plus the eager-closure-budgets and test-file-size-ratchet pins, all pass. Full unit-core lane through the host lock: 13,469 tests / 0 failures. Name-parity vs themainbaseline (5680da519, 13,422 names): every baseline assertion survives exactly once; the diffs are parameterized names carrying run-dependent ports/timestamps (matched 1:1) and names added/renamed by main's own rebased commits (#3500/#3504/#3505/#3493/#3473). Fresh-context self-review ran on the rebased diff: two dead-code findings fixed (the close fixture's snapshot-helper mock seam the port assertions superseded; the admission fixture's unusedreset()); the enforcement-table commit-hygiene finding was rejected — folding it would rewrite commits the reviewer verified unchanged under a three-PR stack.