test: Wallet Service Facade initial tests#949
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #949 +/- ##
==========================================
+ Coverage 85.00% 86.62% +1.62%
==========================================
Files 113 113
Lines 8389 8396 +7
Branches 1832 1834 +2
==========================================
+ Hits 7131 7273 +142
+ Misses 1229 1096 -133
+ Partials 29 27 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1b39152 to
c728b35
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/wallet/wallet.ts
Outdated
| this.retryConfig = { | ||
| maxRetries: retryConfig.maxRetries, | ||
| delayBaseMs: retryConfig.delayBaseMs, | ||
| delayMaxMs: retryConfig.delayMaxMs, | ||
| }; |
There was a problem hiding this comment.
The retry configuration is being copied property by property rather than using object spread or a more concise approach. This creates unnecessary code repetition. Consider simplifying to this.retryConfig = { ...retryConfig }; which achieves the same result with less code and is more maintainable.
| this.retryConfig = { | |
| maxRetries: retryConfig.maxRetries, | |
| delayBaseMs: retryConfig.delayBaseMs, | |
| delayMaxMs: retryConfig.delayMaxMs, | |
| }; | |
| this.retryConfig = { ...retryConfig }; |
There was a problem hiding this comment.
For now we'd rather have this property being copied item by item, but this is a good improvement once we settle on a definitive design for this retryConfig.
| // @ts-expect-error - tokenOutput must exist | ||
| expect(tokenOutput).toStrictEqual( | ||
| expect.objectContaining({ | ||
| value: tokenAmount, | ||
| tokenData: 1, | ||
| script: expect.any(Buffer), | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Using @ts-expect-error to suppress type errors for variables that "must exist" indicates a type safety gap. Consider using non-null assertion (tokenOutput!) or restructuring the code to properly handle the case where these outputs might not be found, either with explicit null checks and error throws, or by using find() with proper type guards.
There was a problem hiding this comment.
These issues will be solved soon, with the Shared Tests PR.
For now we'll have to rely on those typescript adjustments.
src/wallet/api/walletApi.ts
Outdated
| import HathorWalletServiceWallet from '../wallet'; | ||
| import { WalletRequestError, TxNotFoundError } from '../../errors'; | ||
| import { parseSchema } from '../../utils/bigint'; | ||
| import helpers from '../../utils/helpers'; |
src/wallet/api/walletApi.ts
Outdated
| // we pass the response data to the guard for additional validations. | ||
| if (response.status === 404 && response.data) { | ||
| walletApi._txNotFoundGuard(response.data); | ||
| throw new WalletRequestError('Error getting transaction by its id.', { |
There was a problem hiding this comment.
Remove this throw? We already have it down there?
There was a problem hiding this comment.
Good catch, this was an obsolete validation. Removed on 64f43d0
| try { | ||
| const sign = this.signMessage(privKey, timestamp, this.walletId); | ||
| const data = await walletApi.createAuthToken(this, timestamp, privKey.xpubkey, sign); |
There was a problem hiding this comment.
I don't get it... Is this a fix? Why are you try/catching it? Maybe add a log?
There was a problem hiding this comment.
Yes, it's a fix!
We were having errors without any relation to whatever was being tested because this function runs asynchronously and crashes when the original caller context had already been fully finished.
Added a console on 9b045be
This reverts commit 9b045be.
This PR is the immediate sequence to #909 , where the infrastructure to test the Wallet Service Facade was implemented.
Acceptance Criteria
socket hang uperrors while requesting theserverless-offlineenvironmentNotes
--forceExitfrom the integration tests as it was expected that it would have been solved by feat: do not start background tasks during tests #934 , but the CI didn't pass without it, so we just moved this flag to the Jest configs instead.Security Checklist