test: add NWC faucet integration test#535
Open
DSanich wants to merge 6 commits intogetAlby:masterfrom
Open
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:
📝 WalkthroughWalkthroughThis PR introduces end-to-end testing infrastructure for NWC faucet integration. It adds a test helper module for wallet creation with retry logic, a test suite verifying NWCClient balance functionality, separate Jest E2E configuration, and a new npm script for running E2E tests. Changes
Sequence DiagramsequenceDiagram
participant Test as E2E Test Suite
participant Helper as Test Helper
participant Faucet as NWC Faucet
participant Client as NWCClient
participant WS as WebSocket Connection
Test->>Helper: createTestWallet(10000 sats)
loop Retry Logic
Helper->>Faucet: POST /faucet?balance=10000
alt Success
Faucet-->>Helper: NWC URL + lud16 param
Helper->>Helper: Parse URL & extract lightning address
Helper-->>Test: TestWallet {nwcUrl, lightningAddress}
else Retry
Helper->>Helper: Wait & retry
end
end
Test->>Client: new NWCClient(nwcUrl)
Test->>Client: getBalance()
Client->>WS: Connect & send request
WS-->>Client: Balance response (10,000,000 msats)
Client-->>Test: 10,000,000 msats
Test->>Test: Assert balance == expected
Test->>Client: close()
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 |
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?
Contributor
Author
There was a problem hiding this comment.
@rolznz Good catch!
The browser tests in e2e/browser are run by Playwright, not Jest. Jest was only picking up *.test.ts files, while the browser tests use *.spec.ts, so there was no overlap — but that was implicit and fragile. I’ve added e2e/browser to testPathIgnorePatterns so Jest explicitly skips that folder and the split between Jest and Playwright is clear.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/helpers.ts (2)
43-48: Fragile error identity check via string comparison.The rethrow guard on line 45 matches against a hardcoded string literal. If the error message at line 36 is ever changed, this silently breaks — parse errors would resume being retried without any test or type-checker catching it. Using a typed custom error (as was originally suggested) makes the relationship explicit and refactor-safe.
♻️ Proposed refactor — use a custom error class
+class FaucetParseError extends Error {} + export async function createTestWallet( ... if (!lud16Match) { - throw new Error("No lud16 in NWC URL"); + throw new FaucetParseError("No lud16 in NWC URL"); } ... } catch (error) { - if ( - error instanceof Error && - error.message === "No lud16 in NWC URL" - ) { - throw error; - } + if (error instanceof FaucetParseError) throw error;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers.ts` around lines 43 - 48, Replace the fragile string comparison with a typed custom error: define a NoLud16Error class extending Error, throw new NoLud16Error() at the site that currently creates Error("No lud16 in NWC URL"), and change the rethrow guard to check error instanceof NoLud16Error instead of comparing error.message; update any imports/exports so the throw site and the check both reference the same error class (e.g., NoLud16Error) to make the relationship refactor-safe.
23-25: Consider adding a fetch timeout to prevent indefinite hangs.Without an
AbortSignal, a stalled TCP connection will block thefetchpromise forever. The only current safeguard is Jest's test-level timeout, which produces a generic "test timed out" error rather than a descriptive failure.⏱️ Proposed fix — add a per-request timeout
- const response = await fetch(`${FAUCET_URL}?balance=${balanceSats}`, { - method: "POST", - }); + const response = await fetch(`${FAUCET_URL}?balance=${balanceSats}`, { + method: "POST", + signal: AbortSignal.timeout(10_000), + });Note:
AbortSignal.timeout()was added in Node.js 17.3.0. If the project must run on Node 14/16, use anAbortController+setTimeoutpair instead.🤖 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 to `${FAUCET_URL}?balance=${balanceSats}` can hang indefinitely; add a per-request abort timeout by creating an AbortController, start a timer (e.g., setTimeout) that calls controller.abort() after a chosen ms, pass controller.signal into the fetch options, and clear the timer after fetch resolves or rejects; alternatively use AbortSignal.timeout(ms) if the runtime supports it. Ensure the signal is passed into the fetch call that currently only sets method:"POST" and handle the abort error path so tests fail with a descriptive timeout message rather than a generic Jest timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/helpers.ts`:
- Around line 43-48: Replace the fragile string comparison with a typed custom
error: define a NoLud16Error class extending Error, throw new NoLud16Error() at
the site that currently creates Error("No lud16 in NWC URL"), and change the
rethrow guard to check error instanceof NoLud16Error instead of comparing
error.message; update any imports/exports so the throw site and the check both
reference the same error class (e.g., NoLud16Error) to make the relationship
refactor-safe.
- Around line 23-25: The fetch to `${FAUCET_URL}?balance=${balanceSats}` can
hang indefinitely; add a per-request abort timeout by creating an
AbortController, start a timer (e.g., setTimeout) that calls controller.abort()
after a chosen ms, pass controller.signal into the fetch options, and clear the
timer after fetch resolves or rejects; alternatively use AbortSignal.timeout(ms)
if the runtime supports it. Ensure the signal is passed into the fetch call that
currently only sets method:"POST" and handle the abort error path so tests fail
with a descriptive timeout message rather than a generic Jest timeout.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/helpers.tse2e/nwc-faucet.test.tsjest.config.tsjest.e2e.config.tspackage.json
🚧 Files skipped from review as they are similar to previous changes (2)
- jest.e2e.config.ts
- e2e/nwc-faucet.test.ts
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