TesSuiteHelper #246
Closed
TesSuiteHelper #246
Conversation
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
Signed-off-by: Robert Zaremba <robert@zaremba.ch>
Contributor
Reviewer's GuideRefactors the BTC indexer test suite helper from a factory function and interface into a stateful TesSuiteHelper class that encapsulates environment setup, mocks, and utility methods, and updates btcindexer tests to use the new class-based helper lifecycle. Sequence diagram for TesSuiteHelper-based btcindexer test setupsequenceDiagram
actor JestRunner
participant BeforeAll as beforeAll
participant BeforeEach as beforeEach
participant Miniflare as Miniflare
participant Suite as TesSuiteHelper
participant DB as D1Database
participant KVBlocks as KVNamespace_BtcBlocks
participant KVTxs as KVNamespace_nbtc_txs
participant Storage as CFStorage
participant Indexer as Indexer
participant MockSui as MockSuiClient
participant Electrs as Electrs
JestRunner->>BeforeAll: configure Miniflare
BeforeAll->>Miniflare: new Miniflare(options)
Miniflare-->>BeforeAll: mf instance
JestRunner->>BeforeEach: start test setup
BeforeEach->>Suite: new TesSuiteHelper(options)
Suite-->>BeforeEach: suite instance
BeforeEach->>Suite: init(mf)
activate Suite
Suite->>Miniflare: getD1Database(DB)
Miniflare-->>Suite: DB
Suite->>DB: initDb()
Suite->>Miniflare: getBindings()
Miniflare-->>Suite: MfBindings
Suite->>Storage: new CFStorage(DB, BtcBlocks, nbtc_txs)
Suite->>Suite: set blocksKV, txsKV
Suite->>Suite: resolve packageConfig from options or TEST_PACKAGE_CONFIG
Suite->>DB: INSERT setups row
DB-->>Suite: ok
Suite->>DB: INSERT nbtc_deposit_addresses for each deposit address
DB-->>Suite: ok
Suite->>Suite: create MockSuiClient or use customSuiClient
Suite->>Suite: create Electrs mock
Suite->>Indexer: new Indexer(storage, packageConfigs, suiClients, nbtcAddressesMap, confirmationDepth, maxRetries, electrsClients)
Indexer-->>Suite: indexer instance
deactivate Suite
BeforeEach->>Suite: access indexer
Suite-->>BeforeEach: indexer
BeforeEach->>JestRunner: tests run with suite and indexer
JestRunner->>Suite: setupBlock, insertTx, expectMintingCount, expectTxStatus
Suite->>DB: read and write test data
Suite->>KVBlocks: put raw block
Suite->>KVTxs: use txs namespace as needed
Suite->>MockSui: mockSuiMintBatch
Suite->>Electrs: mockElectrsSender, mockElectrsError
Suite-->>JestRunner: helper assertions complete
Class diagram for refactored TesSuiteHelper test utilityclassDiagram
class TesSuiteHelper {
+Indexer indexer
+D1Database db
+KVNamespace blocksKV
+KVNamespace txsKV
+CFStorage storage
+MockSuiClient mockSuiClient
+Electrs mockElectrs
-TestBlocks testData
-SetupOptions options
+TesSuiteHelper(options SetupOptions)
+init(mf Miniflare) void
+setupBlock(height number) void
+getBlock(height number) Block
+getTx(height number, txIndex number) GetTxResult
+createBlockQueueRecord(height number, options BlockQueueRecordOptions) BlockQueueRecord
+mockElectrsSender(address string) void
+mockElectrsError(error Error) void
+mockSuiMintBatch(result MintBatchResult) void
+insertTx(options InsertTxOptions) void
+expectMintingCount(count number) void
+expectSenderCount(count number, expectedAddress string) void
+expectTxStatus(txId string, expectedStatus MintTxStatusOrString) void
}
class SetupOptions {
+NbtcPkgCfg packageConfig
+string[] depositAddresses
+number confirmationDepth
+number maxRetries
+TestBlocks testData
+MockSuiClient customSuiClient
}
class TestBlocks {
}
class GetTxResult {
+TestBlock blockData
+Block block
+Transaction targetTx
+TxInfo txInfo
}
class BlockQueueRecordOptions {
+number height
+string hash
+boolean is_processed
+number retries
}
class InsertTxOptions {
+string txId
+MintTxStatusOrString status
+number retryCount
+number blockHeight
+string blockHash
+string suiRecipient
+number amountSats
+string depositAddress
+string sender
+number vout
}
class MintBatchResult {
+boolean success
+string message
}
class MintTxStatusOrString {
}
class Indexer {
}
class CFStorage {
}
class MockSuiClient {
+tryMintNbtcBatch(result MintBatchResult) void
}
class Electrs {
+getTx(txId string) Response
}
class Miniflare {
+getD1Database(name string) D1Database
+getBindings() MfBindings
}
class MfBindings {
+D1Database DB
+KVNamespace BtcBlocks
+KVNamespace nbtc_txs
}
class NbtcPkgCfg {
+string id
+string btc_network
+string sui_network
+string nbtc_pkg
+string nbtc_contract
+string lc_pkg
+string lc_contract
+string sui_fallback_address
+boolean is_active
}
class NbtcDepositAddrsMap {
}
class SuiClientI {
}
class SuiNet {
}
class BtcNet {
}
TesSuiteHelper --> SetupOptions
TesSuiteHelper --> TestBlocks
TesSuiteHelper --> GetTxResult
TesSuiteHelper --> BlockQueueRecordOptions
TesSuiteHelper --> InsertTxOptions
TesSuiteHelper --> MintBatchResult
TesSuiteHelper --> MintTxStatusOrString
TesSuiteHelper --> Indexer
TesSuiteHelper --> CFStorage
TesSuiteHelper --> MockSuiClient
TesSuiteHelper --> Electrs
TesSuiteHelper --> Miniflare
TesSuiteHelper --> NbtcDepositAddrsMap
TesSuiteHelper --> NbtcPkgCfg
TesSuiteHelper --> MfBindings
MockSuiClient ..|> SuiClientI
Electrs --> BtcNet
MockSuiClient --> SuiNet
CFStorage --> D1Database
CFStorage --> KVNamespace
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:
- The new
TesSuiteHelperclass appears to have a typo in its name (missing the second 't' inTest), which will make usage less clear and inconsistent with the file name and previous helper naming. - In
beforeEach,suite.init(mf)is an async method but is not awaited, andbeforeAllwas changed to non-async; this can lead to tests running before initialization completes, so makebeforeEachasyncandawait suite.init(mf)(and adjustbeforeAllif needed).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `TesSuiteHelper` class appears to have a typo in its name (missing the second 't' in `Test`), which will make usage less clear and inconsistent with the file name and previous helper naming.
- In `beforeEach`, `suite.init(mf)` is an async method but is not awaited, and `beforeAll` was changed to non-async; this can lead to tests running before initialization completes, so make `beforeEach` `async` and `await suite.init(mf)` (and adjust `beforeAll` if needed).
## Individual Comments
### Comment 1
<location> `packages/btcindexer/src/btcindexer.test.ts:69-72` </location>
<code_context>
+let suite: TesSuiteHelper;
-beforeAll(async () => {
+beforeAll(() => {
mf = new Miniflare({
script: "",
</code_context>
<issue_to_address>
**issue (bug_risk):** The async `init` method on `TesSuiteHelper` is not awaited in `beforeEach`, which can leave `indexer` uninitialized when tests run.
`TesSuiteHelper.init` is `async`, but `beforeEach` calls `suite.init(mf);` without `await`. Because `init` sets `suite.indexer`, statements like `indexer = suite.indexer;` can still see the initial `null!` value. Please either make `beforeEach` async and `await suite.init(mf);`, or change `init` to be synchronous if it does not need to be async.
</issue_to_address>
### Comment 2
<location> `packages/btcindexer/src/btcindexer.helpers.test.ts:63-66` </location>
<code_context>
+ nbtc_txs: KVNamespace;
+}
+
+export class TesSuiteHelper {
indexer: Indexer;
db: D1Database;
</code_context>
<issue_to_address>
**suggestion (typo):** The class name `TesSuiteHelper` looks like a typo and may cause confusion.
Since this is an exported class used by the test suite, the misspelling in the name can be misleading. Please rename it to the intended name (for example, `TestSuiteHelper`) and update the corresponding import in `btcindexer.test.ts`.
Suggested implementation:
```typescript
export class TestSuiteHelper {
```
1. In `packages/btcindexer/src/btcindexer.helpers.test.ts`, update any other references to `TesSuiteHelper` (instantiations, type annotations, etc.) to `TestSuiteHelper`.
2. In `packages/btcindexer/src/btcindexer.test.ts` (and any other test files), update the import to use the new class name. For example:
- Change `import { TesSuiteHelper } from './btcindexer.helpers.test';` to `import { TestSuiteHelper } from './btcindexer.helpers.test';`
- Update any usages of `TesSuiteHelper` to `TestSuiteHelper`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+69
to
72
| beforeAll(() => { | ||
| mf = new Miniflare({ | ||
| script: "", | ||
| modules: true, |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The async init method on TesSuiteHelper is not awaited in beforeEach, which can leave indexer uninitialized when tests run.
TesSuiteHelper.init is async, but beforeEach calls suite.init(mf); without await. Because init sets suite.indexer, statements like indexer = suite.indexer; can still see the initial null! value. Please either make beforeEach async and await suite.init(mf);, or change init to be synchronous if it does not need to be async.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the test suite helper from a function-based approach to a class-based approach. The main change converts setupTestIndexerSuite function and TestIndexerHelper interface into a TesSuiteHelper class (note: appears to be a typo). However, the PR title "TesSuiteHelper" suggests this typo may be intentional, though it's likely an error.
Key changes:
- Converted test helper from async function returning an object to a class with constructor and init method
- Changed test setup to instantiate the class and call init() separately
- Converted helper methods from standalone functions to class methods
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| packages/btcindexer/src/btcindexer.helpers.test.ts | Refactored setupTestIndexerSuite function into TesSuiteHelper class with constructor and async init method, converting all helper functions to class methods |
| packages/btcindexer/src/btcindexer.test.ts | Updated to import and instantiate the new TesSuiteHelper class instead of calling the setup function, modified test lifecycle hooks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| beforeEach(async () => { | ||
| suite = await setupTestIndexerSuite(mf, { | ||
| suite = new TesSuiteHelper({ |
There was a problem hiding this comment.
The constructor call uses "TesSuiteHelper" which contains a typo. It should be "TestSuiteHelper" instead of "TesSuiteHelper" (missing 't' in "Test").
| testData: REGTEST_DATA, | ||
| }); | ||
|
|
||
| suite.init(mf); |
There was a problem hiding this comment.
The beforeEach hook should be kept async since it calls suite.init(mf) which is an async method. The init() call should be awaited to ensure proper initialization before tests run.
Suggested change
| suite.init(mf); | |
| await suite.init(mf); |
Comment on lines
+77
to
+83
| this.db = null!; | ||
| this.blocksKV = null!; | ||
| this.txsKV = null!; | ||
| this.storage = null!; | ||
| this.mockSuiClient = null!; | ||
| this.mockElectrs = null!; | ||
| this.indexer = null!; |
There was a problem hiding this comment.
Initializing properties with null! (non-null assertion) is unsafe. These properties are not actually initialized until init() is called, which could lead to runtime errors if any class methods are called before init(). Consider making init() required in the constructor, or use optional types (e.g., indexer?: Indexer) to better reflect the actual state.
| let suite: TesSuiteHelper; | ||
|
|
||
| beforeAll(async () => { | ||
| beforeAll(() => { |
There was a problem hiding this comment.
The removal of the beforeAll async modifier appears incorrect. While the beforeAll function body itself doesn't contain any await statements, it's generally a good practice to keep async/await consistency with lifecycle hooks, and it's harmless to have it marked as async even if not strictly necessary in this case.
Suggested change
| beforeAll(() => { | |
| beforeAll(async () => { |
| nbtc_txs: KVNamespace; | ||
| } | ||
|
|
||
| export class TesSuiteHelper { |
There was a problem hiding this comment.
The class name "TesSuiteHelper" contains a typo. It should be "TestSuiteHelper" instead of "TesSuiteHelper" (missing 't' in "Test").
Suggested change
| export class TesSuiteHelper { | |
| export class TestSuiteHelper { |
| import { MintTxStatus } from "./models"; | ||
| import { BtcNet, type BlockQueueRecord } from "@gonative-cc/lib/nbtc"; | ||
| import { setupTestIndexerSuite, type TestIndexerHelper } from "./btcindexer.helpers.test"; | ||
| import { TesSuiteHelper } from "./btcindexer.helpers.test"; |
There was a problem hiding this comment.
The imported class name "TesSuiteHelper" contains a typo. It should be "TestSuiteHelper" instead of "TesSuiteHelper" (missing 't' in "Test").
| let mf: Miniflare; | ||
| let indexer: Indexer; | ||
| let suite: TestIndexerHelper; | ||
| let suite: TesSuiteHelper; |
There was a problem hiding this comment.
The variable type "TesSuiteHelper" contains a typo. It should be "TestSuiteHelper" instead of "TesSuiteHelper" (missing 't' in "Test").
Contributor
Author
|
let's keep original code - this one is failing |
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.
Description
Summary by Sourcery
Refactor the BTC indexer test helper into a stateful class to simplify test setup and usage.
Enhancements:
Tests: