Skip to content

Conversation

@tkislan
Copy link
Member

@tkislan tkislan commented Nov 3, 2025

Summary by CodeRabbit

  • Refactor

    • Enhanced method accessibility for notebook kernel selection functionality.
  • Tests

    • Expanded test coverage for kernel initialization, environment selection, and configuration validation scenarios.

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 3, 2025

📝 Walkthrough

Walkthrough

Makes the onKernelStarted method public (previously private) in DeepnoteKernelAutoSelector and adds comprehensive unit tests. Tests cover environment selection, kernel startup handling for non-deepnote notebooks, and three scenarios for ensureKernelSelected: missing environment assignments, missing environments in the manager, and successful environment configuration with proper argument wiring.

Possibly related PRs

Suggested reviewers

  • Artmann

Pre-merge checks

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title accurately describes the main change: adding test coverage for DeepnoteKernelAutoSelector, which aligns with the substantial test file modifications.

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

@codecov
Copy link

codecov bot commented Nov 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73%. Comparing base (ed71529) to head (8ae52cc).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #168   +/-   ##
=====================================
  Coverage     73%     73%           
=====================================
  Files        574     574           
  Lines      46684   46751   +67     
  Branches    5513    5513           
=====================================
+ Hits       34215   34306   +91     
+ Misses     10657   10628   -29     
- Partials    1812    1817    +5     
Files with missing lines Coverage Δ
...ebooks/deepnote/deepnoteKernelAutoSelector.node.ts 28% <ø> (+7%) ⬆️
...pnote/deepnoteKernelAutoSelector.node.unit.test.ts 89% <100%> (+3%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ed71529 and 8ae52cc.

📒 Files selected for processing (2)
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1 hunks)
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{test,spec}.ts

📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)

In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined then property to avoid hanging tests

Files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts (2)
src/test/vscode-mock.ts (2)
  • resetVSCodeMocks (60-79)
  • mockedVSCodeNamespaces (17-17)
src/kernels/types.ts (1)
  • IKernel (439-446)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)
src/kernels/types.ts (1)
  • IKernel (439-446)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build & Test
🔇 Additional comments (4)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)

323-323: LGTM: Visibility change enables testing.

Making onKernelStarted public allows direct invocation in unit tests without altering behavior.

src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts (3)

3-3: LGTM: Test infrastructure setup.

Added imports and mock reset support new test suites appropriately.

Also applies to: 24-24, 52-52


307-325: LGTM: Early return path verified.

Test correctly ensures initNotebookRunner isn't called for non-deepnote notebooks using verify().never().


327-441: LGTM: Comprehensive coverage of ensureKernelSelected scenarios.

Three test cases thoroughly validate:

  1. Missing environment assignment → returns false
  2. Missing environment in manager → returns false, removes mapping
  3. Environment found → returns true, validates all arguments passed to ensureKernelSelectedWithConfiguration

Particularly strong argument verification in the third test case.

@tkislan tkislan marked this pull request as ready for review November 3, 2025 20:41
@tkislan tkislan requested a review from a team as a code owner November 3, 2025 20:41
when(mockEnvironmentManager.waitForInitialization()).thenResolve();
when(mockEnvironmentManager.listEnvironments()).thenReturn(environments);

// Mock window.showQuickPick to simulate user selecting the first environment
Copy link
Member

Choose a reason for hiding this comment

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

nit - these comments are overly verbose

@saltenasl saltenasl merged commit 333a169 into main Nov 3, 2025
13 checks passed
@saltenasl saltenasl deleted the tk/add-deepnote-kernel-auto-selector-tests branch November 3, 2025 20:54
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