test: close phase 3 coverage gaps across telemetry, platform, and the long tail - #553
Conversation
… long tail Phase 3 of #522. Covers 333 of the 334 lines the merged Linux+Windows baseline reported uncovered, entirely from `tests/` — no production file is modified. ## Scope - `telemetry.ts` (106) — `posthog-node` is faked via `mock.module`, which makes it safe to lift the `NODE_ENV === "test"` short-circuit in `trackEvent`. That guard was what left the property builders and env detectors dark; with the SDK faked, no event can leave the process. The CI-provider and shell detectors are driven as `test.each` tables through the captured payload, and the constructor's `options.fetch` wrapper is invoked directly to reach the network-failure fallback. - Platform branches (51) — `binary-upgrade.ts`, `vscode-settings.ts`, and `platform.ts`. `getArtifactInfo` is parameterized over the full platform/arch matrix so every combination runs on every runner. - Commands (60), engine and session-context (56), helpers (61) — error paths, `--json` variants, and malformed-input branches. ## `process.platform` is writable in Bun The planned testability seam in `vscode-settings.ts` proved unnecessary. `process.platform` is a writable, configurable data property, so `Object.defineProperty` plus a cache reset reaches every branch of `platform.ts` and every caller of it from any runner, with the descriptor restored per test. This keeps the macOS and WSL paths covered by tests that run everywhere rather than by `skipIf` tests that contribute to neither CI platform. ## One line is not coverable `runner.ts:348` is the `} catch {` token of a branch whose body is already exercised — the pre-existing tests emit `DA:350,1` beside `DA:348,0`. Bun emits a never-incrementing record for a `catch` clause line when the body has its own statements. A test was written to confirm this moved nothing, then removed rather than left behind. The same accounting explains part of the residual elsewhere: on Linux, Bun emits zero-hit records for blank lines, comments, and closing braces that Windows omits entirely, so the union keeps them at zero permanently. All seven lines the baseline attributed to `prompt.ts` are of this kind, which is why that file is untouched. ## Fixed in passing Two `describe` blocks in `binary-upgrade.test.ts` assigned `globalThis.fetch` directly but relied on `mock.restore()`, which does not undo a direct assignment (ARCH-005). Both now save and restore it. Refs #522 Signed-off-by: Rhuan Barreto <rhuan.barreto@gmail.com> Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe change raises the CI line-coverage threshold from 95% to 99.5% and documents platform simulation and coverage troubleshooting. It adds regression tests for command output, error handling, strict modes, engine failures, archive processing, platform detection, environment state, session discovery, repository detection, Sentry, and telemetry behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Deploying archgate-cli with
|
| Latest commit: |
64abea1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://28860765.archgate-cli.pages.dev |
| Branch Preview URL: | https://rhuanbarreto-phase-3-coverag.archgate-cli.pages.dev |
Code Coverage
Full HTML report available in workflow artifacts. Per-directory breakdown
|
Merged Linux+Windows line coverage is 99.9% (9719/9729) as of the phase 3
test additions, up from 96.6%. The gate and ARCH-005's stated target move
together so the ADR and the workflow agree.
The floor sits below the measured figure by ~39 lines of slack rather than
tracking it exactly, because a residue of lines cannot be covered by any
test. Bun emits a never-incrementing lcov record for some structural
tokens: a `} catch {` whose body has its own statements, and — on Linux
only — blank lines, comments, and closing braces that the Windows run
omits from its records entirely, so the union pins them at zero. Ten such
lines remain across `src/`.
Both the action input and the enforcing step compare with awk, so the
decimal threshold is handled as a float in each.
Refs #522
Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
…the simulation idiom ## ARCH-009 The Context listed "cannot be tested" among the costs of reading `process.platform` directly, on the grounds that the property is read-only in Bun. It is not: the descriptor reports `writable: true, configurable: true`, and tests override it with `Object.defineProperty` to reach every platform branch from any runner. The bullet now states the cost that is real. Simulating a platform means overriding the property AND clearing whatever the reading module cached from it, so routing through the helper lets one seam cover every consumer, while direct reads need an override and a cache reset per call site — and a site that captures the value at module load cannot be re-simulated at all. The Decision is unchanged; it rests on this plus the scattered-logic and missed-WSL rationales already recorded. ## ARCH-005 Implementation Pattern gains the simulation idiom, which removes the need to add testability seams to production code: capture the descriptor, override it, reset the dependent cache, and restore the descriptor in a hook so it cannot leak into later files in the shared process. It also records why the idiom is the only one that moves the number. CI merges Linux and Windows runs only, so a `skipIf`-gated darwin test contributes to neither; parametrizing over the matrix runs every case on every runner. Both edits land outside the sections `review-context` briefs, so the briefing budget is unaffected. Refs #522 Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.archgate/adrs/ARCH-005-testing-standards.md:
- Around line 159-185: Update the example test imports to include it from
"bun:test" before using it in the test case. Keep the existing _resetAllCaches()
calls unchanged, since it is the exported cache-reset function.
In @.claude/agent-memory/archgate-developer/MEMORY.md:
- Line 23: Update the artifact download instructions in the coverage guidance to
use two separate gh run download invocations: download coverage-linux into a and
coverage-windows into b independently, rather than combining both artifact names
with separate destination directories in one command.
In `@tests/commands/review-context-strict.test.ts`:
- Around line 67-75: Update the exitWith mock implementation in the strict
review-context test to capture logSpy’s call count at the moment exitWith(1) is
invoked, and assert that the count is exactly one there. Apply the same ordering
assertion to the additional exitWith stub covering the later test block, while
preserving the existing final stdout assertions.
In `@tests/engine/ast-support-errors.test.ts`:
- Around line 61-70: Update the timeout test around probeInterpreter to mock
Bun.spawn with a controllable process whose exited promise resolves only when
kill() is called, while preserving the immediate probe timeout setup. After
asserting probeInterpreter returns null, verify the spawned process’s kill
method was called exactly once, and restore both the timer and spawn mocks in
cleanup.
In `@tests/formats/pack.test.ts`:
- Around line 148-159: Update the “throws a UserError listing every schema
failure” test around parsePackMetadata to assert the UserError message also
includes the maintainer-specific validation text for the empty maintainers list,
alongside the existing schema failure assertions.
In `@tests/helpers/adr-import-failures.test.ts`:
- Around line 70-94: Update the test around resolveAndCloneSources to use two
unique source entries, configure detectTarget to resolve for the first and
reject for the second, and track both clone directories returned by
shallowClone. Assert the rejection message and verify both directories are
removed, while preserving spy cleanup and call-count assertions.
In `@tests/helpers/adr-writer.test.ts`:
- Around line 181-193: The comment in the “rejects a prefix that resolves to
nothing” test incorrectly describes resolveDomainPrefix behavior. Replace it
with a concise current-behavior statement explaining that an explicitly empty
prefix passed to createAdrFile is invalid, without referencing prefix resolution
or inferred behavior.
In `@tests/helpers/binary-upgrade-archive.test.ts`:
- Around line 135-147: Update downloadReleaseBinary to remove its
archgate-upgrade-* temporary directory on validation or extraction failures,
while preserving it for successful returns. Extend the failure-path tests,
including the unsafe archive cases and the scenarios around the referenced
range, to assert the temporary directory is removed after rejection.
In `@tests/helpers/binary-upgrade-artifact.test.ts`:
- Around line 7-31: Update withSimulatedTarget to call _resetPlatformCache()
immediately after overriding process.platform and process.arch, and again after
restoring their original descriptors in the finally block. Import and reuse
_resetPlatformCache from src/helpers/platform.ts so platform-dependent caches
are refreshed before and after each simulation.
In `@tests/helpers/editor-detect.test.ts`:
- Around line 128-140: Update the “defaults to the first detected editor” test
around promptSingleEditorSelection to make the first available MOCK_DETECTED
editor use a non-"claude" ID, then assert that ID as the default; leave the
no-detected-editors test asserting the "claude" fallback.
In `@tests/helpers/exit.test.ts`:
- Around line 212-217: Remove the misleading comment in the test for the root
command fallback; `exitWith(0)` passes the explicit `"root"` name through
`finalizeCommand`, so the existing `"root"` assertion in the test should remain
unchanged.
In `@tests/helpers/install-info.test.ts`:
- Around line 70-114: Refactor the four independent classification tests around
detectInstallMethod into a single test.each table covering binary, proto, local,
and global-pm paths, with each case setting process.execPath and asserting its
expected method. Keep the proto PROTO_HOME setup case and the ~/.proto
fallback/cache-related test separate because they require additional setup or
assertions.
In `@tests/helpers/session-context-copilot.test.ts`:
- Around line 389-391: Replace the historical link-creation wording in the
comments near the dangling-link fixtures with current behavior: in
tests/helpers/session-context-copilot.test.ts lines 389-391 and
tests/helpers/session-context-cursor.test.ts lines 323-327, state that readdir
lists the dangling link while stat resolves it to ENOENT; make no code changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 86148dac-75e5-40c4-b42d-36a0ff1d2651
📒 Files selected for processing (48)
.archgate/adrs/ARCH-005-testing-standards.md.archgate/adrs/ARCH-009-platform-detection-helper.md.claude/agent-memory/archgate-developer/MEMORY.md.github/workflows/code-pull-request.ymltests/commands/adr/domain/add.test.tstests/commands/adr/domain/list.test.tstests/commands/adr/list.test.tstests/commands/adr/update.test.tstests/commands/check-action-strict.test.tstests/commands/clean.test.tstests/commands/login.test.tstests/commands/plugin/url.test.tstests/commands/review-context-strict.test.tstests/commands/session-context/claude-code.test.tstests/commands/upgrade.test.tstests/engine/ast-support-errors.test.tstests/engine/git-files.test.tstests/engine/rule-scanner.test.tstests/engine/runner-error-paths.test.tstests/engine/safe-path.test.tstests/engine/yaml-utils.test.tstests/formats/pack.test.tstests/helpers/adr-import-failures.test.tstests/helpers/adr-writer.test.tstests/helpers/auth.test.tstests/helpers/binary-upgrade-archive.test.tstests/helpers/binary-upgrade-artifact.test.tstests/helpers/binary-upgrade.test.tstests/helpers/editor-detect.test.tstests/helpers/exit.test.tstests/helpers/init-project-editors.test.tstests/helpers/install-info.test.tstests/helpers/log.test.tstests/helpers/login-flow.test.tstests/helpers/pack-recommend.test.tstests/helpers/paths.test.tstests/helpers/platform-simulated.test.tstests/helpers/project-config.test.tstests/helpers/repo.test.tstests/helpers/sentry.test.tstests/helpers/session-context-copilot.test.tstests/helpers/session-context-cursor.test.tstests/helpers/session-context.test.tstests/helpers/stack-detect-errors.test.tstests/helpers/stream-guards.test.tstests/helpers/telemetry-config.test.tstests/helpers/telemetry-events.test.tstests/helpers/vscode-settings.test.ts
…H-009 ## Tests - `review-context-strict.test.ts` samples the stdout call count inside the `exitWith` stub, so the ordering contract is pinned at the moment the exit is requested rather than inferred from final mock state. - `ast-support-errors.test.ts` counts `proc.kill()` calls while the real subprocess still spawns, so the probe's cleanup is observed. - `pack.test.ts` asserts the empty-`maintainers` error too; the test claims to list every schema failure and asserted two of three. - `adr-import-failures.test.ts` uses two sources, resolving the first target and rejecting the second, so cleanup after an earlier success is covered. - `editor-detect.test.ts` marks an editor available that is neither first in the fixture nor the fallback; both assertions previously expected the same value, so the test passed whether or not detection worked. - `install-info.test.ts` collapses four identical-shape classification cases into one `test.each` table (ARCH-025), keeping the `PROTO_HOME` and caching cases separate for their extra setup and assertions. - Comments in `adr-writer`, `exit`, `session-context-copilot` and `session-context-cursor` tests state current behavior (GEN-004). ## ADRs ARCH-009 named `_resetPlatformCache()` in four places. `platform.ts` exports `_resetAllCaches()`, and that is its only `_reset*` export; every reference now uses the real name. The ARCH-005 example imports `it` and `getVscodeUserSettingsPath`, which its body uses. ## Memory The artifact-download recipe passed `-D` twice. `-n` repeats but `-D` does not, so the second silently overrides the first; a single `-D` now takes both artifacts, which land under distinct subdirectories. Refs #522 Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
|
All 13 review findings are answered on their own threads: 10 fixed in 64abea1, 2 declined with evidence, and the temp-directory leak deferred to #556 (acknowledged above). Two of the fixes closed tests that could not fail, which is the part worth flagging:
The review also surfaced a stale symbol in governance: ARCH-009 named Three production defects found while covering this code are filed rather than folded in, since this PR changes no Coverage 99.9% (9719/9729) against the new 99.5% floor; all checks green. @coderabbitai review |
|
I noted that platform-test documentation must use ✏️ Learnings added
|
Phase 3 of #522. Covers 333 of the 334 lines the merged Linux+Windows baseline reported uncovered — entirely from
tests/, with no production file modified.Baseline:
9414/9748 = 96.6%(reproduced from the #550 merge run'scoverage-linux+coverage-windowsartifacts, digit for digit).Scope
telemetry.tsbinary-upgrade.ts,vscode-settings.ts,platform.tscheck.ts(20),adr-import,init-project, and nine smallersession-context.ts(14),ast-support.ts(13), and eight smallerThe telemetry policy call, settled
#522 left this open: mock the SDK, or keep the "validated via dashboard" policy and make it explicit with coverage-ignore comments. This takes the first option.
The mechanism is worth stating, because it is why the file sat at 54%.
trackEventreturns early whenNODE_ENV === "test", and that single guard is what left the property builders (getStaticProperties,getCommonProperties) and all three env detectors unexecuted — they are only ever reached through a capture. Lifting the guard is only safe onceposthog-nodeis faked; otherwise the tests would queue real events against the production project. So the fake is a prerequisite, not a convenience.With it in place, the CI-provider and shell detectors are driven as
test.eachtables and asserted through the captured payload, and the constructor'soptions.fetchwrapper — otherwise unreachable — is invoked directly to cover the network-failure fallback.mock.moduleon a third-party specifier is permitted by ARCH-005 and bylint/no-first-party-module-mock. It is process-global and retroactive, which here means any later in-processinitTelemetry()gets the fake instead of the real SDK — strictly safer than the status quo. The file header says so.process.platformis writable in BunThe testability seam this was expected to need in
vscode-settings.tsturned out to be unnecessary.process.platformis a writable, configurable data property, soObject.definePropertyplus a cache reset reaches every branch ofplatform.ts— and therefore every caller of it — from any runner, with the descriptor restored per test.That matters beyond the diff: CI merges only Linux and Windows, so a
test.skipIf(platform !== "darwin")test contributes to neither and moves the aggregate by zero. The macOS and WSL paths are now covered by tests that run everywhere.One line is not coverable, and the reason generalizes
runner.ts:348is the} catch {token of a branch whose body is already exercised — the pre-existing tests emitDA:350,1besideDA:348,0. Bun emits a never-incrementing record for acatchclause line when the body has its own statements. A test was written to confirm it moved nothing, then removed rather than left in the suite as a redundant fixture.The same accounting explains a slice of the residual elsewhere. On Linux, Bun emits zero-hit records for blank lines, comments, and closing braces that Windows omits entirely, so the union pins them at zero permanently. All seven lines the baseline attributed to
prompt.tsare of exactly this kind — a blank line, two comments, and four closing braces — which is why that file is untouched. Literal 100% is therefore not attainable, and the threshold below leaves headroom for it.Fixed in passing
Two
describeblocks inbinary-upgrade.test.tsassignedglobalThis.fetchdirectly but relied onmock.restore(), which does not undo a direct assignment (ARCH-005). Both now save and restore it.Two production defects found, deliberately not fixed here
Both were verified empirically rather than inferred, and both are behavioural changes that do not belong in a coverage PR. Filed separately.
binary-upgrade.tscan return an unextracted binary on Windows.Expand-Archive's error is non-terminating, sopowershell -NoProfile -Command "Expand-Archive ..."exits0on a corrupt archive anddownloadReleaseBinaryhands back a path to a binary that was never extracted, instead of raising.binary-upgrade.ts:236can never fire. GNU tar escapes backslashes in-tzfoutput, so a member stored as..\evilis listed as..\eviland normalizes to..//evil, matching none of the three guard conditions.Verification
bun run validateclean: 2358 pass / 0 fail / 25 skip,archgate check51/51 with zero warnings, knip and build check clean.Refs #522
Ratchet
min-coveragemoves 95 -> 99.5, incode-pull-request.yml(both the action input and the enforcing step, which compare withawkand so handle the decimal as a float) and in ARCH-005's stated target, so the ADR and the gate agree.The floor sits ~39 lines below the measured 99.9% rather than tracking it exactly, to leave room for the uncoverable residue described above.
src/commands/src/engine/src/formats/src/helpers/The ten remaining lines are exactly the ones named above:
runner.ts:348, the sevenprompt.tsartifacts, andauth.ts:158— a closing brace insidepollForAccessToken'sif ("error" in data)block that no input can reach, sinceDeviceTokenResponseSchemaguarantees eitheraccess_tokenorerrorand every path through the block ends incontinueorthrow.