-
Notifications
You must be signed in to change notification settings - Fork 0
test: Added test for sql queries made in storage.ts in redeem-solver with correct Miniflare Setup #224
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
base: master
Are you sure you want to change the base?
Conversation
…package following the btcindexer approach. Setup the miniflare for redeem-solver with only the D1 db since it is the only one used across the package Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Reviewer's GuideAdds a Bun+Miniflare-based test suite for D1Storage SQL behavior in redeem_solver and wires the package test script to run Bun tests using the shared btcindexer migration schema. Sequence diagram for D1Storage markRedeemProposed Bun+Miniflare testsequenceDiagram
actor Developer
participant BunTestRunner
participant Miniflare
participant D1Database
participant D1Storage
Developer->>BunTestRunner: run bun test
BunTestRunner->>Miniflare: beforeAll create Miniflare instance
Miniflare-->>BunTestRunner: Miniflare with D1 DB DB
BunTestRunner->>Miniflare: beforeEach getD1Database DB
Miniflare-->>BunTestRunner: D1Database
BunTestRunner->>D1Database: initDb(db) apply migrations
BunTestRunner->>Miniflare: getD1Database DB
Miniflare-->>BunTestRunner: D1Database
BunTestRunner->>D1Storage: new D1Storage(db)
BunTestRunner->>D1Database: INSERT nbtc_redeem_requests row
BunTestRunner->>D1Database: INSERT nbtc_utxos row
BunTestRunner->>D1Storage: markRedeemProposed(redeem1, [utxo1], lockTimeMs)
D1Storage->>D1Database: UPDATE nbtc_redeem_requests SET status Proposed
D1Storage->>D1Database: UPDATE nbtc_utxos SET status Locked, locked_until
D1Storage-->>BunTestRunner: Promise resolved
BunTestRunner->>D1Database: SELECT status FROM nbtc_redeem_requests
D1Database-->>BunTestRunner: status Proposed
BunTestRunner->>D1Database: SELECT status, locked_until FROM nbtc_utxos
D1Database-->>BunTestRunner: status Locked, locked_until
BunTestRunner-->>Developer: test assertion results
BunTestRunner->>Miniflare: afterEach getD1Database DB
Miniflare-->>BunTestRunner: D1Database
BunTestRunner->>D1Database: DROP TABLE IF EXISTS for all tables
BunTestRunner->>Miniflare: afterAll dispose
Miniflare-->>BunTestRunner: resources cleaned up
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- The tests rely on
Date.now()for assertions (e.g., comparinglocked_untiland filtering redeems), which can introduce flakiness over time; consider injecting a fixed timestamp or mocking the clock so the expectations are deterministic. - In
initDb, the regex-based stripping of--comments and newlines from migration SQL risks altering valid SQL (e.g.,--inside string literals or needed line breaks); it would be safer to only strip line-leading--comments and execute statements as-is or split on;instead of flattening everything into one line.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests rely on `Date.now()` for assertions (e.g., comparing `locked_until` and filtering redeems), which can introduce flakiness over time; consider injecting a fixed timestamp or mocking the clock so the expectations are deterministic.
- In `initDb`, the regex-based stripping of `--` comments and newlines from migration SQL risks altering valid SQL (e.g., `--` inside string literals or needed line breaks); it would be safer to only strip line-leading `--` comments and execute statements as-is or split on `;` instead of flattening everything into one line.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
sczembor
left a 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.
we should avoid repeating code, lets move some of this code to common functions
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
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.
Pull request overview
This PR introduces comprehensive test coverage for the redeem_solver storage layer using Miniflare-backed D1 databases and the Bun test runner. The changes extract common database initialization logic into a shared library to promote code reuse across packages.
Key Changes:
- Added integration-style tests for D1Storage methods including UTXO selection, redeem status transitions, and network queries
- Extracted shared database test initialization logic to
@gonative-cc/lib/common-setup/db.testfor reusability - Exported the UTXO_LOCK_TIME_MS constant for use in tests
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/redeem_solver/src/storage.ts | Exports UTXO_LOCK_TIME_MS constant (2 minutes) for test usage |
| packages/redeem_solver/src/storage.test.ts | Comprehensive test suite for D1Storage covering all storage methods with proper Miniflare setup |
| packages/redeem_solver/src/db.test.ts | Wrapper that configures shared initDb function with btcindexer migrations path |
| packages/redeem_solver/package.json | Updates test script from placeholder to actual Bun test execution |
| packages/lib/src/common-setup/db.test.ts | Shared database initialization logic that reads and applies SQL migrations |
| packages/lib/package.json | Adds export path for common-setup modules |
| packages/btcindexer/src/db.test.ts | Refactored to use shared initDb function, removing duplicate code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .bind("utxo1") | ||
| .first<{ status: string; locked_until: number }>(); | ||
| expect(utxo!.status).toBe(UtxoStatus.Locked); | ||
| expect(utxo!.locked_until).toBeGreaterThan(Date.now()); |
Copilot
AI
Dec 23, 2025
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.
Potential flaky test due to timing. The assertion expects locked_until to be greater than Date.now(), but there's a race condition if the test executes slowly. Between the markRedeemProposed call and the assertion, time passes. Consider using a more specific assertion that checks locked_until is approximately equal to the expected value (current time + UTXO_LOCK_TIME_MS) with a tolerance window.
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.
I mean these tests focuses more on just verifying the sql queries
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.
then lets just assert if its defined
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.
okay done
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
sczembor
left a 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.
lets rename it to something more meaningful than common-setup
| let storage: D1Storage; | ||
| let db: D1Database; | ||
|
|
||
| async function insertRedeemRequest( |
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.
dont we have those functions in storage.ts
| } | ||
|
|
||
| async function insertUtxo( | ||
| utxoId: number, |
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.
lets reuse what we already have in the storage
| .bind("utxo1") | ||
| .first<{ status: string; locked_until: number }>(); | ||
| expect(utxo!.status).toBe(UtxoStatus.Locked); | ||
| expect(utxo!.locked_until).toBeGreaterThan(Date.now()); |
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.
then lets just assert if its defined
Description
Closes: #241
Summary by Sourcery
Add Bun-based test setup for redeem_solver storage layer using Miniflare-backed D1 database and shared migrations.
Build:
Tests: