Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds five new NWC end-to-end tests (invoice, pay, keysend, insufficient-funds, invoice lookup, transaction listing), tweaks one faucet test invocation, and sets Jest e2e to run serially via Changes
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 |
|
@rolznz hi! I continued working on the library's test coverage. I added tests covering the payments area. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
jest.e2e.config.ts (1)
3-7: Centralize WebSocket polyfill in Jest setup instead of per-test imports.Running with
maxWorkers: 1is a good stability change. To reduce duplication and keep test files cleaner, loadwebsocket-polyfillonce viasetupFiles. This will remove the import statement from all 6 e2e test files.♻️ Proposed config refactor
export default { preset: 'ts-jest', testEnvironment: 'node', testMatch: ['<rootDir>/e2e/**/*.test.ts'], testPathIgnorePatterns: ['/node_modules/', '/e2e/browser'], + setupFiles: ['<rootDir>/e2e/setup-websocket.ts'], maxWorkers: 1, // Run e2e tests serially to avoid faucet rate limiting };// e2e/setup-websocket.ts import "websocket-polyfill";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jest.e2e.config.ts` around lines 3 - 7, Add a single global setup file to load the websocket polyfill instead of importing it in each test: create an e2e/setup-websocket.ts that does import "websocket-polyfill"; then update the Jest config (the object with keys preset, testEnvironment, testMatch, testPathIgnorePatterns, maxWorkers) to include that file in setupFiles (e.g. setupFiles: ['<rootDir>/e2e/setup-websocket.ts']), and remove the per-test websocket-polyfill imports from the e2e test files so the polyfill is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/nwc-pay-invoice-insufficient-funds.test.ts`:
- Around line 37-39: The test currently only asserts the thrown type; tighten it
by also asserting the specific insufficient-funds error code: when calling
senderClient.payInvoice({ invoice: invoiceResult.invoice }) update the assertion
to check both the error class (Nip47WalletError) and that the thrown error
contains the INSUFFICIENT_BALANCE code (e.g. await
expect(...).rejects.toMatchObject({ code: 'INSUFFICIENT_BALANCE' }) or combine
with .toBeInstanceOf(Nip47WalletError)); reference senderClient.payInvoice,
Nip47WalletError and the INSUFFICIENT_BALANCE error code in your assertion.
---
Nitpick comments:
In `@jest.e2e.config.ts`:
- Around line 3-7: Add a single global setup file to load the websocket polyfill
instead of importing it in each test: create an e2e/setup-websocket.ts that does
import "websocket-polyfill"; then update the Jest config (the object with keys
preset, testEnvironment, testMatch, testPathIgnorePatterns, maxWorkers) to
include that file in setupFiles (e.g. setupFiles:
['<rootDir>/e2e/setup-websocket.ts']), and remove the per-test
websocket-polyfill imports from the e2e test files so the polyfill is
centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd797b9a-8d37-4702-84a0-ffd9e6cfdba2
📒 Files selected for processing (7)
e2e/nwc-faucet.test.tse2e/nwc-list-transactions-after-payment.test.tse2e/nwc-lookup-invoice.test.tse2e/nwc-pay-invoice-insufficient-funds.test.tse2e/nwc-pay-invoice.test.tse2e/nwc-pay-keysend.test.tsjest.e2e.config.ts
|
@rolznz Hello! Have you had a chance to test this? I think this PR is useful as a continuation of our work on test coverage. |
|
@DSanich did you see the comment from coderabbit? I tagged you there |
|
@DSanich thanks for the additional tests, looks good now! |
2 participants
Summary
pay_invoice(success path)lookup_invoiceafter paymentpay_keysend(success path)pay_invoicewith insufficient funds (negative case)list_transactionsafter successful paymente2esuite.maxWorkers: 1injest.e2e.config.tsto reduce flakiness caused by faucet rate limits.Why
INSUFFICIENT_BALANCE).Test plan
yarn jest --config=jest.e2e.config.ts e2e/nwc-pay-invoice.test.tsyarn jest --config=jest.e2e.config.ts e2e/nwc-lookup-invoice.test.tsyarn jest --config=jest.e2e.config.ts e2e/nwc-pay-keysend.test.tsyarn jest --config=jest.e2e.config.ts e2e/nwc-faucet.test.tsyarn jest --config=jest.e2e.config.ts e2e/nwc-pay-invoice-insufficient-funds.test.tsyarn jest --config=jest.e2e.config.ts e2e/nwc-list-transactions-after-payment.test.tsSummary by CodeRabbit