Repository navigation
[CI] test: load GitHub operations from their public modules - #26384
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughGitHub test files and the shared test harness now import client functions and cache-reset helpers from their implementation modules instead of the Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No user-facing regression is established. The reported compatibility runs passed, and the reviewed changes present no concrete blocker; fresh shipping-head CI remains part of normal readiness. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a129c20 to
1856e66
Compare
1856e66 to
63754a8
Compare
nwparker
changed the title
1 participant
ELI5
GitHub tests repeatedly load the full client even when they exercise one operation. Import each operation from its existing public module so those tests do less startup work. The complete 27-file workload takes 16.7% less time in the measured trial.
What Changed
Change imports in 26 existing test files and their existing test harness. Also synchronize two entries in the generated English runtime catalog after upstream #26422 removed their literal-default call sites; this lets the unchanged catalog check pass. Each imported function still comes from the module that implements it. The separate public-client test remains, as do all 292 original cases, assertion bodies, mocks, deadlines and the canonical cache reset added in #26346. No production module, dependency or runner setting changes.
Why
Use the existing operation exports to avoid repeatedly loading unrelated GitHub operations. This keeps the existing mock boundaries and test isolation. Sharing state between files or removing setup guards would weaken coverage; adding another implementation or test runner would increase maintenance.
Linked Issue
Related measured test-suite work: #26346, #26360 and #26373.
Visual Proof
N/A — test imports only; no visual or interaction change.
Testing
Compatibility run 37707262849 passed all eight original/candidate commands under Bun 1.4.2 and actual Node 24.21.0 with shuffle seeds 104729 and 130363. Each retained all 27 nonempty test files and the exact same 292 passed case identities, with no skips. Runtime routes, errors, four workers, isolation, all six setup files, source hashes and normal child exits were checked independently.
Timing run 37709277511 retained every whole
pnpm testcommand in before/after, after/before, before/after order:All three pairs improve, including the first commands: 1.20× faster for this complete 27-file cohort. There was no warmup, retry or discarded sample. All six commands passed the same original 292 cases on one held Linux ARM host with Vitest 5.0.3, four workers, isolation, six setup files and the Vitest filesystem module cache disabled. Shared OS and Bun transpiler caches can warm between counted commands. This scoped measurement does not establish shipping CI latency or the overall 2× full-suite goal.
The measured source was
1f790332303926ef8bbec86006dc12ee8654865d, after the canonical cache-reset fix merged. All 27 changed files retain the exact measured before/after bytes on current-main parent00a82505c46d3beb8d7af0df293e88cc363dd394, repaired head0bd619df290c01cd66eae3a868e645e5b6391c90. All 370 source guards were checked. Two intervening changes are inherited: the SQLite registry routes the unrelated Pi launch-resolution test to Node, and the WSL interactive fish launch adds the canonical Claude shell wrapper. The quoting, argv and captured-shell functions used by these tests remain unchanged. The prior shipping head passed 37715370766; the fresh full shipping run 37727635399 passed on this repaired head and its byte-identical merge tree, including measured helper improvement #26396.The first combined run 37726640344 stopped before tests because #26422 removed two translation defaults while the runtime catalog still omitted those English entries. Actual Node and Bun both reproduced the same two missing keys. The existing canonical generator adds exactly those two entries, with no removal or changed value. Both runtimes now pass the full catalog verification, and formatting passes. The failed CI run and its original log remain retained; it supplies no speed evidence.
The original full required-CI baseline is 37443159334: 18m09s from run creation to the successful
verifycheck. The overall target is 9m04.5s. The fresh normal full run 37727635399 took 12m37s, including setup, queues and all required checks: 30.5% less time than the original observed baseline. Its slowest unit test step took 5m10s, versus the original 10m55s: 52.7% less time (2.11×). All ten unit shards, relay integration, Linux/Windows packaging, static analysis, typecheck and requiredverifypassed; path-ineligible contract lanes retain their normal skips. The final verify job still waited 79 seconds to start. The preceding passing shipping run 37715370766 took 18m10s. These are descriptive whole-run observations across evolving source/workloads and variable runner queues, not a matched attribution to this small import change. The overall 9m04.5s target is not yet met.The preceding shipping run 37711195460 passed all ten unit shards but had 13 passing and three failing relay cases after an upstream minimum connection-age change. Fixture repair #26389 is now merged, with all sixteen original relay cases passing and full CI green. This branch was rebased onto that repaired main; all 27 changed before/after file bytes remain exactly identical to the measured candidate. The blocked run remains retained.
Earlier qualification attempts on the source before #26346 remain retained. The original and candidate could both fail the merge-queue case because the summary cache was not cleared. The cache fix is present in both arms of this new comparison; those earlier failures are not treated as speed gains or discarded timing samples.
verify, before merge.Review
Root and independent source reviews verified existing public exports and preserved callbacks/assertions. Two reviewers independently checked all eight compatibility maps and all six raw timing maps, settings, runtime routes, process evidence and arithmetic. Fresh full CI passed. The current-head bot review covers
0bd619dfand reports no actionable comments; there are no unresolved review threads.Agent skill upstream boundary
Notes
Production execution and GitHub/GitLab provider behavior are unchanged. Existing SSH, Windows and folder-workspace expectations remain in the same tests. Native/runtime contracts keep their configured actual Node routes.
Checklist