test+fix: expand coverage to 194 + harden 4 high-severity edge cases#28
Merged
Conversation
extension.ts has been growing a long tail of one-off helpers — the
install-in-terminal flow, the clipboard-fallback copier, and the
`pipelineCheck.lspReady` context-key setter — all anchored to that
file because they read or write vscode-namespace APIs. Each one is
small but they collectively obscure the extension's main job
(activation orchestration), and none of them were unit-testable while
they lived inside the activate() closure.
Extract three things:
- install.ts: `installInTerminal`, `copyInstallCommandToClipboard`,
`PIP_INSTALL_COMMAND`. Both the welcome-panel CTA and the LSP-
failure toast now share one implementation per CTA.
- lspState.ts: `setLspReady` + the `LSP_READY_CONTEXT_KEY` constant
that drives the conditional viewsWelcome entries. Single writer,
single key name, single hop to VS Code's setContext channel.
- workspaceScan.ts now exports `findScannableFiles` so the unit
suite can pin the per-pattern findFiles contract that fences
against the nested-brace bug returning.
No behaviour change. extension.ts shrinks; the new modules are pure
and unit-testable. Follow-up commit wires up the tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Backstop the recent fixes and add coverage for surfaces that had none.
The most load-bearing additions:
- findScannableFiles: per-pattern findFiles, dedupe-on-toString,
first-seen order. Locks the fence against the nested-brace glob
that produced "no scannable files found" returning.
- scanWorkspace orchestration: no-workspace path, no-files path,
quiet vs notification toasts, openTextDocument-failure counting,
user cancellation. Previously only formatSummary had tests.
- installInTerminal: opens a terminal, types pip command, asserts
addNewLine=false so the user's unactivated venv isn't violated.
copyInstallCommandToClipboard: clipboard write + status-bar
confirmation TTL. The PIP_INSTALL_COMMAND literal is pinned.
- setLspReady: exact setContext payload for both true and false
transitions. The conditional viewsWelcome panel rides on this.
- registerStatusBar visibility latch: hidden until either a CI
candidate file or a pipeline-check diagnostic is observed; non-
pipeline-check sources don't flip the latch.
- manifest.test.ts: viewsWelcome shape (two gated entries, primary
CTAs, no "Copy install command" button), every welcome-link
command target declared, workspace-trust capability locked.
- Integration: runs the actual pipelineCheck.scanWorkspace command
against the fixture workspace so a real VS Code findFiles walks
the glob — end-to-end regression fence for the nested-brace bug.
Shared infra: the __testStubs__/vscode.ts module gains findFiles,
createTerminal, createStatusBarItem, openTextDocument, clipboard,
setStatusBarMessage, withProgress, configurable workspaceFolders, and
a resetStubState() helper that beforeEach hooks use to keep each
test's globalThis observations isolated.
134 → 187 vitest assertions; lint, typecheck, integration typecheck,
and bundle all green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR enhances the VS Code test infrastructure and refactors LSP installation logic: the test stub now provides observable call recording and state management; LSP readiness and pip installation helpers are extracted into dedicated modules; extension.ts is simplified to delegate responsibilities; and comprehensive unit and integration tests validate manifest contracts, status bar visibility, workspace scanning, and installation flows. ChangesTest Infrastructure and Module Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/manifest.test.ts (1)
150-151: ⚡ Quick winBroaden command-link target parsing to avoid brittle false failures.
At Line 150,
pipelineCheck\.[A-Za-z]+is too narrow and can truncate valid command IDs (for example IDs with extra dots/digits). Widening the capture keeps this contract test future-proof.Suggested diff
- const targets = [...w.contents.matchAll(/command:(pipelineCheck\.[A-Za-z]+)/g)] + const targets = [ + ...w.contents.matchAll(/command:(pipelineCheck\.[A-Za-z0-9._-]+)/g), + ] .map((m) => m[1]);🤖 Prompt for 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. In `@src/manifest.test.ts` around lines 150 - 151, The current extraction for targets in the test uses a too-narrow regex `command:(pipelineCheck\.[A-Za-z]+)` which can truncate valid command IDs; update the regex used when building `targets` (the matchAll call that currently uses `pipelineCheck\.[A-Za-z]+`) to allow digits, additional dots, underscores, and hyphens (for example `command:(pipelineCheck[^\s)]+)` or `command:(pipelineCheck\.[\w.-]+)`) so the full command-id is captured; keep the `.map((m) => m[1])` logic unchanged.src/statusBar.test.ts (2)
298-301: ⚡ Quick winAvoid sharing a mutable
ExtensionContextacross tests.A module-level
ctxletssubscriptionsaccumulate between test cases, which can mask cleanup regressions and introduce cross-test coupling. Create a fresh context per test (or inbeforeEach).♻️ Suggested change
- const ctx = { - subscriptions: [] as Array<{ dispose?: () => void }>, - } as unknown as import("vscode").ExtensionContext; + let ctx: import("vscode").ExtensionContext; + + beforeEach(() => { + ctx = { + subscriptions: [] as Array<{ dispose?: () => void }>, + } as unknown as import("vscode").ExtensionContext; + });🤖 Prompt for 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. In `@src/statusBar.test.ts` around lines 298 - 301, The test file currently defines a module-level ctx object (variable ctx with subscriptions: [] cast to ExtensionContext) which is shared across tests and allows subscriptions to accumulate; change tests to create a fresh ExtensionContext for each test (e.g. instantiate ctx inside each test or in a beforeEach) so the subscriptions array is reinitialized per test, and update any references to the module-level ctx variable to use the per-test/local ctx instead to avoid cross-test coupling and hidden cleanup regressions.
386-398: ⚡ Quick winStrengthen the cleanup assertion to verify the status-bar item is actually subscribed.
The current
subs.length >= 1can pass even if only the diagnostics disposable is registered. Assert that the created item is present insubscriptions.✅ Suggested assertion improvement
registerStatusBar(localCtx); await tick(); - // The returned item itself + the onDidChangeDiagnostics disposable - // both land here. The item should be one of them. - expect(subs.length).toBeGreaterThanOrEqual(1); + const item = lastItem(); + expect(subs).toContain(item);🤖 Prompt for 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. In `@src/statusBar.test.ts` around lines 386 - 398, The test currently only checks subs.length but should verify the actual status bar item was pushed; change registerStatusBar to return the created StatusBarItem (or expose it) and update the test to capture that return value (e.g. const item = registerStatusBar(localCtx)) then assert subs.includes(item) (or find the exact object in subs by identity) so you confirm the status bar item itself was added to localCtx.subscriptions; reference registerStatusBar, localCtx and subs when locating code to change.
🤖 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.
Nitpick comments:
In `@src/manifest.test.ts`:
- Around line 150-151: The current extraction for targets in the test uses a
too-narrow regex `command:(pipelineCheck\.[A-Za-z]+)` which can truncate valid
command IDs; update the regex used when building `targets` (the matchAll call
that currently uses `pipelineCheck\.[A-Za-z]+`) to allow digits, additional
dots, underscores, and hyphens (for example `command:(pipelineCheck[^\s)]+)` or
`command:(pipelineCheck\.[\w.-]+)`) so the full command-id is captured; keep the
`.map((m) => m[1])` logic unchanged.
In `@src/statusBar.test.ts`:
- Around line 298-301: The test file currently defines a module-level ctx object
(variable ctx with subscriptions: [] cast to ExtensionContext) which is shared
across tests and allows subscriptions to accumulate; change tests to create a
fresh ExtensionContext for each test (e.g. instantiate ctx inside each test or
in a beforeEach) so the subscriptions array is reinitialized per test, and
update any references to the module-level ctx variable to use the per-test/local
ctx instead to avoid cross-test coupling and hidden cleanup regressions.
- Around line 386-398: The test currently only checks subs.length but should
verify the actual status bar item was pushed; change registerStatusBar to return
the created StatusBarItem (or expose it) and update the test to capture that
return value (e.g. const item = registerStatusBar(localCtx)) then assert
subs.includes(item) (or find the exact object in subs by identity) so you
confirm the status bar item itself was added to localCtx.subscriptions;
reference registerStatusBar, localCtx and subs when locating code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb15b97c-5bdd-4670-be8e-b767beeafe65
📒 Files selected for processing (11)
src/__testStubs__/vscode.tssrc/extension.tssrc/install.test.tssrc/install.tssrc/lspState.test.tssrc/lspState.tssrc/manifest.test.tssrc/statusBar.test.tssrc/test/integration/activation.test.tssrc/workspaceScan.test.tssrc/workspaceScan.ts
…landed - status snapshot: mark v0.1.1, v0.2.0, v1.0.0 as shipped (was 'in flight'); add Post-1.0.0 row pointing at scan-workspace fix, welcome-panel rework, serialize-javascript override, and open PRs - Tests section: integration tests bullet flipped to done with back-ref to R17/PR #14 and the src/test/integration/activation.test.ts surface - C1 / H4 / maintainer-item-4 manual smokes: marked done; v1.0.0 has been live on the marketplace without a regression report, so the hypothesis is moot
1. LSP crash mid-session no longer leaves the welcome panel showing 'Scan workspace' against a dead server. Subscribe to onDidChangeState so State.Stopped flips pipelineCheck.lspReady back to false; the install-prompt welcome state returns automatically. 2. providerForPath now matches case-insensitively, so a workspace with lowercase 'dockerfile' or 'jenkinsfile' is classified correctly and the disabledProviders middleware can actually silence it. Affects Windows case-preserving filesystems and any user who lowercased the file. Globs themselves stay POSIX-shaped. 3. client.start() is raced against a 30-second timeout. Previously an empty pipelineCheck.serverArgs (or any hung interpreter) left activation pending forever and the welcome panel stuck on the install prompt. On timeout we fire-and-forget client.stop() and surface the standard 'Install in terminal / Open server log' toast. 4. scanWorkspace gates on isLspReady() (new synchronous mirror of the context key) and routes the user toward Install / Restart / Show log via a warning toast when the LSP is down. Previously the scan would openTextDocument every candidate file and complete with a misleading 'scanned N files' toast even though no LSP was alive to produce findings. Quiet mode (scan-on-save) stays silent. Tests: +7 (194 total). Adds coverage for the isLspReady readback, the lowercase/mixed-case provider match, and the LSP-not-ready scan gate (zero counts, no findFiles call, warning toast in noisy mode, silence in quiet mode). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dmartinochoa
changed the title
5 tasks
5 tasks
4 tasks
11 tasks
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.
Summary
Two unrelated workstreams folded into one PR per maintainer instruction:
installInTerminal,copyInstallCommandToClipboard, andsetLspReadyinto their own modules so they're unit-testable instead of trapped inactivate().High-severity fixes
client.onDidChangeState;State.StoppedflipspipelineCheck.lspReadyback to falsedisabledProviderssilently misses lowercasedockerfile/jenkinsfileproviderForPath's internal regex now carries theiflagserverArgs(e.g.[])client.start()raced against a 30s timeout; on timeout we fire-and-forgetclient.stop()and surface the standard install-failure toastScan workspaceagainst a dead LSP completes with a misleading "scanned N files" toastisLspReady()synchronous mirror; surface a warning toast with Install in terminal / Restart language server / Open server log actions. Quiet mode (scan-on-save) stays silent.Test coverage (134 → 194)
findScannableFiles(the nested-brace bug path)scanWorkspaceorchestrationinstallInTerminal+copyInstallCommandToClipboardsetLspReady/isLspReadyregisterStatusBarvisibility latchproviderForPathcase-insensitive matchpipelineCheck.scanWorkspaceThe shared test stub (
src/__testStubs__/vscode.ts) gainedfindFiles,createTerminal,createStatusBarItem,openTextDocument,clipboard,setStatusBarMessage,withProgress, configurableworkspaceFolders, and aresetStubState()helper.Roadmap reconcile
ROADMAP.mdwas contradicting reality (status snapshot said "v0.1.1 → v0.2.0 (in flight)" with v1.0.0 already shipped; integration-test bullet marked[ ]while R17 said landed in PR #14). Now consistent: v1.0.0 shipped, integration tests done, historical manual-smoke items cleared by the marketplace release going without a regression report.Test plan
npm test— 194/194 vitest assertions passnpm run typecheck— cleannpm run lint— cleannpm run bundle:dev— bundle builds (808 kB)tsc -p tsconfig.integration.json --noEmit— integration tests typecheckscanWorkspaceassertion)🤖 Generated with Claude Code