Skip to content

Conversation

@FilipPyrek
Copy link
Member

@FilipPyrek FilipPyrek commented Oct 30, 2025

Summary by CodeRabbit

  • Tests

    • Added a dedicated integration test job to CI.
    • Updated test configuration to use the new extension identifier for test runs.
  • Chores

    • Set 2-space indentation for workflow/action YAML files (other YAML unchanged).
    • Adjusted test environment uninstall commands to be non-automated (may prompt).
  • Refactor

    • Deferred loading of a notebook conversion module to reduce activation-time overhead.

@linear
Copy link

linear bot commented Oct 30, 2025

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 30, 2025

📝 Walkthrough

Walkthrough

EditorConfig: added a section for YAML files under .github/workflows and .github/actions (*.yml) setting indent_style = space and indent_size = 2, while leaving root-level YAML rules at indent_size = 4. CI: adds an "Integration Tests" job with a matrix strategy (os / python / pythonVersion / tags), 30m timeout, environment variables, steps to checkout, set up Node and Python (includes Python 3.12 in matrix), cache pip, install Node/Python deps, compile TypeScript, prepare a temporary user-data directory, run integration tests under xvfb-run (Windows path commented out), and upload VS Code logs and test results. Code: replaced top-level import of @deepnote/convert with dynamic await import(...) at three call sites in deepnoteExplorerView.ts. Tests: updated JVSC_EXTENSION_ID_FOR_TESTS from ms-toolsai.jupyter to Deepnote.vscode-deepnote in test constants. Test env script: removed automatic --yes from two pip uninstall commands.

Sequence Diagram(s)

sequenceDiagram
    participant CI as CI Workflow
    participant Job as Integration Tests Job
    participant Steps as Job Steps

    CI->>Job: Trigger Integration Tests job (matrix)
    activate Job
    Job->>Steps: Checkout repo
    Steps->>Steps: Setup Node.js
    Steps->>Steps: Setup Python (matrix)
    Steps->>Steps: Cache pip & install deps
    Steps->>Steps: npm ci & compile TypeScript
    rect rgba(102,187,106,0.12)
    Note over Steps: Run integration tests under Xvfb (Linux/macOS)
    Steps->>Steps: Prepare temp user-data dir
    Steps->>Steps: xvfb-run -> run tests
    end
    Steps->>Job: Upload logs & results
    deactivate Job
Loading
sequenceDiagram
    participant UI as Extension UI
    participant DE as deepnoteExplorerView
    participant C as @deepnote/convert

    UI->>DE: Request notebook import
    activate DE
    alt converter not loaded
        DE->>C: await import('@deepnote/convert')
        Note right of C: dynamic runtime load
    end
    DE->>DE: Convert notebook
    DE-->>UI: Return imported notebook
    deactivate DE
Loading

Possibly related PRs

Suggested reviewers

  • saltenasl
  • Artmann

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "feat: add integration tests to CI" directly and accurately reflects the primary change in this PR: the addition of a new "Integration Tests" job to the CI workflow in .github/workflows/ci.yml. While the changeset includes supporting modifications (EditorConfig updates, test constant changes, setup script adjustments), these are preparatory changes enabling the core objective. The title is specific, concise, and uses proper semantic versioning convention, making it clear to reviewers what the PR accomplishes.

📜 Recent 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 20b2d29 and 0d582ab.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml (1 hunks)
⏰ 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). (5)
  • GitHub Check: Integration Tests (ubuntu-latest, python, 3.12, ^[^@]+$|@mandatory|@kernelcore|@python|@jupyter)
  • GitHub Check: Integration Tests (ubuntu-latest, python, 3.12, @widgets)
  • GitHub Check: Integration Tests (ubuntu-latest, python, 3.12, @debugger)
  • GitHub Check: Integration Tests (ubuntu-latest, python, 3.12, @iw)
  • GitHub Check: Integration Tests (ubuntu-latest, python, 3.12, @webview|@export|@Lsp|@variableViewer)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)

302-310: Verify xvfb-action command format.

Line 305 passes ${{ env.xvfbCommand }} npm run test:integration to the run parameter. The xvfbCommand variable (--server-args=\"-screen 0 1024x768x24\") is prepended as a CLI argument before the npm command. Verify that xvfb-action accepts this format; typically the action handles xvfb server setup internally and expects only the target command in run. If custom xvfb args are needed, check if the action supports dedicated parameters (e.g., options or server-args) rather than embedding them in the command string.


324-324: Confirm upload-artifact version upgrade.

Lines 324 and 332 pin to actions/upload-artifact@330a01c490aca151604b8cf639adc76d48f6c5d4 #v5, while prior code review flagged unpinned @v4 actions in this job and recommended pinning to v4 commit hashes. Verify this v5 upgrade is intentional or revert to the v4 pins suggested in previous review (per the workflow's consistent use of pinned action versions).

Also applies to: 332-332


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

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 30, 2025
@FilipPyrek FilipPyrek force-pushed the fpyrek/grn-4766-add-basic-integration-tests-for-a-deepnote-file branch from 702e13e to 573d6b3 Compare October 30, 2025 12:18
@codecov
Copy link

codecov bot commented Oct 30, 2025

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
1633 2 1631 187
View the top 2 failed test(s) by shortest run time
Cancelling pending Internal Jupyter execution code should not interrupt the kernel::Code Execution Cancelling pending Internal Jupyter execution code should not interrupt the kernel
Stack Traces | 0s run time
Cannot read properties of null (reading 'catch')
Verify Kernel Shutdown::Kernel Api Verify Kernel Shutdown
Stack Traces | 0s run time
Access to Jupyter Kernel has been revoked

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

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

📜 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 702e13e and 573d6b3.

📒 Files selected for processing (2)
  • .editorconfig (1 hunks)
  • .github/workflows/ci.yml (1 hunks)
⏰ 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: copilot-setup-steps
🔇 Additional comments (1)
.editorconfig (1)

19-21: Good EditorConfig scope for workflow files.

The new YAML subsection correctly targets GitHub workflow and action files with appropriate 2-space indentation, which aligns with GitHub's conventions.

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 573d6b3 and 50a4122.

📒 Files selected for processing (4)
  • src/notebooks/deepnote/deepnoteExplorerView.ts (3 hunks)
  • src/test/constants.node.ts (1 hunks)
  • src/test/constants.ts (1 hunks)
  • src/test/datascience/setupTestEnvs.sh (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/test/constants.ts
  • src/notebooks/deepnote/deepnoteExplorerView.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/test/constants.ts
  • src/test/constants.node.ts
  • src/notebooks/deepnote/deepnoteExplorerView.ts
**/*.node.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use *.node.ts for Desktop-specific implementations that require full file system access and Python environments

Files:

  • src/test/constants.node.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteExplorerView.ts
🧬 Code graph analysis (2)
src/test/constants.ts (1)
src/test/constants.node.ts (1)
  • JVSC_EXTENSION_ID_FOR_TESTS (19-19)
src/test/constants.node.ts (1)
src/test/constants.ts (1)
  • JVSC_EXTENSION_ID_FOR_TESTS (4-4)
⏰ 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: Integration Tests
🔇 Additional comments (6)
src/test/constants.node.ts (1)

19-19: LGTM – consistent with constants.ts.

Both test constant files now use the same extension ID.

src/notebooks/deepnote/deepnoteExplorerView.ts (4)

4-5: LGTM – clear rationale.

The comment explains why dynamic imports are used instead of top-level imports.


361-365: LGTM – dynamic import defers module loading.

The dynamic import delays loading @deepnote/convert until needed, preventing activation failures in test environments where the package may be unresolvable. The surrounding catch block (lines 377-381) handles import failures.


434-438: LGTM – consistent pattern.

Same dynamic import approach as line 361, with error handling at lines 451-455.


1-11: Verification complete—no static imports of @deepnote/convert remain.

The codebase correctly uses only dynamic imports (lines 361 and 434 in deepnoteExplorerView.ts), matching the documented pattern. No other files contain static imports that would cause test environment activation failures.

src/test/constants.ts (1)

4-4: Extension ID cannot be verified in public marketplace; manual test verification required.

The referenced extension Deepnote.vscode-deepnote does not appear in search results for the VS Code Marketplace. Tests relying on JVSC_EXTENSION_ID_FOR_TESTS across 13+ files will attempt to load this extension, and failure to locate it will break test infrastructure.

Confirm:

  • Extension is published (or available in your test environment)
  • It exports compatible APIs (Jupyter, IExtensionApi)
  • All dependent test files pass with the new ID

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 50a4122 and 20b2d29.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml (1 hunks)
⏰ 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: Integration Tests
🔇 Additional comments (1)
.github/workflows/ci.yml (1)

193-292: Job structure and integration test setup look solid.

The new integration-tests job is well-structured with appropriate environment variables, Python 3.12 setup, screen capture dependencies, and xvfb-based headless execution. The caching, compilation, and artifact upload steps are sensible. Once action pinning is resolved, this should run reliably.

@FilipPyrek
Copy link
Member Author

Closing this PR because the existing integration tests are not working in microsoft/vscode-jupyter anyway. https://github.com/microsoft/vscode-jupyter/actions/runs/18935319908

@FilipPyrek FilipPyrek closed this Oct 30, 2025
@FilipPyrek FilipPyrek deleted the fpyrek/grn-4766-add-basic-integration-tests-for-a-deepnote-file branch October 30, 2025 17:17
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.

2 participants