feat: unify db helper test functions#318
Merged
robert-zaremba merged 11 commits intomasterfrom Jan 23, 2026
Merged
Conversation
Signed-off-by: Robert Zaremba <robert@zaremba.ch>
Contributor
Reviewer's GuideUnifies database test helper logic across btcindexer and sui-indexer by centralizing migration application and table cleanup in a shared lib helper, updating tests to use it, and aligning schema/docs with current tables (removing nbtc_withdrawal). Entity relationship diagram for updated nBTC test database schemaerDiagram
nbtc_minting {
TEXT id
TEXT address_id
INTEGER status
INTEGER created_at
}
nbtc_redeem_requests {
TEXT id
TEXT sender
INTEGER amount
TEXT recipient
INTEGER created_at
INTEGER status
}
setups {
INTEGER id
TEXT config
}
btc_blocks {
INTEGER height
TEXT hash
INTEGER created_at
}
nbtc_deposit_addresses {
INTEGER id
INTEGER setup_id
TEXT address
}
nbtc_withdrawal {
TEXT sui_tx_id
TEXT sender
INTEGER amount
TEXT recipient
TEXT note
INTEGER sent_at
TEXT btc_tx_id
INTEGER status
}
nbtc_deposit_addresses ||--o{ nbtc_minting : records_deposits_for
setups ||--o{ nbtc_deposit_addresses : provides_config_for
nbtc_withdrawal ||--|| nbtc_redeem_requests : replaced_by_in_schema
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
init_db.ts,initDbis declaredasyncbut doesn'tawaitor return theapplyMigrationspromise, so callers may proceed before migrations complete; considerreturn applyMigrations(db, MIGRATIONS_PATH);. - In
btcindexer.test.tsthe per-test DB cleanup logic was removed butsuite.cleanupDB()fromsetupTestIndexerSuiteisn’t called inafterEach, which may allow state to leak between tests; consider invoking the shared cleanup there.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `init_db.ts`, `initDb` is declared `async` but doesn't `await` or return the `applyMigrations` promise, so callers may proceed before migrations complete; consider `return applyMigrations(db, MIGRATIONS_PATH);`.
- In `btcindexer.test.ts` the per-test DB cleanup logic was removed but `suite.cleanupDB()` from `setupTestIndexerSuite` isn’t called in `afterEach`, which may allow state to leak between tests; consider invoking the shared cleanup there.
## Individual Comments
### Comment 1
<location> `packages/lib/src/test-helpers/init_db.ts:40-41` </location>
<code_context>
-
-const MIGRATIONS_PATH = path.resolve(__dirname, "../db/migrations");
-
-export async function initDb(db: D1Database) {
- await applyMigrations(db, MIGRATIONS_PATH);
-}
</code_context>
<issue_to_address>
**issue (bug_risk):** initDb does not await applyMigrations, which can make DB-dependent tests flaky
Because initDb is async but doesn’t return/await applyMigrations, callers that do `await initDb(db)` can continue before migrations finish, leading to flaky DB tests. Please return the promise (`return applyMigrations(db, MIGRATIONS_PATH);`) or explicitly `await` it so the schema is guaranteed to be ready before use.
</issue_to_address>
### Comment 2
<location> `packages/btcindexer/README.md:84` </location>
<code_context>
-The state is stored in the `nbtc_minting` and `nbtc_withdrawal` tables in the SQL (D1) database.
+The state is stored in the `nbtc_minting` and `nbtc_redeem_requests` tables in the SQL (D1) database.
Bitcoin transaction can have one of the following status:
</code_context>
<issue_to_address>
**suggestion (typo):** Consider pluralizing "transaction" and "status" for correct grammar.
For example: "Bitcoin transactions can have one of the following statuses:".
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request consolidates duplicate database helper test functions from individual packages into a shared library location. The PR removes the db.test.ts files from btcindexer and sui-indexer packages and centralizes initDb and dropTables functions in @gonative-cc/lib/test-helpers/init_db. Additionally, the PR removes the obsolete nbtc_withdrawal table from the database schema and updates related documentation.
Changes:
- Centralized database test helper functions (
initDb,dropTables) into@gonative-cc/lib/test-helpers/init_db - Removed duplicate
db.test.tsfiles frombtcindexerandsui-indexerpackages - Removed obsolete
nbtc_withdrawaltable from database migration and updated README documentation
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/lib/src/test-helpers/init_db.ts |
Added centralized initDb and dropTables functions with hardcoded migration path and table list |
packages/sui-indexer/src/db.test.ts |
Removed - functionality moved to centralized location |
packages/btcindexer/src/db.test.ts |
Removed - functionality moved to centralized location |
packages/sui-indexer/src/storage.test.ts |
Updated imports and simplified afterEach to use centralized dropTables |
packages/btcindexer/src/storage.test.ts |
Updated imports and simplified afterEach to use centralized dropTables |
packages/btcindexer/src/btcindexer.test.ts |
Removed table dropping code from afterEach but missing call to cleanup function |
packages/btcindexer/src/btcindexer.helpers.test.ts |
Added cleanupDB helper function to test interface that wraps dropTables |
packages/btcindexer/db/migrations/0001_initial_schema.sql |
Removed obsolete nbtc_withdrawal table definition and related comments |
packages/btcindexer/README.md |
Updated documentation to reference nbtc_redeem_requests instead of nbtc_withdrawal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Robert Zaremba <robert@zaremba.ch>
Signed-off-by: Robert Zaremba <robert@zaremba.ch>
Signed-off-by: Robert Zaremba <robert@zaremba.ch>
robert-zaremba
commented
Jan 23, 2026
| await initDb(db); | ||
|
|
||
| storage = new D1Storage(db); | ||
| indexerStorage = new D1Storage(db); |
Contributor
Author
There was a problem hiding this comment.
bug - one instance of storage should be under test
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+29
| const numCreate = cleanedMigration.match(/\bcreate\b/gi)?.length || 0; | ||
| assert(result.count > 0, "migrations execution failed"); | ||
| assert(result.count === numCreate, "migrations execution failed"); |
There was a problem hiding this comment.
The assertion that result.count equals the number of CREATE statements may be fragile and depends on D1's internal behavior. Consider documenting this assumption more clearly or making the validation more robust. For example, you could verify the actual tables exist rather than relying on result.count, or add a comment explaining what result.count represents in the context of D1 DDL operations.
Suggested change
| const numCreate = cleanedMigration.match(/\bcreate\b/gi)?.length || 0; | |
| assert(result.count > 0, "migrations execution failed"); | |
| assert(result.count === numCreate, "migrations execution failed"); | |
| // D1's `result.count` reflects the number of statements executed. We only | |
| // assert that some statements ran, rather than assuming a strict mapping | |
| // to a derived count of CREATE statements, which would be fragile. | |
| assert(result.count > 0, "migrations execution failed"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Unify database test helpers across indexer packages and clean up obsolete schema and test utilities.
New Features:
Bug Fixes:
Enhancements:
Documentation: