-
Notifications
You must be signed in to change notification settings - Fork 0
test: test coverage for core transformation functions #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
⚙️ Control Options:
|
📝 WalkthroughWalkthroughThis PR adds and updates test suites and adjusts configs/source handling: new unit tests for convertDeepnoteBlockToJupyterCell, convertDeepnoteBlockTypeToJupyter, transformDeepnoteYamlToNotebookContent, and a rewritten NotebookPicker test file. Tests cover many Deepnote block types and mappings (code/sql/visualization/input/markdown/text-cell variants), output handling (multiple outputs, truncated flags), nbformat/kernel metadata population, multi-notebook and empty-notebook cases, deserialization error propagation, and non-mutation of inputs. Code changes include improved promise/error handling and null-checks in NotebookPicker, stricter error and JSON handling in requestAPI, jest transform allowlist additions, and stricter TypeScript ESLint rules in package.json. No exported/public API signatures were changed. Sequence Diagram(s)sequenceDiagram
autonumber
participant Test
participant Transformer as transformDeepnoteYamlToNotebookContent
participant Deserializer as deserializeDeepnoteFile
participant Converter as convertDeepnoteBlockToJupyterCell
Note right of Transformer #f0f8ff: Top-level YAML -> NotebookContent
Test->>Transformer: call(YAML)
Transformer->>Deserializer: deserializeDeepnoteFile(YAML)
alt deserialization fails
Deserializer-->>Transformer: throw Error
Transformer-->>Test: propagate Error
else deserialization succeeds
Deserializer-->>Transformer: DeepnoteFile(notebooks[])
loop for each notebook
loop for each block
Transformer->>Converter: convertDeepnoteBlockToJupyterCell(block)
Converter-->>Transformer: Jupyter cell(s) (type, source, outputs, metadata)
end
Transformer->>Transformer: assemble cells + metadata (kernelspec, language_info, deepnote.notebooks)
end
Transformer-->>Test: NotebookContent(nbformat, nbformat_minor, cells, metadata)
end
sequenceDiagram
autonumber
participant Test
participant Mapper as convertDeepnoteBlockTypeToJupyter
Note right of Mapper #e6f7ff: Maps Deepnote block.type -> "code" | "markdown"
Test->>Mapper: call(block.type)
alt type ∈ {code, sql, visualization, notebook-function, big-number}
Mapper-->>Test: "code"
else type ∈ {input-text, input-checkbox, input-textarea, input-file, input-select, input-date-range, input-date, input-slider}
Mapper-->>Test: "code"
else type ∈ {markdown, image, button, separator, text-cell-*}
Mapper-->>Test: "markdown"
else unknown or empty
Mapper-->>Test: "markdown"
end
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (3 passed)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
===========================================
+ Coverage 52.56% 69.56% +16.99%
===========================================
Files 13 13
Lines 253 253
Branches 28 28
===========================================
+ Hits 133 176 +43
+ Misses 116 73 -43
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts(1 hunks)src/__tests__/convert-deepnote-block-type-to-jupyter.spec.ts(1 hunks)src/__tests__/transform-deepnote-yaml-to-notebook-content.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T13:43:36.364Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T13:43:36.364Z
Learning: Applies to packages/*/src/**/*.test.{ts,tsx} : Follow existing test patterns (see packages/blocks/src/blocks/*.test.ts); write clear names, group with describe(), cover edge cases, errors, special characters, and verify exact output for code-generation functions
Applied to files:
src/__tests__/convert-deepnote-block-type-to-jupyter.spec.tssrc/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts
🧬 Code graph analysis (2)
src/__tests__/convert-deepnote-block-type-to-jupyter.spec.ts (1)
src/convert-deepnote-block-type-to-jupyter.ts (1)
convertDeepnoteBlockTypeToJupyter(1-32)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts (1)
src/convert-deepnote-block-to-jupyter-cell.ts (1)
convertDeepnoteBlockToJupyterCell(10-44)
🔇 Additional comments (6)
src/__tests__/convert-deepnote-block-type-to-jupyter.spec.ts (2)
6-144: Good coverage and structure.Covers all current mappings, unknowns, and empty input. Nice separation of suites.
8-29: Reduce duplication with table-driven tests.Consider
test.eachto compress repetitive cases and ease future additions.Example:
+describe('maps to code', () => { + test.each([ + 'code','sql','notebook-function','big-number','visualization', + 'input-text','input-checkbox','input-textarea','input-file', + 'input-select','input-date-range','input-date','input-slider' + ])('%s -> code', (t) => { + expect(convertDeepnoteBlockTypeToJupyter(t)).toBe('code'); + }); +});Based on learnings
Also applies to: 30-68, 72-131, 133-143 <details> <summary>⛔ Skipped due to learnings</summary>Learnt from: CR
PR: deepnote/deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T13:43:36.364Z
Learning: Applies to packages//src/**/.test.{ts,tsx} : Follow existing test patterns (see packages/blocks/src/blocks/*.test.ts); write clear names, group with describe(), cover edge cases, errors, special characters, and verify exact output for code-generation functionsLearnt from: CR
PR: deepnote/deepnote-internal#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-10-22T11:56:07.963Z
Learning: Applies to **/*.{spec,test}.{ts,tsx,js,jsx} : Avoid defining test-specific values outside individual tests; define within each test for clarityLearnt from: CR
PR: deepnote/deepnote#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-08-18T23:41:22.727Z
Learning: Applies to **/*.{spec,e2e}.{ts,tsx} : Avoid defining test-specific values outside individual test casesLearnt from: CR
PR: deepnote/deepnote-internal#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-10-22T11:56:07.963Z
Learning: Applies to **/*.{spec,test}.{ts,tsx,js,jsx} : Only specify data relevant to the tested requirement inside a single test; move general setup to factories/beforeEach</details> </blockquote></details> <details> <summary>src/__tests__/transform-deepnote-yaml-to-notebook-content.spec.ts (2)</summary><blockquote> `1-310`: **Overall: solid, thorough tests.** Great coverage across success and failure paths; metadata and structure checks are on point. --- `7-9`: **Remove the ESM mock suggestion—project uses CommonJS, not ESM.** The codebase doesn't use ESM (package.json lacks `"type": "module"` and jest.config.js has no ESM extensions). Jest.mock() doesn't need `__esModule: true` for CommonJS module mocks with named exports. The suggestion defends against a non-existent runtime. > Likely an incorrect or invalid review comment. </blockquote></details> <details> <summary>src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts (2)</summary><blockquote> `7-11`: **ESM-safe mocks.** If the repo runs Jest in ESM mode, include `__esModule: true` in mock factories to avoid interop quirks. ```diff -jest.mock('@deepnote/blocks', () => ({ +jest.mock('@deepnote/blocks', () => ({ + __esModule: true, createPythonCode: jest.fn((block: any) => block.source || 'print("test")'), createMarkdown: jest.fn((block: any) => block.source || '# Test') })); -jest.mock('../convert-deepnote-block-type-to-jupyter', () => ({ +jest.mock('../convert-deepnote-block-type-to-jupyter', () => ({ + __esModule: true, convertDeepnoteBlockTypeToJupyter: jest.fn((type: string) => { // ... }) }));Also applies to: 12-35
⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/deepnote-internal#0 File: .cursor/rules/tests.mdc:0-0 Timestamp: 2025-10-22T11:56:53.582Z Learning: Applies to {**/*.{test,spec}.{jsx,tsx},**/__tests__/**/*.{jsx,tsx}} : Only use jest.mock() for external modules in React tests; do not mock in-repo React hooks, context providers, or GraphQL operationsLearnt from: CR PR: deepnote/deepnote-internal#0 File: CONVENTIONS.md:0-0 Timestamp: 2025-10-22T11:56:07.963Z Learning: Applies to **/*.{spec,test}.{tsx,jsx} : Avoid excessive mocking in React component tests; use TestProvider for context; prefer apolloMocks and flags props over jest.mock for internal hooks/contextsLearnt from: CR PR: deepnote/deepnote-internal#0 File: .cursor/rules/tests.mdc:0-0 Timestamp: 2025-10-22T11:56:53.582Z Learning: Applies to {**/*.{test,spec}.{jsx,tsx},**/__tests__/**/*.{jsx,tsx}} : Avoid excessive mocking in React component tests
5-5: False alarm—no runtime collision risk.
DeepnoteBlockis used only as a type annotation (never as a value). TypeScript's default compiler settings automatically elide unused imports. The import will not reach runtime, sojest.mock()presents no collision. The code is correct.Likely an incorrect or invalid review comment.
There was a problem hiding this 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
📒 Files selected for processing (2)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts(1 hunks)src/__tests__/transform-deepnote-yaml-to-notebook-content.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T13:43:36.364Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-14T13:43:36.364Z
Learning: Applies to packages/*/src/**/*.test.{ts,tsx} : Follow existing test patterns (see packages/blocks/src/blocks/*.test.ts); write clear names, group with describe(), cover edge cases, errors, special characters, and verify exact output for code-generation functions
Applied to files:
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts
🧬 Code graph analysis (2)
src/__tests__/transform-deepnote-yaml-to-notebook-content.spec.ts (2)
src/transform-deepnote-yaml-to-notebook-content.ts (1)
transformDeepnoteYamlToNotebookContent(7-58)src/convert-deepnote-block-to-jupyter-cell.ts (1)
convertDeepnoteBlockToJupyterCell(10-44)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts (1)
src/convert-deepnote-block-to-jupyter-cell.ts (1)
convertDeepnoteBlockToJupyterCell(10-44)
⏰ 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). (4)
- GitHub Check: check_release
- GitHub Check: build
- GitHub Check: Audit - Production
- GitHub Check: Lint & Format
🔇 Additional comments (11)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts (3)
189-250: Solid markdown cell tests.Covers basic markdown, text-cell variants, image blocks, and metadata handling. Creator assertions added per past review (lines 204-208).
252-304: Good coverage of special block types.Efficiently tests SQL, visualization, and all 8 input type variants converting to code cells.
12-35: Mock type list is currently synchronized with the implementation.Verification confirms the mock contains all 13 types that map to 'code' in the actual implementation. However, the hardcoded duplication creates a maintenance risk: any future changes to the actual type mappings require manual updates here. Consider extracting the type list to a shared constant or document this synchronization requirement in a code comment.
src/__tests__/transform-deepnote-yaml-to-notebook-content.spec.ts (8)
7-21: Mock setup is reasonable for unit tests.The simplified mock for
convertDeepnoteBlockToJupyterCellhandles code/markdown distinction appropriately. Per PR objectives, verify against actual behavior during manual testing.
28-65: Solid foundation test for basic transformation.Verifies cell conversion, metadata structure, and nbformat version.
67-113: Verifies all notebooks in metadata.Correctly ensures both notebooks are indexed by name with proper IDs.
115-172: Correctly verifies first-notebook-only for primary cells.Lines 155-171 properly assert converter called 3 times: twice for metadata (both notebooks), once more for primary cells (first notebook). Addresses past review concern.
174-210: Good edge case coverage for empty scenarios.Correctly handles empty notebooks (0 cells) and missing notebooks (fallback message).
212-233: Verifies kernel metadata presence.Ensures Python kernel metadata included in output.
235-243: Error handling verified.Ensures deserialization failures are properly caught and re-thrown with context.
245-327: Structure and ordering tests thorough.Verifies notebook metadata includes cell details (245-277) and that mixed block types maintain order (279-327).
Added comprehensive test suites for core transformation functions: - convert-deepnote-block-to-jupyter-cell.spec.ts (13 tests) - transform-deepnote-yaml-to-notebook-content.spec.ts (11 tests) Overall coverage improved from 31.03% to 57.24% Both transformation files now have 100% line coverage
* Add ESLint rules for type safety and fix violations - Add @typescript-eslint/no-floating-promises: error - Add @typescript-eslint/no-non-null-assertion: error - Add @typescript-eslint/prefer-nullish-coalescing: error - Change @typescript-eslint/no-explicit-any from off to error Fixes: - Fix floating promise in NotebookPicker constructor by adding .catch() - Fix non-null assertion in NotebookPicker.onAfterAttach with null check - Fix prefer-nullish-coalescing in handler.ts by using ?? instead of || - Add inline eslint-disable comments for legitimate any usage in handler.ts * Add test for error handling in NotebookPicker constructor Improves test coverage for the .catch() error handler added to handle promise rejections in the constructor. * Add comprehensive test coverage for NotebookPicker - Add test for null model in handleChange - Add test for invalid metadata in handleChange - Add test for onAfterAttach without parent - Import Message type for test Coverage improved from 84.09% to 97.72% for NotebookPicker.tsx * Merge test coverage from PR #45 Added comprehensive test suites for core transformation functions: - convert-deepnote-block-to-jupyter-cell.spec.ts (13 tests) - transform-deepnote-yaml-to-notebook-content.spec.ts (11 tests) Overall coverage improved from 31.03% to 57.24% Both transformation files now have 100% line coverage * fix: remove mocks, test real implementation * chore: format * chore: remove out of scope tests * chore: remove out of scope test * refactor: fix test and format * chore: remove out of scope test * fix: address violations * refactor: improve test * refactor: TypeError * fix: drop unnecessary property * refactor: simplify promise * chore: improve "dirty" test --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: dinohamzic <dino@subtlebits.com>
There was a problem hiding this 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
📒 Files selected for processing (1)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts (1)
src/convert-deepnote-block-to-jupyter-cell.ts (1)
convertDeepnoteBlockToJupyterCell(10-44)
⏰ 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). (3)
- GitHub Check: check_release
- GitHub Check: build
- GitHub Check: Lint & Format
🔇 Additional comments (3)
src/__tests__/convert-deepnote-block-to-jupyter-cell.spec.ts (3)
1-7: Verify mock setup is configured.No
jest.mock()for@deepnote/blocksis visible in this file. Past review comments request assertions aboutcreatePythonCodeandcreateMarkdowninvocation, but the current tests don't contain such assertions. Either mocks are configured in a shared test setup file, or creator invocation verification needs to be added.Can you confirm whether
jest.mock('@deepnote/blocks')is configured in a test setup file (e.g.,jest.config.ts,setupTests.ts, or similar)? If mocks are defined there, please share that context so I can verify they align with how the tests use them.
167-226: Inconsistency: Past review requestedcreateMarkdowninvocation assertions.Past review comment (on markdown cells) explicitly requested: "Mirror the code-cell expectation for
createMarkdown" and proposed adding mock assertions likeexpect(createMarkdown).toHaveBeenCalledTimes(1). The current tests do not include these assertions.Clarify: Were creator invocation assertions intentionally deferred, or should they be added here? If they should be added, I can generate the required test updates.
9-25: Verify mock behavior aligns with implementation.The test expects
result.sourceto equal'print("hello")', implyingcreatePythonCode(block)returns the block's content directly. Past review comments requested asserting thatcreatePythonCodeis called with a cloned (not original) block.If mocks are set up elsewhere, ensure they reflect the actual behavior: that the function receives a deep clone and that the returned source string matches expectations across all code-cell tests.
|
@Artmann @saltenasl this is now ready for review. |
- Add tests for convert-deepnote-block-type-to-jupyter.ts (100% coverage) - Add tests for convert-deepnote-block-to-jupyter-cell.ts (100% coverage) - Add tests for transform-deepnote-yaml-to-notebook-content.ts (100% coverage) This increases overall test coverage from 26.57% to 55.94%, providing comprehensive testing for the core Deepnote-to-Jupyter transformation logic.
- Add assertions to verify createPythonCode is called correctly in code cell tests - Add assertions to verify createMarkdown is called correctly in markdown cell tests - Add assertions to verify convertDeepnoteBlockToJupyterCell is called for all notebooks in transform tests - Verify mock call counts and arguments to strengthen test contracts
* Add ESLint rules for type safety and fix violations - Add @typescript-eslint/no-floating-promises: error - Add @typescript-eslint/no-non-null-assertion: error - Add @typescript-eslint/prefer-nullish-coalescing: error - Change @typescript-eslint/no-explicit-any from off to error Fixes: - Fix floating promise in NotebookPicker constructor by adding .catch() - Fix non-null assertion in NotebookPicker.onAfterAttach with null check - Fix prefer-nullish-coalescing in handler.ts by using ?? instead of || - Add inline eslint-disable comments for legitimate any usage in handler.ts * Add test for error handling in NotebookPicker constructor Improves test coverage for the .catch() error handler added to handle promise rejections in the constructor. * Add comprehensive test coverage for NotebookPicker - Add test for null model in handleChange - Add test for invalid metadata in handleChange - Add test for onAfterAttach without parent - Import Message type for test Coverage improved from 84.09% to 97.72% for NotebookPicker.tsx * Merge test coverage from PR #45 Added comprehensive test suites for core transformation functions: - convert-deepnote-block-to-jupyter-cell.spec.ts (13 tests) - transform-deepnote-yaml-to-notebook-content.spec.ts (11 tests) Overall coverage improved from 31.03% to 57.24% Both transformation files now have 100% line coverage * fix: remove mocks, test real implementation * chore: format * chore: remove out of scope tests * chore: remove out of scope test * refactor: fix test and format * chore: remove out of scope test * fix: address violations * refactor: improve test * refactor: TypeError * fix: drop unnecessary property * refactor: simplify promise * chore: improve "dirty" test --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: dinohamzic <dino@subtlebits.com>
e0f87d9 to
9f0360d
Compare
4a5bd7e
|
@coderabbitai approve |
✅ Actions performedComments resolved and changes approved. |
Add comprehensive test coverage for core transformation functions
Summary
Added test suites for three critical transformation functions in the jupyterlab-deepnote extension, improving overall test coverage from 26.57% to 55.94%. The newly tested functions handle the core logic for converting Deepnote files to Jupyter notebook format.
Files added:
convert-deepnote-block-type-to-jupyter.spec.ts- 30 tests covering all block type mappings (code, markdown, SQL, visualizations, input types, etc.)convert-deepnote-block-to-jupyter-cell.spec.ts- 13 tests for block-to-cell conversion including output handling and metadata preservationtransform-deepnote-yaml-to-notebook-content.spec.ts- 11 tests for the main YAML transformation pipelineAll three files now have 100% line coverage.
Review & Testing Checklist for Human
@deepnote/blocksfunctions (createPythonCode,createMarkdown,deserializeDeepnoteFile). Ensure the mocked behavior matches the actual library functions, especially for edge cases..deepnotefile in JupyterLab locally to verify the transformation still works correctly with actual data. The tests are unit-level only and don't exercise the full integration.as any[]and!operators were added to handle TypeScript strict checking in tests. While necessary for the test infrastructure, verify these don't indicate actual type safety issues in the production code.Test Plan
jlpm testto verify all 54 tests passjupyter lab --debugexample.deepnotefile (or create a test .deepnote file)Notes
@deepnote/blocksis a proprietary package with limited testabilityRequested by: James Hobbs (james@deepnote.com) (@jamesbhobbs)
Session: https://app.devin.ai/sessions/13b72387222a4a1a9322d6dbd266d01c
Summary by CodeRabbit