Skip to content

review-followups-4: @vscode/test-electron integration tests (R17)#14

Closed
dmartinochoa wants to merge 1 commit into
review-followups-batch-3from
review-followups-batch-4
Closed

review-followups-4: @vscode/test-electron integration tests (R17)#14
dmartinochoa wants to merge 1 commit into
review-followups-batch-3from
review-followups-batch-4

Conversation

@dmartinochoa
Copy link
Copy Markdown
Member

Summary

Fourth batch from the post-v0.1.1 review. Stacked on #13. Lands R17 — `@vscode/test-electron` integration tests.

Scope is deliberately bounded to client-side activation contracts so CI doesn't need Python + pipeline-check[lsp] installed. Future PRs can add an LSP stub to verify end-to-end diagnostic publishing if it earns its keep.

What changed

New test surface

  • .vscode-test.mjs — workspace-folder pinned to `test-fixtures/sample-workflow` so `workspaceContains:` events fire.
  • tsconfig.integration.json — compiles `src/test/integration/` to `out-test/` with mocha globals enabled.
  • src/test/integration/activation.test.ts — 5 tests covering activation, every command's registration, the Findings view registration, the configuration-schema completeness, and the workspace-trust capability declarations.

Wiring

  • `npm run test:integration` compiles + runs the suite via `vscode-test`. `xvfb-run -a npm run test:integration` in CI on Linux only.
  • `tsconfig.json` and `vitest.config.ts` exclude `src/test/integration/` so the main build / unit suite don't pick up the mocha-style files.
  • `.vscodeignore` excludes `out-test/` and the new tsconfig/test-cli config files so the `.vsix` stays clean.

DevDeps

  • `@vscode/test-cli`, `@vscode/test-electron`, `mocha@^11`, `@types/mocha`.
  • Mocha pinned to `^11` deliberately — npm chose `mocha@12-beta` by default and the beta has unfixed advisories in `diff` and `serialize-javascript`. CI's prod-only `npm audit` still returns 0; the dev-only advisories don't reach users.

What this catches that unit tests can't

The 5 integration tests verify:

  1. Activation actually fires. `vi.mock("vscode")` can't tell us whether the extension wakes up in a real editor.
  2. Every declared command registers. Catches the class of bug where a contributions entry exists but the `registerCommand` call is missing (or vice versa).
  3. The Findings view registers under the activity-bar container. The auto-generated `pipelineCheck.findings.focus` command is a proxy for view-registration without using private API.
  4. The configuration schema deserialises — every setting we document is queryable via `config.inspect()`.
  5. Workspace-trust capability is correct — `untrustedWorkspaces.supported === "limited"` and `virtualWorkspaces === false`.

Test plan

  • `npm run lint` clean
  • `npm run compile` clean
  • `npm test` — 90 unit tests pass (unchanged)
  • `npm run test:integration:compile` — emits to `out-test/` cleanly
  • `npm run smoke` clean
  • `vsce ls` confirms `out-test/` + config files don't ship
  • CI run on this PR exercises `xvfb-run -a npm run test:integration` on Linux (Windows/macOS skip).
  • First failing-activation PR after this lands will fail-fast in CI instead of in the marketplace.

Notes

  • The pattern `assert(value)` (default import) instead of `assert.ok(value)` (namespace import) is the cleanest way to get TypeScript's assertion-function narrowing inside the test file.
  • The CI step uses `xvfb-run -a` (auto-server-number) so concurrent matrix shards don't collide on a fixed display number. The Windows/macOS shards don't need it — VS Code's GUI mode works headless there.
  • Open Open VSX's tests run on push only, not pre-merge, so we don't need an Open-VSX-specific integration story.

🤖 Generated with Claude Code

Stacked on review-followups-batch-3 (#13). Boots a real VS Code
extension host in CI and verifies the contracts that unit tests can
only approximate.

Scope deliberately bounded to client-side activation contracts so the
suite doesn't depend on Python + pipeline-check[lsp] being installed
in CI. Future tests can add an LSP stub if end-to-end diagnostic
publishing is worth verifying.

Wiring:
- .vscode-test.mjs: workspace-folder is test-fixtures/sample-workflow,
  so `workspaceContains:` activation events fire.
- tsconfig.integration.json: compiles src/test/integration/ to
  out-test/. Inherits the main tsconfig's strict flags; overrides
  noEmit (the production build is the only no-emit path) and pulls in
  the mocha + node global types.
- tsconfig.json: excludes src/test/integration/ so the main build
  doesn't try to type-check the mocha suite without mocha globals.
- vitest.config.ts: same exclusion so the unit-test suite doesn't
  pick up mocha-style describe/test files.
- package.json: npm run test:integration:compile + test:integration.
- ci.yml: Linux-only step `xvfb-run -a npm run test:integration`.
  Windows / macOS in the matrix still exercise the bundle + smoke +
  unit suite; the integration suite adds the genuine extension-host
  contract on top.
- .vscodeignore: excludes out-test/, .vscode-test.mjs,
  tsconfig.integration.json, vitest.config.ts. Test infra never
  ships in the .vsix.
- .gitignore: adds out-test/. (.vscode-test/ was already ignored.)

Tests (src/test/integration/activation.test.ts, 5 cases):
- Extension is installed and activates.
- All six declared commands register
  (pipelineCheck.restart / showLog / findings.refresh /
  findings.changeGrouping / goToNextFinding / goToPreviousFinding).
- Findings view registers under the activity-bar container (proxied
  by the auto-generated pipelineCheck.findings.focus command).
- Configuration schema exposes every documented setting
  (serverCommand, serverArgs, severityThreshold, disabledProviders,
  trace.server).
- untrustedWorkspaces capability is "limited" and virtualWorkspaces
  is false.

DevDeps:
- @vscode/test-cli, @vscode/test-electron, mocha@^11, @types/mocha.
- mocha pinned to ^11 (npm chose mocha@12-beta by default; the beta
  has unfixed advisories in `diff` and `serialize-javascript`).
  CI's `npm audit --omit=dev` still returns clean — the remaining
  dev-only advisories don't reach users.

Total: 90 unit tests (unchanged), 5 integration tests. Lint, compile,
unit, integration-compile, smoke all green locally.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cd1e345d-b0e0-4e74-9476-fe424a3b1f55

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch review-followups-batch-4

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dmartinochoa
Copy link
Copy Markdown
Member Author

Rolled into #16 (v0.2.0 mega-PR). Every commit from this branch is in #16's history; closing to consolidate the merge.

@dmartinochoa dmartinochoa deleted the review-followups-batch-4 branch May 19, 2026 11:05
dmartinochoa added a commit that referenced this pull request May 19, 2026
…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
dmartinochoa added a commit that referenced this pull request May 19, 2026
…28)

* refactor: extract install + lspState modules from extension.ts

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>

* test: expand coverage to 187 unit + integration tests (was 134)

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>

* docs(roadmap): reconcile with v1.0.0 release + R17 integration tests 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

* fix: harden four high-severity activation / scan edge cases

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>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant