test: add NWC faucet integration test#535
Open
DSanich wants to merge 5 commits intogetAlby:masterfrom
Open
Conversation
📝 WalkthroughWalkthroughAdds end-to-end testing for the NWC faucet: new E2E test and helper with retry/backoff and lud16 parsing, a dedicated Jest E2E config, updates to main Jest config and npm scripts, and expanded .gitignore entries for test artifacts. No exported/public library APIs changed. Changes
Sequence Diagram(s)mermaid TestRunner->>Helper: createTestWallet(balanceSats) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
Contributor
Author
|
@rolznz Hi! #533 (comment) - As we discussed here, I added one Jest test. I look forward to your review 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.gitignore (1)
3-4:playwright-reportentry seems unrelated to this Jest-only PR.If Playwright isn't used anywhere in the repo, this entry is dead weight. If it's anticipating future Playwright adoption, a comment would help.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 3 - 4, The .gitignore contains an entry "playwright-report" that appears unrelated to this Jest-only PR; either remove the "playwright-report" line to avoid dead-weight ignores or keep it but add a clarifying comment above it indicating it's reserved for future Playwright artifacts (e.g., "reserved for Playwright reports if/when Playwright is adopted"); ensure you only modify the "playwright-report" entry (leave "test-results" as-is) and commit the change so the intent is clear to reviewers.src/test/helpers.ts (2)
23-25: Add anAbortControllersignal to thefetchcall.Without a signal, the underlying TCP connection persists even after Jest's
30_000 mstimeout fires, which can delay teardown or interfere with subsequent test runs.♻️ Proposed fix
+ const controller = new AbortController(); + const fetchTimeout = setTimeout(() => controller.abort(), 25_000); const response = await fetch(`${FAUCET_URL}?balance=${balanceSats}`, { method: "POST", + signal: controller.signal, }); + clearTimeout(fetchTimeout);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/helpers.ts` around lines 23 - 25, The fetch call that posts to `${FAUCET_URL}?balance=${balanceSats}` (the const response = await fetch(...) in helpers.ts) needs an AbortController signal so the request is cancelled on test timeout; create an AbortController, start a timer (e.g., via setTimeout) that calls controller.abort() after Jest's timeout threshold, pass controller.signal into the fetch options, and clear the timer once the fetch resolves/rejects so the TCP connection is properly torn down.
27-30: Duplicated backoff logic and an unreachable finalthrow.Two separate issues in the retry loop:
- The delay-and-
continuepattern (lines 27–30 and 43–46) is identical in both branches — extract to a shared helper or consolidate into a singlefinally-style check after the try/catch.- The
throwat line 50 is unreachable: in the last iteration thethrowat line 31 is raised insidetry, immediately caught at line 42, and re-thrown at line 47, so theforloop never completes normally.♻️ Proposed refactor
export async function createTestWallet( balanceSats = 10_000, retries = 5, ): Promise<TestWallet> { + let lastError: unknown; for (let i = 0; i < retries; i++) { try { const response = await fetch(`${FAUCET_URL}?balance=${balanceSats}`, { method: "POST", }); if (!response.ok) { throw new Error(`Faucet request failed: ${response.status}`); } const nwcUrl = (await response.text()).trim(); const lud16Match = nwcUrl.match(/lud16=([^&\s]+)/); if (!lud16Match) { throw new Error("No lud16 in NWC URL"); } return { nwcUrl, lightningAddress: decodeURIComponent(lud16Match[1]), }; } catch (error) { + lastError = error; if (i < retries - 1) { await new Promise((r) => setTimeout(r, 2000 * (i + 1))); - continue; } - throw error; } } - throw new Error("Failed to create test wallet"); + throw lastError ?? new Error("Failed to create test wallet"); }Also applies to: 43-46, 50-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/helpers.ts` around lines 27 - 30, Consolidate the duplicate backoff logic by extracting the setTimeout-based delay into a shared helper (e.g., sleep(ms)) and replace both duplicated await new Promise((r) => setTimeout(...)) occurrences with await sleep(2000 * (i + 1)); also fix the unreachable throw by removing the throw that never executes and instead either rethrow the caught error when i === retries - 1 inside the catch or capture the last error (e.g., lastError) and after the for loop throw lastError so that the final error is actually propagated; update the retry loop that uses variables retries and i to use this single-place delay and proper final throw behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.gitignore:
- Around line 3-4: The .gitignore contains an entry "playwright-report" that
appears unrelated to this Jest-only PR; either remove the "playwright-report"
line to avoid dead-weight ignores or keep it but add a clarifying comment above
it indicating it's reserved for future Playwright artifacts (e.g., "reserved for
Playwright reports if/when Playwright is adopted"); ensure you only modify the
"playwright-report" entry (leave "test-results" as-is) and commit the change so
the intent is clear to reviewers.
In `@src/test/helpers.ts`:
- Around line 23-25: The fetch call that posts to
`${FAUCET_URL}?balance=${balanceSats}` (the const response = await fetch(...) in
helpers.ts) needs an AbortController signal so the request is cancelled on test
timeout; create an AbortController, start a timer (e.g., via setTimeout) that
calls controller.abort() after Jest's timeout threshold, pass controller.signal
into the fetch options, and clear the timer once the fetch resolves/rejects so
the TCP connection is properly torn down.
- Around line 27-30: Consolidate the duplicate backoff logic by extracting the
setTimeout-based delay into a shared helper (e.g., sleep(ms)) and replace both
duplicated await new Promise((r) => setTimeout(...)) occurrences with await
sleep(2000 * (i + 1)); also fix the unreachable throw by removing the throw that
never executes and instead either rethrow the caught error when i === retries -
1 inside the catch or capture the last error (e.g., lastError) and after the for
loop throw lastError so that the final error is actually propagated; update the
retry loop that uses variables retries and i to use this single-place delay and
proper final throw behavior.
Contributor
|
@DSanich thanks for the PR! these should somehow be differentiated from our non-E2E tests. How can we do that? |
Contributor
Author
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
e2e/helpers.ts (1)
23-25: No timeout onfetch— a stalled connection will hang until Jest's test-level timeout fires, blocking all retry attempts.Add an
AbortControllersignal so each attempt has a bounded duration:♻️ Suggested fix
+ const controller = new AbortController(); + const fetchTimeout = setTimeout(() => controller.abort(), 5000); const response = await fetch(`${FAUCET_URL}?balance=${balanceSats}`, { method: "POST", + signal: controller.signal, }); + clearTimeout(fetchTimeout);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers.ts` around lines 23 - 25, The fetch call that posts to `${FAUCET_URL}?balance=${balanceSats}` has no timeout and can hang; modify the helper so each attempt creates an AbortController, set a per-attempt timeout (e.g., setTimeout that calls controller.abort()) and pass controller.signal into fetch, and clear the timeout after fetch completes so stalled connections are aborted and retries can proceed; update the code that uses the response variable to handle AbortError appropriately.package.json (1)
51-51:lint:jsdoes not covere2e/.
formatandformat:fixwere extended to includee2e/**/*.(js|ts)(lines 54–55), butlint:jsstill targets onlysrc. E2E files will be formatted but never statically analysed.♻️ Suggested fix
- "lint:js": "eslint src --ext .js,.ts --max-warnings 0", + "lint:js": "eslint src e2e --ext .js,.ts --max-warnings 0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 51, The "lint:js" npm script currently only lints src files; update the "lint:js" script (script name lint:js) to include the e2e directory as well (same extensions as format/format:fix) so E2E files are statically analysed—modify the command to target both src and e2e (or use a glob like e2e/**/*.{js,ts}) while keeping --ext .js,.ts and --max-warnings 0.
🤖 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/helpers.ts`:
- Around line 33-48: The parse error "No lud16 in NWC URL" is being swallowed by
the surrounding retry loop because the throw inside the try is caught by the
catch that retries; update the logic in the function that reads nwcUrl (use
symbols nwcUrl, lud16Match, lightningAddress and the retries loop) so that only
transient I/O/errors trigger a retry: after reading nwcUrl and computing
lud16Match, if lud16Match is falsy immediately throw the parse error (or
rethrow) outside the retry catch path (e.g., validate and throw before the
try/retry block or detect parse errors in the catch and rethrow without
retrying) so parse/logic errors do not incur retry delays.
In `@e2e/nwc-faucet.test.ts`:
- Around line 19-23: The test currently calls nwc.getBalance() and then
nwc.close(), but if NWCClient.getBalance() rejects the nwc.close() call is
skipped and sockets remain open; wrap the call to nwc.getBalance() in a
try/finally (using the existing nwc variable and NWCClient.close()) so that
nwc.close() is invoked in the finally block regardless of success or failure,
ensuring WebSocket connections are always closed even on errors.
- Around line 14-26: The Jest test timeout (30_000) is too small for
createTestWallet's linear backoff (retries default = 5) plus network round-trips
and NWCClient.getBalance's replyTimeout; update the test to either increase the
Jest timeout (e.g., to cover worst-case backoff + replyTimeout) or call
createTestWallet with a reduced retries/ceiling (e.g., retries = 3) so the total
backoff (2+4s) fits inside 30_000; adjust the call site in the test that invokes
createTestWallet and/or the test's third-argument timeout value to implement the
chosen option and rerun the test.
---
Nitpick comments:
In `@e2e/helpers.ts`:
- Around line 23-25: The fetch call that posts to
`${FAUCET_URL}?balance=${balanceSats}` has no timeout and can hang; modify the
helper so each attempt creates an AbortController, set a per-attempt timeout
(e.g., setTimeout that calls controller.abort()) and pass controller.signal into
fetch, and clear the timeout after fetch completes so stalled connections are
aborted and retries can proceed; update the code that uses the response variable
to handle AbortError appropriately.
In `@package.json`:
- Line 51: The "lint:js" npm script currently only lints src files; update the
"lint:js" script (script name lint:js) to include the e2e directory as well
(same extensions as format/format:fix) so E2E files are statically
analysed—modify the command to target both src and e2e (or use a glob like
e2e/**/*.{js,ts}) while keeping --ext .js,.ts and --max-warnings 0.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/nwc-faucet.test.ts`:
- Around line 14-28: No changes needed — the test was updated to increase the
timeout to 60_000 and call createTestWallet(EXPECTED_BALANCE_SATS, 3) to handle
retries, and the NWCClient instance is properly closed in a finally block via
nwc.close() after calling nwc.getBalance(), so leave the test as-is.
DSanich
force-pushed
the
feat/jest-nwc-faucet-test
branch
from
f8c77bd to
7e7f034
Compare
Contributor
Author
rolznz
reviewed
Feb 21, 2026
| preset: 'ts-jest', | ||
| testEnvironment: 'node', | ||
| testPathIgnorePatterns: ['/node_modules/', '/e2e/browser/'], | ||
| testPathIgnorePatterns: ['/node_modules/', '/e2e/'], |
Contributor
There was a problem hiding this comment.
ah, I see there's a new config for e2e
rolznz
reviewed
Feb 21, 2026
| module.exports = { | ||
| preset: 'ts-jest', | ||
| testEnvironment: 'node', | ||
| testMatch: ['<rootDir>/e2e/**/*.test.ts'], |
Contributor
There was a problem hiding this comment.
how does this work? the browser folder is in there and that should be done by playwright?
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Adds a single Jest integration test that creates a wallet via the NWC faucet and verifies the balance is 10_000 sats. Per reviewer feedback, this PR focuses on the test setup for easier review.
Changes
src/test/helpers.ts—createTestWallet(balanceSats)helper using https://faucet.nwc.dev (inspired by Alby CLI tests)src/test/nwc-faucet.test.ts— Integration test: create wallet → get balance viaNWCClient.getBalance()→ assert 10_000 sats.gitignore— Addtest-resultsandplaywright-reportHow to run
yarn test src/test/nwc-faucet.test.ts
Summary by CodeRabbit
Tests
Chores