test: add test case to reproduce eth_getBlockByNumber Promise serialization bug #1993
Conversation
…zation bug Reproduces issue #1972 where Promise objects in JSON-RPC response results get serialized to empty objects when using createServer with createMemoryClient and forking. The test verifies that eth_getBlockByNumber returns proper block data instead of serialized Promise objects. Co-authored-by: Will Cory <roninjin10@users.noreply.github.com>
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
WalkthroughAdded an integration test that calls eth_blockNumber then eth_getBlockByNumber and asserts the latter returns a resolved, non-empty block object (not a Promise), verifies key block fields and formats, and checks the JSON-RPC response shape. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
Claude finished @roninjin10's task —— View job Code Review Complete ✅I've reviewed the test case that reproduces the eth_getBlockByNumber Promise serialization bug. Here's my comprehensive feedback: Todo List:
📋 Code Review Feedback✅ Code Quality & Best PracticesThe new test case (lines 170-227) demonstrates excellent code quality:
✅ Test Coverage & StructureThe test effectively reproduces the reported issue:
✅ Security ConcernsNo security issues identified:
✅ Performance ConsiderationsThe test is well-optimized for a test environment:
🔍 Specific Technical ObservationsLines 210-213 - The core bug detection logic: // The bug was that result contained a Promise object which serializes to {}
expect(res.body.result).not.toEqual({})
expect(typeof res.body.result).toBe('object')
expect(res.body.result).not.toBeInstanceOf(Promise)This is exactly the right approach to detect Promise serialization issues. Lines 215-221 - Property validation: ✅ Bug Reproduction EffectivenessThis test case should effectively reproduce issue #1972 because:
📝 Minor Suggestions
🎯 Overall AssessmentThis is a well-written, focused test case that effectively reproduces the reported issue. The code quality is high, follows established patterns, and includes comprehensive assertions. Once the underlying bug is fixed, this test will serve as an excellent regression test to prevent the issue from reoccurring. Recommendation: ✅ Approve - This PR successfully adds the reproduction test case as intended. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server/src/createHttpHandler.spec.ts (1)
213-213: Redundant Promise instance check after JSON deserialization.The check
expect(res.body.result).not.toBeInstanceOf(Promise)will always pass becauseres.bodyis already parsed JSON. Promises serialize to{}and become plain objects afterJSON.parse(). The meaningful assertion is line 211 (expect(res.body.result).not.toEqual({})), which catches the actual bug where a Promise serializes to an empty object.Consider removing this redundant check:
expect(res.body.result).not.toEqual({}) expect(typeof res.body.result).toBe('object') -expect(res.body.result).not.toBeInstanceOf(Promise)Alternatively, add a comment explaining that this check documents the expected behavior even though it's not a runtime concern after deserialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/createHttpHandler.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,ts,jsx,tsx}: Formatting: Biome with tabs (2 spaces wide), 120 char line width, single quotes
Naming: camelCase for functions/variables, PascalCase for classes/types
Always add comments for hacks, workarounds, or tech debt.
Files:
packages/server/src/createHttpHandler.spec.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,ts}: Imports: Organized by Biome, use barrel files (index.js/ts) for exports
Error handling: Extend BaseError, include detailed diagnostics
Single file per item (e.g., createFoo.js for each item)
We strive for typesafety at all times
Files:
packages/server/src/createHttpHandler.spec.ts
**/*.{spec,test}.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{spec,test}.{ts,js}: Repetition is fine in test files. ts-ignore is fine in tests.
Make sure the tests especially snapshot tests are not dependent on file system e.g. absolute paths shouldn't be in a snapshot
For error/inline snapshots consider using error messages or patterns that don't include absolute paths
For mocked modules, use patterns that focus on the behavior, not path-specific information
Files:
packages/server/src/createHttpHandler.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: We always are explicit with function return types
We always explicitly type return types
Files:
packages/server/src/createHttpHandler.spec.ts
⏰ 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: WASM Bundle Size Check
- GitHub Check: CI Checks
- GitHub Check: Analyze (javascript-typescript)
|
@claude fix the lint ✔ │ @tevm/example-vite:lint:check │ 0s │ Remote Cache Hit │ |
|
Claude finished @roninjin10's task —— View job ✅ Fixed the lint issues successfully!Todo List:
Changes Made:
The lint issues have been resolved and the changes have been committed and pushed to the PR branch. The CI should now pass the linting checks. |
- Fix trailing whitespace in eth_getBlockByNumber test - Add timeout configuration for fork-based test (consistent with other tests) Co-authored-by: Will Cory <roninjin10@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/server/src/createHttpHandler.spec.ts (1)
227-227: Timeout configured correctly.The timeout configuration addresses the previous review concern about fork-based tests potentially hanging on slow networks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/createHttpHandler.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,ts,jsx,tsx}: Formatting: Biome with tabs (2 spaces wide), 120 char line width, single quotes
Naming: camelCase for functions/variables, PascalCase for classes/types
Always add comments for hacks, workarounds, or tech debt.
Files:
packages/server/src/createHttpHandler.spec.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,ts}: Imports: Organized by Biome, use barrel files (index.js/ts) for exports
Error handling: Extend BaseError, include detailed diagnostics
Single file per item (e.g., createFoo.js for each item)
We strive for typesafety at all times
Files:
packages/server/src/createHttpHandler.spec.ts
**/*.{spec,test}.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{spec,test}.{ts,js}: Repetition is fine in test files. ts-ignore is fine in tests.
Make sure the tests especially snapshot tests are not dependent on file system e.g. absolute paths shouldn't be in a snapshot
For error/inline snapshots consider using error messages or patterns that don't include absolute paths
For mocked modules, use patterns that focus on the behavior, not path-specific information
Files:
packages/server/src/createHttpHandler.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: We always are explicit with function return types
We always explicitly type return types
Files:
packages/server/src/createHttpHandler.spec.ts
🧠 Learnings (1)
📚 Learning: 2024-07-28T16:00:32.153Z
Learnt from: 0xNonCents
PR: evmts/tevm-monorepo#769
File: vm/vm/src/tevm.spec.ts:0-0
Timestamp: 2024-07-28T16:00:32.153Z
Learning: The user indicated that a test case was a duplicate and outdated. Upon checking, only one instance of the test case with the description "should create an http handler" was found in the `tevm.spec.ts` file.
Applied to files:
packages/server/src/createHttpHandler.spec.ts
⏰ 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: WASM Bundle Size Check
- GitHub Check: CI Checks
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/server/src/createHttpHandler.spec.ts (1)
170-227: Well-structured test reproducing the Promise serialization bug.The test comprehensively verifies that
eth_getBlockByNumberreturns properly resolved block data rather than serialized Promises. The sequential request pattern (eth_blockNumber followed by eth_getBlockByNumber) accurately reproduces the bug scenario described in issue #1972.
| expect(res.body.error).toBeUndefined() | ||
| expect(res.body.result).toBeDefined() | ||
|
|
||
| // The bug was that result contained a Promise object which serializes to {} | ||
| expect(res.body.result).not.toEqual({}) | ||
| expect(typeof res.body.result).toBe('object') | ||
| expect(res.body.result).not.toBeInstanceOf(Promise) |
There was a problem hiding this comment.
Redundant toBeInstanceOf(Promise) check on line 213.
The assertion expect(res.body.result).not.toBeInstanceOf(Promise) will never fail because res.body.result is the output of JSON.parse(), which cannot produce Promise instances. By the time the HTTP response is received and parsed, any Promise would have already been serialized. The checks on lines 211-212 (not empty object, is object type) are sufficient to verify the bug fix.
Consider removing the redundant assertion:
// The bug was that result contained a Promise object which serializes to {}
expect(res.body.result).not.toEqual({})
expect(typeof res.body.result).toBe('object')
- expect(res.body.result).not.toBeInstanceOf(Promise)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(res.body.error).toBeUndefined() | |
| expect(res.body.result).toBeDefined() | |
| // The bug was that result contained a Promise object which serializes to {} | |
| expect(res.body.result).not.toEqual({}) | |
| expect(typeof res.body.result).toBe('object') | |
| expect(res.body.result).not.toBeInstanceOf(Promise) | |
| expect(res.body.error).toBeUndefined() | |
| expect(res.body.result).toBeDefined() | |
| // The bug was that result contained a Promise object which serializes to {} | |
| expect(res.body.result).not.toEqual({}) | |
| expect(typeof res.body.result).toBe('object') |
🤖 Prompt for AI Agents
In packages/server/src/createHttpHandler.spec.ts around lines 207 to 213, the
assertion expect(res.body.result).not.toBeInstanceOf(Promise) is redundant
because res.body is parsed JSON and cannot be a Promise; remove that line (the
final assertion) so the test keeps the meaningful checks (result is defined, not
an empty object, and typeof is 'object') and update any test comments if needed.
|
AUTOFIX |
Reproduces issue #1972 where Promise objects in JSON-RPC response results get serialized to empty objects when using createServer with createMemoryClient and forking.
The test verifies that eth_getBlockByNumber returns proper block data instead of serialized Promise objects.
Closes #1972
Generated with Claude Code
Summary by CodeRabbit