Test: Shared getBalance tests for both facades#1051
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReorganized and split getBalance tests: removed legacy suites and added a shared adapter-driven suite plus service- and fullnode-specific suites. Tests cover empty-wallet state, fund injection, readiness errors, internal-transfer invariance, and custom-token scenarios; wallets are stopped/teared down for isolation. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1051 +/- ##
=======================================
Coverage 87.93% 87.93%
=======================================
Files 114 114
Lines 8910 8910
Branches 2030 2020 -10
=======================================
Hits 7835 7835
Misses 1047 1047
Partials 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/integration/service-specific/get-balance.test.ts (1)
61-110: Consider adding issue references to the FIXME comments.The fullnode-specific tests reference issue
#397for known bugs. Adding similar issue references here would help track when these skipped tests can be enabled.- // FIXME: The test does not return balance for empty wallet. It should return 0 for the native token + // FIXME: The test does not return balance for empty wallet. It should return 0 for the native token. Ref `#XXX` it.skip('should return balance array for empty wallet', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/service-specific/get-balance.test.ts` around lines 61 - 110, Update the FIXME comments above the two skipped tests ("should return balance array for empty wallet" and "should return balance for specific token when token parameter is provided") to include a reference to the tracking issue (e.g., "#397" or the appropriate issue number) and a short note about the expected behavior; locate the comments adjacent to the it.skip blocks that use buildWalletInstance, emptyWallet, and NATIVE_TOKEN_UID and append the issue reference so future maintainers know which issue to watch before unskipping the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/integration/service-specific/get-balance.test.ts`:
- Around line 61-110: Update the FIXME comments above the two skipped tests
("should return balance array for empty wallet" and "should return balance for
specific token when token parameter is provided") to include a reference to the
tracking issue (e.g., "#397" or the appropriate issue number) and a short note
about the expected behavior; locate the comments adjacent to the it.skip blocks
that use buildWalletInstance, emptyWallet, and NATIVE_TOKEN_UID and append the
issue reference so future maintainers know which issue to watch before
unskipping the tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09d987e9-870c-456c-b129-d46998f045ae
📒 Files selected for processing (5)
__tests__/integration/fullnode-specific/get-balance.test.ts__tests__/integration/hathorwallet_facade.test.ts__tests__/integration/service-specific/get-balance.test.ts__tests__/integration/shared/get-balance.test.ts__tests__/integration/walletservice_facade.test.ts
tuliomir
force-pushed
the
test/shared-get-balance
branch
from
8165381 to
d21bcb7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/integration/fullnode-specific/get-balance.test.ts (1)
51-52: Consider adding non-null assertion for consistency.Other tests in the codebase use
hash!when passing the transaction hash to wait functions (e.g.,pollForTx(wallet, fundTx.hash!)). For consistency and to make the assumption explicit:🔧 Suggested change
const tx1 = await hWallet.sendTransaction(await hWallet.getAddressAtIndex(1), 2n); - await waitForTxReceived(hWallet, tx1.hash); + await waitForTxReceived(hWallet, tx1.hash!);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/fullnode-specific/get-balance.test.ts` around lines 51 - 52, The test passes tx1.hash to waitForTxReceived without a non-null assertion; update the call to assert the hash is non-null (use tx1.hash!) so it matches the project's convention and makes the assumption explicit when calling waitForTxReceived(hWallet, tx1.hash!). Locate the send/await block around tx1 created by hWallet.sendTransaction and change the waitForTxReceived invocation to use the non-null asserted tx1.hash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/integration/fullnode-specific/get-balance.test.ts`:
- Around line 51-52: The test passes tx1.hash to waitForTxReceived without a
non-null assertion; update the call to assert the hash is non-null (use
tx1.hash!) so it matches the project's convention and makes the assumption
explicit when calling waitForTxReceived(hWallet, tx1.hash!). Locate the
send/await block around tx1 created by hWallet.sendTransaction and change the
waitForTxReceived invocation to use the non-null asserted tx1.hash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1a24054-58de-4100-9c8f-214dd2e26693
📒 Files selected for processing (5)
__tests__/integration/fullnode-specific/get-balance.test.ts__tests__/integration/hathorwallet_facade.test.ts__tests__/integration/service-specific/get-balance.test.ts__tests__/integration/shared/get-balance.test.ts__tests__/integration/walletservice_facade.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/integration/service-specific/get-balance.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/hathorwallet_facade.test.ts
Extract getBalance coverage from monolithic facade test files into the shared test framework. Shared tests run against both fullnode and wallet-service adapters via describe.each. - shared/get-balance.test.ts: zero balance, funded balance - fullnode-specific/get-balance.test.ts: custom token, mandatory tokenUid, internal transfer - service-specific/get-balance.test.ts: no-arg getBalance, not-ready error, skipped empty-wallet bugs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add issue #397 reference to FIXME comments on skipped empty-wallet balance tests in service-specific - Add non-null assertion to tx1.hash in fullnode-specific waitForTxReceived call Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tuliomir
force-pushed
the
test/shared-get-balance
branch
from
d21bcb7 to
310958b
Compare
Replace non-null assertion with explicit expect guard before passing address to injectFunds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move 'internal transfer' and 'custom token' tests from fullnode-specific to shared get-balance suite. Extract fullnode-only assertions (nonexistent token zero-balance, cross-wallet isolation) into dedicated fullnode-specific tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
__tests__/integration/shared/get-balance.test.ts (1)
74-95: Minor inconsistency in address null-checking.Line 85 passes
getAddressAtIndex(1)directly tosendTransactionwithout the explicittoBeDefined()check used on lines 79-80 for index 0. While the test would fail clearly if the address is undefined, maintaining consistency aids readability.♻️ Optional: Add consistent null-check
- const tx = await wallet.sendTransaction(await wallet.getAddressAtIndex(1), 2n, { + const addr1 = await wallet.getAddressAtIndex(1); + expect(addr1).toBeDefined(); + const tx = await wallet.sendTransaction(addr1!, 2n, { pinCode: adapter.defaultPinCode, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/shared/get-balance.test.ts` around lines 74 - 95, The test inconsistently omits a null-check for the destination address: capture the address returned by wallet.getAddressAtIndex(1) into a variable (e.g., destAddr), add an expect(destAddr).toBeDefined() before calling wallet.sendTransaction, and then pass destAddr! to wallet.sendTransaction; update references to the address variable so wallet.sendTransaction and any other uses reference the checked variable (functions: wallet.getAddressAtIndex, wallet.sendTransaction).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/integration/fullnode-specific/get-balance.test.ts`:
- Line 1: There are Prettier formatting violations starting at the file's
leading JSDoc comment (the "/**" block); run your project's formatter (e.g.,
prettier --write) against this test file to reformat it, stage the updated file,
and commit the changes so the CI style checks pass; ensure you use the repo's
Prettier config and re-run the test/CI locally before pushing.
---
Nitpick comments:
In `@__tests__/integration/shared/get-balance.test.ts`:
- Around line 74-95: The test inconsistently omits a null-check for the
destination address: capture the address returned by wallet.getAddressAtIndex(1)
into a variable (e.g., destAddr), add an expect(destAddr).toBeDefined() before
calling wallet.sendTransaction, and then pass destAddr! to
wallet.sendTransaction; update references to the address variable so
wallet.sendTransaction and any other uses reference the checked variable
(functions: wallet.getAddressAtIndex, wallet.sendTransaction).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32b8d6e7-7efa-4070-a782-bd890275c1e1
📒 Files selected for processing (2)
__tests__/integration/fullnode-specific/get-balance.test.ts__tests__/integration/shared/get-balance.test.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| it('should not show custom token balance on a different wallet', async () => { | ||
| const hWallet = await generateWalletHelper(); | ||
|
|
||
| await GenesisWalletHelper.injectFunds(hWallet, await hWallet.getAddressAtIndex(0), 10n); | ||
| const newTokenAmount = BigInt(getRandomInt(1000, 10)); | ||
| const { hash: tokenUid } = await createTokenHelper( | ||
| hWallet, | ||
| 'BalanceToken', | ||
| 'BAT', | ||
| newTokenAmount | ||
| ); | ||
|
|
||
| const { hWallet: gWallet } = await GenesisWalletHelper.getSingleton(); | ||
| const genesisTknBalance = await gWallet.getBalance(tokenUid); | ||
| expect(genesisTknBalance).toHaveLength(1); | ||
| expect(genesisTknBalance[0]).toMatchObject({ | ||
| token: { id: tokenUid }, | ||
| balance: { unlocked: 0n, locked: 0n }, | ||
| transactions: 0, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Why isn't it in the shared tests?
There was a problem hiding this comment.
TL;DR: It's blocked by a wallet-service inconsistency (issue #397). Once the wallet-service returns a zero-balance entry instead of an empty array for unknown tokens, this test can be promoted to the shared suite.
--
The fullnode facade returns a zero-balance entry (line 60-64 of the test) when you ask for the balance of a token the wallet has never interacted with — the fullnode always synthesizes a response. The wallet-service facade, on the other hand, has a known bug where it returns an empty array for tokens the wallet doesn't hold (see the two FIXME(wallet-service) comments at lines 62-64 and 79-80 of service-specific/get-balance.test.ts, referencing issue #397).
Since the two facades don't agree on the return shape for "token not on this wallet," the test can't be written with a single shared assertion. Moving it to the shared suite would either require:
- Different expectations per adapter (defeating the purpose of shared tests), or
- The wallet-service bug to be fixed first so both facades return a consistent zero-balance entry.
The test was intentionally kept fullnode-specific until the wallet-service behavior converges.
There was a problem hiding this comment.
Did you test it? We had this problem and Abadesso fixed it in the wallet service. It was implemented December last year
| // FIXME(wallet-service): getBalance() on an empty wallet should return a single | ||
| // entry with 0 balance for the native token, but currently returns an empty array. | ||
| // Ref: https://github.com/HathorNetwork/hathor-wallet-lib/issues/397 | ||
| it.skip('should return balance array for empty wallet', async () => { |
There was a problem hiding this comment.
We had fixed that and the next one, not sure why it's not working
There was a problem hiding this comment.
I tried removing all the skips but the tests just failed. Here's a small report on it:
--
-
should return balance array for empty wallet — wallet-service still returns an empty array (
length: 0) instead of one entry with zero balance (length: 1) -
should return balance for specific token when token parameter is provided — similarly, the assertion at line
83expectsbalances.lengthto be1, but the first test at line69already shows it's0.
There was a problem hiding this comment.
Because of this investigation above I ended up identifying another specific test that could to go Shared.
It's not perfect since it cannot validate the value, but it's better than a skipped specific test anyway. Done on 08f37ef
There was a problem hiding this comment.
@tuliomir did you run with a updated image of wallet-sevice? This is a known issue of fixes not being applied to the integration tests due to the usage of stale container images.
There was a problem hiding this comment.
I just re-created the images locally with the HEAD at HathorNetwork/hathor-wallet-service@947e90f . This has still failed.
The wallet service still does not synthesize a "zero balance" record for HTR when there are no transactions.
Could we solve this in a future PR so that we can advance with the Shared Tests suite construction? Future improvements will be easier to implement when we have that in place.
There was a problem hiding this comment.
Yep. Open an issue for it pls
The getBalance(tokenUid) test for empty wallets works on both facades. Moved from service-specific (where it was skipped) to the shared suite with updated assertions for the `version` field and facade-agnostic authority checks. The no-arg getBalance() empty wallet test remains skipped per #397. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| try { | ||
| await wallet.stop({ cleanStorage: true }); | ||
| } catch { | ||
| // Wallet may already be stopped |
There was a problem hiding this comment.
it's good to see if the error that is being raised is related to the wallet stopping. Otherwise it could hide something.
| // FIXME(wallet-service): getBalance() on an empty wallet should return a single | ||
| // entry with 0 balance for the native token, but currently returns an empty array. | ||
| // Ref: https://github.com/HathorNetwork/hathor-wallet-lib/issues/397 | ||
| it.skip('should return balance array for empty wallet', async () => { |
There was a problem hiding this comment.
@tuliomir did you run with a updated image of wallet-sevice? This is a known issue of fixes not being applied to the integration tests due to the usage of stale container images.
github-project-automation
bot
moved this from In Progress (Done)
to In Review (WIP)
in Hathor Network
github-project-automation
bot
moved this from In Review (WIP)
to Waiting to be deployed
in Hathor Network
3 participants
Summary
getBalancetests fromhathorwallet_facade.test.tsandwalletservice_facade.test.tsinto the shared test frameworkshared/get-balance.test.tswith adapter-driven tests that run against both facadesfullnode-specific/get-balance.test.tsservice-specific/get-balance.test.tsAcceptance Criteria
getBalance/balancesdescribe blocks remain in the monolithic facade filesTest plan
npm run testpassesnpm run test_integrationpasses (shared tests run against both facades)🤖 Generated with Claude Code
Summary by CodeRabbit