Skip to content

test(e2e): add ExTester end-to-end test for the Deepnote notebook flow#430

Merged
tkislan merged 16 commits into
mainfrom
tk/extester-e2e
Jul 1, 2026
Merged

test(e2e): add ExTester end-to-end test for the Deepnote notebook flow#430
tkislan merged 16 commits into
mainfrom
tk/extester-e2e

Conversation

@tkislan

@tkislan tkislan commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a black-box ExTester (vscode-extension-tester) E2E test that drives the full Deepnote happy path through the real VS Code UI: open a workspace folder + a one-notebook .deepnote file → create a Deepnote environment → select it for the notebook (kernel connects, venv + deepnote-toolkit provisioned) → Run All → assert the rendered output contains hello world.

What's included

  • test/e2e/ — the suite (helloWorld.e2e.test.ts), config (tsconfig.json, .mocharc.js, settings.json), and the hello-world fixture
  • package.jsonvscode-extension-tester devDep + compile-e2e / setup:e2e* / test:e2e scripts
  • .gitignore / .vscodeignore — ignore ExTester artifacts and keep them out of the packaged VSIX
  • specs/e2e-extester-testing-plan.md — self-contained plan/reference, with a "§0 Implementation reality" section documenting the fixes below

Key fixes needed to pass headlessly

  • Open the containing workspace folder (the serializer otherwise blocks on a "no workspace folder" snapshot warning) and open the notebook via Quick Open — ExTester's code -r reuse-window silently no-ops in the sandbox
  • Accept the simple folder dialog via its OK button (Enter navigates into directories)
  • Run via the toolbar "Run All" button, re-issued until output renders (deepnote.runallcells is gated behind context keys that are unset under automation)
  • Idempotent environment creation with a stable name (reuses the venv)

No proposed-API grant needed

The extension declares Jupyter's enabledApiProposals, but the core flow (activation, the notebook serializer, kernel execution, output rendering) runs on stable VS Code APIs, so the suite runs on a plain stable VS Code with no product.json allow-listing — the way the published Marketplace / Open VSX extension runs for end users. (An earlier revision patched product.json; that was verified unnecessary and removed.)

How to run

npm run compile
npm run setup:e2e   # download test VS Code + ChromeDriver, install ms-python.python
xvfb-run --auto-servernum --server-args='-screen 0 1920x1080x24' npm run test:e2e

Verification

Verified passing locally on headless Linux (Xvfb, VS Code 1.111.0), with no proposed-API grant in product.json, including a clean-state run that provisions the venv from scratch:

✔ creates an environment, connects the kernel, runs the cell and renders output
1 passing

🤖 Generated with Claude Code

https://claude.ai/code/session_01LQk7n13UfmeDo2ujy8X4LX

Summary by CodeRabbit

  • New Features
    • Added a new E2E UI test suite covering the “hello world” notebook flow, including a dedicated Deepnote fixture and E2E test runner setup.
  • Bug Fixes
    • Improved kernel selection by targeting the visible notebook editor when available.
    • Increased E2E resiliency with longer Mocha timeouts/retries, refined output polling, and more robust toast/quick-input handling.
  • Chores
    • Added an E2E GitHub Actions workflow with caching, headless execution, and failure screenshot uploads; updated VS Code packaging/ignores, license checking rules, and simplified CD dependency installation.

Adds a black-box ExTester (vscode-extension-tester) E2E suite that drives the
full Deepnote happy path through the real VS Code UI: open a workspace folder
and a one-notebook `.deepnote` file, create a Deepnote environment, select it
for the notebook (kernel connects, venv + deepnote-toolkit provisioned), run
the cell, and assert the rendered output contains "hello world".

Beyond the test itself, this wires up reproducible setup and the fixes required
to make it pass in a headless/sandboxed VS Code instance:

- enable-proposed-api.js allow-lists the extension's proposed APIs in the test
  VS Code's product.json (the extension does not activate otherwise)
- open the containing folder as a workspace (the serializer otherwise blocks on
  a "no workspace folder" snapshot warning) and open the notebook via Quick
  Open, since ExTester's `code -r` reuse-window silently no-ops in the sandbox
- accept the simple folder dialog via its OK button (Enter navigates into dirs)
- run via the toolbar "Run All" button and re-issue until output renders
  (deepnote.runallcells is gated behind context keys unset under automation)
- idempotent environment creation with a stable name (reuses the venv)
- exclude e2e artifacts from the VSIX in .vscodeignore

Verified passing locally on headless Linux (Xvfb, VS Code 1.111.0), including a
clean-state run that provisions the venv from scratch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LQk7n13UfmeDo2ujy8X4LX
@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0%. Comparing base (8eddb48) to head (5e4edbf).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@     Coverage Diff     @@
##   main   #430   +/-   ##
===========================
===========================
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tkislan and others added 10 commits June 24, 2026 08:40
Verified the extension does not need a proposed-API grant: although it declares
Jupyter's `enabledApiProposals`, the core flow — activation, the notebook
serializer, kernel execution, and output rendering — runs on stable VS Code APIs
(output goes through `controller.createNotebookCellExecution` with a
`replaceCells` fallback; the one genuinely-proposed call is guarded). On a plain
stable VS Code the proposals are simply ungranted (a non-fatal log) and nothing
breaks — which is how the published Marketplace / Open VSX extension runs for end
users.

So the E2E suite no longer patches the test VS Code's `product.json`:
- remove `test/e2e/enable-proposed-api.js`
- remove the `setup:e2e:proposed-api` script and drop it from `setup:e2e`
- update the plan doc accordingly (§0, §6.x, §8, §9)

Verified: `npm run test:e2e` passes with no Deepnote entry in the test VS Code's
`product.json` allow-list, including a clean-state run that builds the venv from
scratch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LQk7n13UfmeDo2ujy8X4LX
Mirror the unit-test convention (compile-tsc / compile-tsc-watch then
test:unittests): compile the test sources explicitly via `compile-e2e` (or
`compile-e2e-watch` while iterating) instead of auto-compiling through a
`pretest:e2e` npm lifecycle hook. `test:e2e` now just runs the already-compiled
suite under out/e2e. Updates the plan doc to match.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LQk7n13UfmeDo2ujy8X4LX
vscode-extension-tester's transitive deps tripped two CI gates:

- Check Licenses: allow WTFPL (@azu/style-format) and Artistic-2.0
  (binaryextensions/editions/istextorbinary/textextensions/version-range), and
  exclude the proprietary Microsoft-licensed @vscode/vsce-sign* binaries via
  --excludePackagesStartingWith.
- Package Lock Drift: regenerate package-lock.json so `npm install` is a no-op;
  typed-rest-client's requires.qs had the resolved "6.15.2" pinned instead of its
  declared range "^6.9.1", which npm install kept rewriting.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LQk7n13UfmeDo2ujy8X4LX
Remove specs/e2e-extester-testing-plan.md from version control while keeping the
working-tree copy on disk (now untracked). .gitignore is intentionally left
unchanged. The E2E suite under test/e2e/ does not depend on this doc.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LQk7n13UfmeDo2ujy8X4LX
Move the reusable ExTester interaction helpers out of the single large test file
into focused, reusable modules under test/e2e/helpers/:

- constants            timeouts + the output-iframe selector
- fixtures             copyFixtureToTempDir
- notifications        dismissAllNotifications, waitForNotification
- quickInput           tryOpenInputBox, clickDialogOkButton
- workspace            openFolderViaDialog, openWorkspaceFile
- notebook             clickRunAll, readRenderedOutput, runAndAwaitOutput
- deepnoteEnvironment  createEnvironment, selectEnvironmentForNotebook
- index                barrel re-export

Helpers take the WebDriver from VSBrowser.instance.driver and receive the
notebook name as a parameter instead of closing over suite state, so future
suites can reuse them. helloWorld.e2e.test.ts shrinks from 439 to 87 lines and is
now just the suite wiring. Behaviour is unchanged — verified passing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LQk7n13UfmeDo2ujy8X4LX
New .github/workflows/e2e.yml triggers via workflow_run after the CD workflow,
downloads the extension VSIX that CD already built and uploaded, installs it into
the test VS Code (extest install-vsix), and runs the ExTester suite against it —
so the extension is built once (in CD) rather than twice.

Adds a test:e2e:prebuilt script (extest run-tests) that runs the suite against an
already-installed extension without repackaging; the workflow uses it after
install-vsix. Locally verified: install-vsix + test:e2e:prebuilt passes against a
prebuilt VSIX.

workflow_run workflows fire from the default branch, so this takes effect once on
main; a workflow_dispatch trigger is included for manual runs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LQk7n13UfmeDo2ujy8X4LX
Replace the workflow_run-on-CD trigger with push/pull_request to main
plus workflow_dispatch, matching the CI workflow. Package the extension
here via `npm run package` (with the Tailwind native-module workaround
and a global vsce install, same as CD) instead of resolving and
downloading the VSIX artifact from the triggering CD run.

This makes the E2E suite self-contained and PR-gating, dropping the
CD-coupling steps and the actions:read permission they required.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014SefBqWeg8vf7jQ6vDhGU3
The single E2E test spends ~4m30s of its ~5m provisioning the Deepnote
environment, which pip-installs the toolkit dependency tree
(deepnote-toolkit[server], ipykernel, python-lsp-server[all],
deepnote-cli) into a fresh venv — cold from PyPI on every run.

Cache ~/.cache/pip so those wheels are reused across runs. The installs
already use pip's cache (nothing passes --no-cache-dir). The key busts
when the toolkit version / install set changes; the restore-keys prefix
keeps the cache warm across unrelated changes since pip's cache is
additive.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014SefBqWeg8vf7jQ6vDhGU3
ensureControllerSelectedForNotebook passed the NotebookDocument as the
notebook.selectKernel `notebookEditor` argument, which can fail to bind
the controller to the notebook — leaving it without an executable kernel
so the first cell-execution requests are silently dropped until VS Code
happens to settle. The placeholder path already resolves a real
NotebookEditor via findNotebookEditor (with a comment noting it is
required); apply the same here, falling back to the document.

This removes a 40-106s stall observed before the first cell runs after
selecting an environment (kernel now binds in ~0.5s), benefiting both
real users and the E2E suite.

Also harden the E2E run loop: drop RUN_ALL_REISSUE_INTERVAL 25s -> 5s
(and extract OUTPUT_POLL_INTERVAL) so that if a run is ever still
dropped, recovery is fast instead of up to a full 25s per miss.

Measured locally (built VSIX + xvfb + ms-python), the hello-world E2E
test body dropped from ~225s to ~88-107s across 5 runs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014SefBqWeg8vf7jQ6vDhGU3
createEnvironment unconditionally drove the packages and description
input boxes after confirming the name. When the environment name already
exists, `deepnote.environments.create` short-circuits after the name
prompt with an "already exists" notification and opens no further inputs,
so those `InputBox.create()` calls timed out and threw before the helper
reached its `/already exists/` success check — breaking the documented
idempotent retry path (Mocha `retries: 1` plus the workflow retry loop)
on any rerun with a leftover `E2E Hello Env`.

Guard the optional prompts behind `tryOpenInputBox` so they are driven
only when the packages box actually appears; otherwise fall through to
the existing notification check, which matches the sticky "already
exists" toast and reuses the environment. Adds OPTIONAL_PROMPT_TIMEOUT
(5s, matching the previous implicit InputBox.create default).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014SefBqWeg8vf7jQ6vDhGU3
@tkislan tkislan marked this pull request as ready for review June 26, 2026 16:03
@tkislan tkislan requested a review from a team as a code owner June 26, 2026 16:03
@tkislan tkislan requested review from jamesbhobbs and mfranczel June 26, 2026 16:03
Comment thread .github/workflows/e2e.yml Outdated
Co-authored-by: James Hobbs <15235276+jamesbhobbs@users.noreply.github.com>
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 29, 2026
@tkislan tkislan requested a review from dinohamzic June 30, 2026 07:47
Add shared logging helpers for best-effort E2E paths and use them in
all catch handlers across the suite and helpers so transient UI failures
remain visible in test output.
Remove the logCaughtError/catchAndLog helpers and log directly where
errors are caught.
@dinohamzic

Copy link
Copy Markdown
Contributor

@tkislan should we have CodeRabbit do a final pass?

@tkislan

tkislan commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 50065113-40e6-416c-9c44-f03caf893048

📥 Commits

Reviewing files that changed from the base of the PR and between ffc2d64 and 5e4edbf.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
  • package.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json

📝 Walkthrough

Walkthrough

Adds E2E support for the Deepnote extension with new helper modules, a Deepnote fixture, a hello-world UI test, and supporting npm, TypeScript, Mocha, settings, ignore, and GitHub Actions updates. The workflow installs dependencies, builds the extension and E2E sources, provisions the VS Code test runtime, runs the suite under Xvfb, and uploads screenshots on failure. A notebook kernel selection change now passes a NotebookEditor when selecting a kernel.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

Suggested reviewers: andyjakubowski, saltenasl

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Updates Docs ⚠️ Warning No documentation files appear in the branch diff; only code, test, and workflow files changed. Update the OSS docs and the landing-page roadmap in deepnote/deepnote-internal; I can’t see the private repo, so please verify that change too.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding an ExTester E2E test for the Deepnote notebook flow.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (4)
test/e2e/helpers/fixtures.ts (1)

17-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Temp dirs are never cleaned up. Each call leaks a deepnote-e2e-* dir under os.tmpdir(). Negligible in CI, noisy locally. Consider returning/registering a cleanup (e.g. fs.rmSync(tempDir, { recursive: true, force: true })) in the suite's after.

🤖 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 `@test/e2e/helpers/fixtures.ts` around lines 17 - 24, The copyFixtureToTempDir
helper leaks temporary directories because it creates a deepnote-e2e-* folder
without any cleanup hook. Update copyFixtureToTempDir to either return cleanup
info alongside tempDir/filePath or register the created tempDir for teardown,
and make sure the e2e suite’s after/afterEach removes it with fs.rmSync using
recursive and force options. Keep the fix centered around copyFixtureToTempDir
and the suite teardown so every temporary fixture directory is cleaned up.
test/e2e/helpers/deepnoteEnvironment.ts (1)

34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Name the interpreter-pick timeout.

tryOpenInputBox(5_000) uses a raw literal while line 69 uses OPTIONAL_PROMPT_TIMEOUT. Add a sibling constant (e.g. INTERPRETER_PROMPT_TIMEOUT) for consistency.

As per coding guidelines: "Extract magic numbers (retry counts, delays, timeouts) as named constants near the top of the module".

🤖 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 `@test/e2e/helpers/deepnoteEnvironment.ts` at line 34, The interpreter-pick
flow in deepnoteEnvironment still uses a raw timeout literal, which should be
extracted into a named constant for consistency with the existing prompt
timeout. Add a top-of-module constant alongside OPTIONAL_PROMPT_TIMEOUT (for
example, INTERPRETER_PROMPT_TIMEOUT) and use it in tryOpenInputBox within the
interpreter selection logic so the timeout is clearly named and easy to
maintain.

Source: Coding guidelines

test/e2e/helpers/notebook.ts (1)

49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the 5_000 frame-switch timeout as a named constant.

switchToFrame(5_000) is a raw literal while every other timeout in this suite is a named constant in constants.ts. Hoist it (e.g. OUTPUT_FRAME_SWITCH_TIMEOUT).

As per coding guidelines: "Extract magic numbers (retry counts, delays, timeouts) as named constants near the top of the module".

🤖 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 `@test/e2e/helpers/notebook.ts` at line 49, The notebook helper uses a raw
frame-switch timeout literal in the webView.switchToFrame call, while the rest
of the suite keeps timeouts in constants. Extract the 5_000 value into a named
constant near the top of test/e2e/helpers/notebook.ts (for example, a dedicated
frame-switch timeout constant alongside the other module constants), then update
switchToFrame to use that constant.

Source: Coding guidelines

test/e2e/suite/helloWorld.e2e.test.ts (1)

39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the suite timeout constant.

Line 40 hardcodes another timeout value inline. Please lift it into a named module constant with the other test constants so future tuning stays centralized. As per coding guidelines, extract magic numbers (retry counts, delays, timeouts) as named constants near the top of the module.

🤖 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 `@test/e2e/suite/helloWorld.e2e.test.ts` around lines 39 - 40, The suite
timeout in helloWorld.e2e.test.ts is still hardcoded inline, so move the value
used by the `this.timeout(...)` call into a named module-level constant
alongside the other test constants near the top of the file. Update the
`describe`/suite setup to reference that constant instead of the literal so
timeout tuning stays centralized and consistent with the coding guidelines.

Source: Coding guidelines

🤖 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 @.github/workflows/e2e.yml:
- Around line 30-31: Update the Checkout step in the e2e workflow to disable
credential persistence. In the actions/checkout usage, set persist-credentials
to false so the GitHub token is not left in local git config before the later
npm and packaging commands run.
- Around line 44-53: The fallback dependency installs in the e2e workflow are
unpinned, so they can pull newer releases than the lockfile intends. Update the
Install dependencies and Install vsce steps to use exact versions for the native
packages and `@vscode/vsce`, or move those dependencies into package.json and
invoke them via npm ci / npm exec. Keep the fixes localized around the
lightningcss, `@tailwindcss/oxide`, and vsce install commands.

In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts`:
- Around line 740-747: The notebook kernel selection path is still falling back
to a NotebookDocument when findNotebookEditor() returns nothing, but
notebook.selectKernel only works with a NotebookEditor. Update
deepnoteKernelAutoSelector.node.ts in the code path around selectKernel to
return early when findNotebookEditor(notebook) yields no editor, and only pass
the notebookEditor into commands.executeCommand; align this behavior with
selectPlaceholderController() so we do not send an invalid payload.

---

Nitpick comments:
In `@test/e2e/helpers/deepnoteEnvironment.ts`:
- Line 34: The interpreter-pick flow in deepnoteEnvironment still uses a raw
timeout literal, which should be extracted into a named constant for consistency
with the existing prompt timeout. Add a top-of-module constant alongside
OPTIONAL_PROMPT_TIMEOUT (for example, INTERPRETER_PROMPT_TIMEOUT) and use it in
tryOpenInputBox within the interpreter selection logic so the timeout is clearly
named and easy to maintain.

In `@test/e2e/helpers/fixtures.ts`:
- Around line 17-24: The copyFixtureToTempDir helper leaks temporary directories
because it creates a deepnote-e2e-* folder without any cleanup hook. Update
copyFixtureToTempDir to either return cleanup info alongside tempDir/filePath or
register the created tempDir for teardown, and make sure the e2e suite’s
after/afterEach removes it with fs.rmSync using recursive and force options.
Keep the fix centered around copyFixtureToTempDir and the suite teardown so
every temporary fixture directory is cleaned up.

In `@test/e2e/helpers/notebook.ts`:
- Line 49: The notebook helper uses a raw frame-switch timeout literal in the
webView.switchToFrame call, while the rest of the suite keeps timeouts in
constants. Extract the 5_000 value into a named constant near the top of
test/e2e/helpers/notebook.ts (for example, a dedicated frame-switch timeout
constant alongside the other module constants), then update switchToFrame to use
that constant.

In `@test/e2e/suite/helloWorld.e2e.test.ts`:
- Around line 39-40: The suite timeout in helloWorld.e2e.test.ts is still
hardcoded inline, so move the value used by the `this.timeout(...)` call into a
named module-level constant alongside the other test constants near the top of
the file. Update the `describe`/suite setup to reference that constant instead
of the literal so timeout tuning stays centralized and consistent with the
coding guidelines.
🪄 Autofix (Beta)

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: CHILL

Plan: Pro

Run ID: f4de13a4-70fc-412b-b8d6-ac5991e9f604

📥 Commits

Reviewing files that changed from the base of the PR and between aef728e and 0163085.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (18)
  • .github/workflows/e2e.yml
  • .gitignore
  • .vscodeignore
  • package.json
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
  • test/e2e/.mocharc.js
  • test/e2e/fixtures/hello-world.deepnote
  • test/e2e/helpers/constants.ts
  • test/e2e/helpers/deepnoteEnvironment.ts
  • test/e2e/helpers/fixtures.ts
  • test/e2e/helpers/index.ts
  • test/e2e/helpers/notebook.ts
  • test/e2e/helpers/notifications.ts
  • test/e2e/helpers/quickInput.ts
  • test/e2e/helpers/workspace.ts
  • test/e2e/settings.json
  • test/e2e/suite/helloWorld.e2e.test.ts
  • test/e2e/tsconfig.json

Comment thread .github/workflows/e2e.yml
Comment thread .github/workflows/e2e.yml Outdated
Comment thread src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts Outdated

@dinohamzic dinohamzic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some AI findings:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts: The fix still falls back to passing NotebookDocument into notebook.selectKernel when findNotebookEditor() misses. That is the exact path the comment says can leave the notebook without an executable kernel, and the caller still reports “Kernel ready”. I’d align this with selectPlaceholderController: warn/return or throw when no editor is found, and only call notebook.selectKernel with a real NotebookEditor.

  • test/e2e/helpers/notebook.ts: The E2E test can mask the kernel-selection regression because it re-clicks “Run All” every 5s and ignores click failures until eventual output appears. Since the production bug is “first execution requests are silently dropped”, this can still pass if a later click succeeds. For this regression, the test should issue one run after environment selection, or have a separate stricter assertion for first-run behavior.

  • .github/workflows/e2e.yml: actions/checkout leaves the PR token persisted in git config, then the workflow runs PR-controlled npm/build commands. Set persist-credentials: false for this pull-request workflow.

  • .github/workflows/e2e.yml: The fallback npm install ... commands and global npm install -g @vscode/vsce bypass the lockfile, so the E2E VSIX may be built with unreviewed newer tooling/packages. Use exact versions or make them normal dev dependencies invoked through npm ci/npm exec.

Resolve the review comments from CodeRabbit and Dino on PR #430.

Major:
- ci: set persist-credentials: false on actions/checkout in e2e.yml so the
  GITHUB_TOKEN is not left in git config for PR-controlled build steps to read.
- deps: declare @vscode/vsce as a devDependency and the lightningcss /
  @tailwindcss/oxide linux-x64 binaries as optionalDependencies, then drop the
  unpinned `npm install` fallbacks and the global vsce install from both e2e.yml
  and cd.yml. The native binaries are os/cpu constrained, so optionalDependencies
  (installed on linux, skipped elsewhere) keeps cross-platform installs working
  while removing the lockfile-bypassing fallback.
- deepnote: in ensureControllerSelectedForNotebook, return early with a warning
  when findNotebookEditor() yields no editor instead of passing a NotebookDocument
  to notebook.selectKernel, matching selectPlaceholderController. Passing the
  document can leave the controller unbound and silently drop the first execution.

Test guard (Dino):
- e2e: replace the re-click-every-5s runAndAwaitOutput loop with a single-run
  runOnceAndAwaitOutput so a dropped first execution fails the test instead of
  being masked by a later re-run. Make clickRunAll retry the find+click atomically
  to survive the StaleElementReferenceError toolbar race without re-running the
  notebook. Drop the now-unused RUN_ALL_REISSUE_INTERVAL/OUTPUT_TIMEOUT constants
  and add FIRST_RUN_OUTPUT_TIMEOUT.

Nitpicks:
- e2e: clean up the throwaway fixture temp dir in the suite's after() hook.
- e2e: extract INTERPRETER_PROMPT_TIMEOUT, OUTPUT_FRAME_SWITCH_TIMEOUT and
  SUITE_TIMEOUT named constants (consistent _000 formatting).

Verified: full project + e2e type-check, formatter, and the ExTester e2e suite
running under Xvfb (1 passing).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019VuoSZXF8YHsN81Lra45JW
@tkislan tkislan requested a review from dinohamzic June 30, 2026 19:51
@tkislan tkislan merged commit 735e5f5 into main Jul 1, 2026
14 checks passed
@tkislan tkislan deleted the tk/extester-e2e branch July 1, 2026 08:07
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.

3 participants