Repository navigation
refactor: keep test-only code out of production exports - #3369
Conversation
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 80 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Production-unused exports accumulated while check:production-exports reported them as warnings that never failed. Clear the ones with no production or public consumer: - move test support out of production modules: the maestro conformance projection joins its harness under packages/maestro/test/conformance, and test-only helpers move into co-located *.fixtures.ts modules (iOS conformance harness, maestro position wrapper, audio-probe descriptor, the renamed target-classification fixtures) - point tests at source modules instead of facade re-exports that only tests read (IS_PREDICATES, CLOUD_WEBDRIVER_PROVIDERS, runtime factory, runner artifact, iOS snapshot engine members) - drive helpers through their production callers: tests call buildIosInteractiveSnapshotPresentation, compare iosSnapshotComparisonIdentityKey, and resolve wrappers through resolveElementReportedTwice; the test-only wrappers and the duplicate equality predicate are removed, as is an unused runtime scroll snapshot helper Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tialCase private The failure artifact's replay command quoted the case path with JSON.stringify, which leaves $ and backticks live inside the double quotes; shellQuoteIfNeeded is the shared host-side quoting primitive. The fixtures derive DifferentialCase from compareDifferentialCases, as the generator already does, instead of widening the harness exports. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4fe9877 to
9afc83d
Compare
|
The code looks good at 9afc83d. I have one design question below, so I am not marking it ready yet. Smoke Tests was still running when I checked. The diff only removes exports that tests read and moves test helpers. The daemon, Apple, Android and CLI code bodies on the smoke route are unchanged, so I found no overlap with it. I did not run fallow, so I could not confirm the health baseline state or that the gate is clean at head. No conflicts. Smoke Tests must finish green on 9afc83d before merge. Not blocking, and you can take or leave these. The PR body says the moved conformance-normalize keeps its baseline counts, but at the merge base there is no baseline entry for the old path, so the two complexity_critical and two crap_critical entries at fallow-baselines/health.json:533 are new. Please say so in the body, or drop the entry if the move did not cause them. The new ignoreExports entries at .fallowrc.json:371 keep test-only re-exports (SCRIPT_FLAG_COMMANDS and scriptFlagEntries, acquireDurableCaptureRecoveryAuthorityBeforeDeadline, collectedRecordingPath in stop-sequence.ts). The rest of the PR points tests at the owning module, so these tests could import internal/script-utils.ts, recovery-authority.ts and artifact-paths.ts, and the re-exports and entries could go. The fixture selectMaestroPositionMatches at packages/maestro/src/internal/runtime-target-ranking.fixtures.ts:8 shares its name with a different function in runtime-target-position.ts:17, so a name like resolveMaestroPositionForTest would avoid confusion. The tests at packages/selectors/src/interaction-targeting.test.ts:458 are titled "through the rule on its own" but now call the composite resolveElementReportedTwice, so they need a new title. Could the enforcement footprint be smaller? Production net is -48 lines, but the PR adds a test-only subpath (capture-kit/audio-probe-descriptor-fixtures) and many ownership entries. If tests imported owning modules for every symbol that only tests read, only the fixture subpath would stay, which follows the existing replay-port and selectors fixtures pattern. The managed-local allowlist entry is fine as a recorded seam until there is a decision to delete the staged managed-local lease foundations, since their owner kind is persisted in device-claim and durable-resource records. Both cubic-dev-ai P2 threads are fixed at this commit and can be resolved: #3369 (comment) (shellQuoteIfNeeded now quotes casePath) and #3369 (comment) (DifferentialCase is no longer exported). |
…-control tests Review follow-up. resolveMaestroPositionForTest no longer shares a name with the production selectMaestroPositionMatches, and the wrapper-control tests are titled for the composite resolveElementReportedTwice they now call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Exports maps and the capture-kit subpath pin follow the fixture moves. The moved conformance projection gets new health-baseline entries for its pre-existing complexity findings: fallow 2.104's audit treats a moved file as changed, while the old path needed no entry because no change touched it. The .fallowrc entries name exports whose consumers sit outside fallow's --production graph: a published provider's plugin default export, a member read off a lazy import(), contracts facade members the facade-exhaustiveness rule requires, two test-tree seams with no fixture subpath (.ad grammar drift guard, recovery authority), the recording-path contract, and the staged managed-local lease foundations, which go with their feature rather than export by export. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9afc83d to
600c3cb
Compare
|
Thanks. I tried the smaller footprint, and two gates explain why it stops where it does. Design question (enforcement footprint).
The other new entries are cases fallow can't see: the provider plugin default export, the lazy Non-blocking items, all taken:
Head: |
|
Summary
check:production-exportslisted 59 production-unused exports but never failed: fallow 2.x--fail-on-issuesignored the gate'swarnseverity. This clears them before the fallow 3.32 upgrade (next PR in the stack) makes the gate enforce again, as #1282 intended.None has an external consumer. GitHub code search across the 19 public repos importing
agent-device(includingtester-army/e2e/@e2edev/mobile) found no use, none is documented, and none is reachable fromsrc/sdk/*orsrc/ai-sdk.packages/maestro/test/conformance/; helpers move to co-located*.fixtures.ts.buildIosInteractiveSnapshotPresentation,iosSnapshotComparisonIdentityKey,resolveElementReportedTwice). Test-only wrappers and a duplicate equality predicate are removed.chore(gates)records seams fallow cannot see. Among them are the staged managed-local lease foundations (ADR 0021). Their Simlock side, simlock#70, was closed as not planned. Removing that feature changes persisted record formats, so it is left for a separate decision.80 files. The moved conformance projection gets new health-baseline entries for its pre-existing complexity findings: fallow 2.104 counts a moved file as changed, and the old path never needed an entry. An unused test helper is also dropped.
Validation
Tested
600c3cb39(rebased on main19203c620):pnpm check:affected --runpassed (full check set, selected fail-open by the tooling and config changes).maestro:conformance62/62 andscripts/ios-snapshot-differential.test.ts12/12 (node:test suites outside vitest). Pure tooling and test refactor, so no device run applies.🤖 Generated with Claude Code