refactor: Introduce new setupTest function and helper file for unifying tests#209
Conversation
…ng test suite of btcindexer Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Reviewer's GuideRefactors btcindexer tests to use a centralized setupTestIndexer helper and TestIndexerHelper utilities, consolidating environment, DB, mocks, and common operations such as inserting transactions, seeding blocks, and asserting DB state into a new btcindexer.test.helpers module. Sequence diagram for btcindexer tests using setupTestIndexer helpersequenceDiagram
actor Tester
participant Jest as JestTest
participant Miniflare
participant HelperFn as setupTestIndexer
participant Helper as TestIndexerHelper
participant Indexer
participant D1 as D1Database
participant KVBlocks as BtcBlocks_KV
participant KVTxs as NbtcTxs_KV
participant SuiMock as MockSuiClient
participant ElectrsMock as ElectrsMock
Tester->>Jest: run beforeEach
Jest->>Miniflare: getD1Database DB
Miniflare-->>Jest: D1Database
Jest->>HelperFn: setupTestIndexer(mf, options)
HelperFn->>D1: initDb
HelperFn->>Miniflare: getBindings
Miniflare-->>HelperFn: env(DB, BtcBlocks, nbtc_txs)
HelperFn->>HelperFn: create CFStorage with DB, BtcBlocks, nbtc_txs
HelperFn->>D1: insert nbtc_packages
HelperFn->>D1: insert nbtc_deposit_addresses
HelperFn->>SuiMock: construct MockSuiClient
HelperFn->>ElectrsMock: mkElectrsServiceMock
HelperFn->>Indexer: new Indexer(storage, configs, suiClients, nbtcAddressesMap, confirmationDepth, maxRetries, electrsClients)
HelperFn-->>Jest: TestIndexerHelper
Jest->>Tester: expose helper and indexer
Tester->>Helper: setupBlock(329)
Helper->>KVBlocks: put(blockHash, rawBlockHex)
Tester->>Helper: mockElectrsSender(fakeSender)
Helper->>ElectrsMock: getTx mockResolvedValue(Response vout sender)
Tester->>Indexer: processBlock(blockQueueRecord)
Indexer->>KVBlocks: get(blockHash)
Indexer->>D1: insert nbtc_minting and sender data
Tester->>Helper: expectMintingCount(1)
Helper->>D1: SELECT * FROM nbtc_minting
Helper-->>Tester: assertion result
Class diagram for new btcindexer test helper typesclassDiagram
class TxInfo {
+string id
+string suiAddr
+number amountSats
}
class TestBlock {
+string depositAddr
+number height
+string hash
+string rawBlockHex
+Record~string, TxInfo~ txs
}
class TestBlocks {
<<type>>
+Record~number, TestBlock~
}
class SetupOptions {
+string[] depositAddresses
+NbtcPkgCfg packageConfig
+number confirmationDepth
+number maxRetries
+MockSuiClient customSuiClient
+TestBlocks testData
}
class TestIndexerHelper {
+Indexer indexer
+D1Database db
+KVNamespace blocksKV
+KVNamespace txsKV
+CFStorage storage
+MockSuiClient mockSuiClient
+Electrs mockElectrs
+setupBlock(height number) Promise~void~
+getBlock(height number) Block
+getTx(height number, txIndex number) GetTxResult
+createBlockQueueRecord(height number, options BlockQueueRecordPartial) BlockQueueRecord
+mockElectrsSender(address string) void
+mockElectrsError(error Error) void
+mockSuiMintBatch(result MintBatchResult) void
+insertTx(options InsertTxOptions) Promise~void~
+expectMintingCount(count number) Promise~void~
+expectSenderCount(count number, expectedAddress string) Promise~void~
+expectTxStatus(txId string, expectedStatus string) Promise~void~
}
class GetTxResult {
+TestBlock blockData
+Block block
+Transaction targetTx
+TxInfo txInfo
}
class BlockQueueRecord {
+string hash
+number height
+BtcNet network
+number timestamp_ms
}
class BlockQueueRecordPartial {
<<type>>
+string hash
+number height
+BtcNet network
+number timestamp_ms
}
class InsertTxOptions {
+string txId
+string status
+number retryCount
+number blockHeight
+string blockHash
+string suiRecipient
+number amountSats
+string depositAddress
+string sender
+number vout
}
class MintBatchResult {
<<type>>
+tuple[boolean, string] or null
}
class Indexer
class CFStorage
class MockSuiClient {
+tryMintNbtcBatch(request any) Promise~MintBatchResult~
+verifyBlocks(hashes string[]) Promise~boolean[]~
}
class Electrs {
+getTx(txId string) Promise~Response~
}
class NbtcPkgCfg {
+number id
+BtcNet btc_network
+string sui_network
+string nbtc_pkg
+string nbtc_contract
+string lc_contract
+string lc_pkg
+string sui_fallback_address
+boolean is_active
}
class BtcNet
class Block
class Transaction
class D1Database
class KVNamespace
TestBlocks --> TestBlock : values
TestBlock --> TxInfo : txs values
TestIndexerHelper --> Indexer : owns
TestIndexerHelper --> CFStorage : uses
TestIndexerHelper --> MockSuiClient : exposes mockSuiClient
TestIndexerHelper --> Electrs : exposes mockElectrs
TestIndexerHelper --> D1Database : uses db
TestIndexerHelper --> KVNamespace : uses blocksKV
TestIndexerHelper --> KVNamespace : uses txsKV
TestIndexerHelper --> TestBlocks : uses testData
TestIndexerHelper --> InsertTxOptions : insertTx options
TestIndexerHelper --> BlockQueueRecord : returns
TestIndexerHelper --> BlockQueueRecordPartial : optional override
TestIndexerHelper --> GetTxResult : returns
SetupOptions --> NbtcPkgCfg : packageConfig
SetupOptions --> MockSuiClient : customSuiClient
SetupOptions --> TestBlocks : testData
NbtcPkgCfg --> BtcNet
GetTxResult --> TestBlock
GetTxResult --> Block
GetTxResult --> Transaction
GetTxResult --> TxInfo
MockSuiClient --> MintBatchResult
Indexer --> CFStorage
Indexer --> MockSuiClient
Indexer --> Electrs
CFStorage --> D1Database
CFStorage --> KVNamespace
CFStorage --> KVNamespace
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
few of the names are to be improved. How does it relate to the other PR, where we are trying to move the functions to a common module and reuse it across different packages
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new helper module imports
expectfrombun:testbut the surrounding test suite appears to be Jest-based; consider relying on the Jest globalexpect(and removing the Bun import) to avoid mixing test runners and potential type/runtime conflicts. - In
insertTxand the related tests you pass arbitrary strings like "confirming" and "reorg" asstatus; it would be safer to narrow this type (e.g., a union includingMintTxStatusplus the extra literal states) to catch typos and keep status values consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new helper module imports `expect` from `bun:test` but the surrounding test suite appears to be Jest-based; consider relying on the Jest global `expect` (and removing the Bun import) to avoid mixing test runners and potential type/runtime conflicts.
- In `insertTx` and the related tests you pass arbitrary strings like "confirming" and "reorg" as `status`; it would be safer to narrow this type (e.g., a union including `MintTxStatus` plus the extra literal states) to catch typos and keep status values consistent.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR refactors the btcindexer test file by extracting test setup logic into a new helper file (btcindexer.test.helpers.ts). The refactoring introduces a setupTestIndexer function that centralizes the creation of test fixtures including Miniflare instances, database initialization, mock clients, and helper methods for common test operations.
Key changes:
- Introduced
setupTestIndexerfunction to unify test setup and provide reusable helper methods - Replaced inline test setup with declarative helper calls for improved readability
- Added helper methods for common assertions (
expectMintingCount,expectSenderCount,expectTxStatus)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/btcindexer/src/btcindexer.test.ts | Refactored to use the new test helper, removing ~150 lines of repetitive setup code and replacing with helper method calls |
| packages/btcindexer/src/btcindexer.test.helpers.ts | New file containing setupTestIndexer function and related helper methods for test setup, mocking, and assertions |
Comments suppressed due to low confidence (2)
packages/btcindexer/src/btcindexer.test.ts:25
- These type definitions (TxInfo, TestBlock, TestBlocks) are duplicated in both the test file and the helper file. Since these types are now exported from the helper file and used there, they should be imported from the helper file instead of redeclared here to maintain a single source of truth and avoid potential inconsistencies.
interface TxInfo {
id: string;
suiAddr: string;
amountSats: number;
}
interface TestBlock {
depositAddr: string;
height: number;
hash: string;
rawBlockHex: string;
txs: Record<string, TxInfo>;
}
type TestBlocks = Record<number, TestBlock>;
packages/btcindexer/src/btcindexer.test.ts:602
- This describe block is duplicated - there's another "Indexer.processBlock" describe block at line 161. Both blocks have a test with the same name "should process a block and insert nBTC transactions and sender deposits". The second block (starting here) also has an additional test for reorgs. These should be merged into a single describe block to avoid test name conflicts and confusion. When test frameworks encounter duplicate describe/test names, behavior can be unpredictable (some may skip duplicates, others may run both).
describe("Indexer.processBlock", () => {
const timestamp_ms = Date.now();
it("should process a block and insert nBTC transactions and sender deposits", async () => {
const blockInfo = helper.createBlockQueueRecord(329, { timestamp_ms });
const fakeSenderAddress = "bc1qtestsenderaddress";
await helper.setupBlock(329);
helper.mockElectrsSender(fakeSenderAddress);
await indexer.processBlock(blockInfo);
await helper.expectMintingCount(1);
await helper.expectSenderCount(1, fakeSenderAddress);
});
it("should call detectMintedReorgs when processing a block that causes a reorg", async () => {
const blockData329 = REGTEST_DATA[329]!;
const txData = blockData329.txs[1]!;
await helper.insertTx({ txId: txData.id, status: MintTxStatus.Minted });
await helper.setupBlock(329);
await helper.db
.prepare(
"INSERT INTO btc_blocks (hash, height, network, inserted_at, is_scanned) VALUES (?, ?, ?, ?, ?)",
)
.bind(blockData329.hash, blockData329.height, BtcNet.REGTEST, timestamp_ms, 1)
.run();
const reorgBlockInfo = helper.createBlockQueueRecord(327, {
height: blockData329.height,
timestamp_ms: timestamp_ms + 1000,
});
await helper.setupBlock(327);
await indexer.processBlock(reorgBlockInfo);
const status = await indexer.storage.getTxStatus(txData.id);
expect(status).toEqual(MintTxStatus.MintedReorg);
});
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@robert-zaremba I've opened a new pull request, #239, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Robert Zaremba <robert@zaremba.ch>
Description
Closes: #199
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdSummary by Sourcery
Refactor BTC indexer tests to use a shared setup helper for constructing test indexers and interacting with test data and storage.
Enhancements: