feat: jest-e2e#324
Open
DSanich wants to merge 4 commits into
Open
Conversation
Add a separate Jest config and e2e/ suite that exercises package entry points with jest-fetch-mock. Unit test runs exclude e2e; CI runs yarn test:e2e after yarn test.
Cover fetch402 routing (plain, L402, MPP, x402), fetchWithX402 and fetchWithMpp happy paths, LightningAddress with proxy disabled (generateInvoice, zapInvoice, boost), and sendBoostagram. Extend barrel smoke exports; add JSON fixtures for well-known and callback responses.
Add Invoice LNURL-verify success path, fetch402 Basic auth and maxAmount guard failures, LightningAddress.zap with WebLN, and smoke imports via @getalby/lightning-tools/* using tsconfig.e2e paths plus Jest moduleNameMapper and ts-jest transform.
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis pull request adds comprehensive end-to-end test infrastructure with test suites validating payment protocols (L402, X402, MPP), LNURL functionality, fiat utilities, and package exports. It includes Jest E2E configurations, test fixtures in JSON format, updated CI workflows, and updated build tooling configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
e2e/fixtures/lightning-address-details.json (1)
2-25: Consider reducing duplicated fixture payloads to avoid drift.Lines 2-25 duplicate data already present in dedicated fixture files. Composing this from canonical fixtures in test setup would reduce maintenance mismatches over time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/fixtures/lightning-address-details.json` around lines 2 - 25, This fixture duplicates the "lnurlp" and "keysend" payloads; instead of hardcoding those objects in lightning-address-details.json, refactor the test setup to compose this fixture from the canonical fixtures (e.g., the shared lnurlp fixture and keysend fixture) so the "lnurlp" and "keysend" properties are sourced from those single-source fixtures; update whatever loader/creator (test bootstrap or fixture builder used by your tests) to merge the canonical lnurlp and keysend objects into the lightning-address-details structure and remove the duplicated JSON block to prevent drift..github/workflows/ci.yml (1)
22-22: Nice addition—consider isolating e2e in a dedicated CI job.Running
yarn test:e2eon Line 22 is correct. As a follow-up, a separate job can improve parallelism and make unit-vs-e2e failures easier to triage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 22, Separate the end-to-end test command `yarn test:e2e` into its own CI job rather than running it inline with the existing job: create a new job (e.g., "e2e") in the workflow that runs the same setup steps required (checkout, install/cache deps, build if needed) and then executes `yarn test:e2e`, and configure job-level dependencies or concurrency (using needs: or separate runners) so unit tests can run in parallel and failures are isolated; update the original job to remove the `run: yarn test:e2e` step and, if desired, add a conditional or needs relationship between the unit and e2e jobs.e2e/fiat.package.test.ts (1)
15-23: Assert fetch target URLs to prevent false-positive passes.Right now the test validates response handling but not endpoint routing.
🔧 Proposed test hardening
test('currencies list then satoshi conversion for 1 USD', async () => { fetchMock.mockResponseOnce(fixture('fiat-currencies.json')) const currencies = await getFiatCurrencies() + expect(fetchMock).toHaveBeenNthCalledWith(1, 'https://getalby.com/api/rates') expect(currencies.map((c) => c.code)).toEqual(['USD', 'EUR']) expect(currencies.find((c) => c.code === 'BTC')).toBeUndefined() fetchMock.mockResponseOnce(fixture('fiat-usd-rate.json')) const sats = await getSatoshiValue({ amount: 1, currency: 'USD' }) + expect(fetchMock).toHaveBeenNthCalledWith(2, 'https://getalby.com/api/rates/usd.json') const ratePerSat = 100000 / 100_000_000 expect(sats).toBe(Math.floor(1 / ratePerSat)) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/fiat.package.test.ts` around lines 15 - 23, The test currently only supplies mock responses via fetchMock.mockResponseOnce but doesn't assert that the code calls the expected endpoints, which can hide routing errors; update the test around getFiatCurrencies and getSatoshiValue to assert the fetch target URLs (e.g., check fetchMock calls or use fetchMock.mockIf/matcher) so you verify the request was made to the expected fiat list and USD rate endpoints before/after invoking getFiatCurrencies and getSatoshiValue; reference the existing fetchMock.mockResponseOnce usage and the functions getFiatCurrencies and getSatoshiValue when adding assertions to ensure the correct URLs were requested.e2e/podcasting-boost.package.test.ts (1)
40-42: Strengthen record assertion to exact payload serialization.
stringContaining('value_msat')is too permissive for regression detection.🔧 Proposed test hardening
customRecords: expect.objectContaining({ - '7629169': expect.stringContaining('value_msat') + '7629169': JSON.stringify(boost) }) as Record<string, string> })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/podcasting-boost.package.test.ts` around lines 40 - 42, The test currently uses expect.objectContaining({ '7629169': expect.stringContaining('value_msat') }) which is too permissive; replace the stringContaining assertion for the '7629169' key with an assertion that matches the exact serialized payload expected (the full string value that should be present in customRecords['7629169']), keeping the outer shape via expect.objectContaining if needed – locate the customRecords assertion in e2e/podcasting-boost.package.test.ts and update the expect for the '7629169' entry to compare against the exact expected serialized payload string (ensuring types still satisfy the cast to Record<string,string>).e2e/lnurl-without-proxy.package.test.ts (2)
23-45: Consider extracting a shared fetch router mock helper.These blocks duplicate near-identical URL routing logic; consolidating into one helper will reduce drift and simplify future fixture updates.
Also applies to: 66-96, 117-139, 190-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/lnurl-without-proxy.package.test.ts` around lines 23 - 45, Extract the duplicated fetch mock routing into a shared helper (e.g., createFetchRouterMock or mockFetchRoutes) that accepts a mapping of URL predicates to Response fixtures and returns a function suitable for fetchMock.mockImplementation; replace each inline fetchMock.mockImplementation block (the ones calling requestUrl and fixture and returning Responses for '/.well-known/lnurlp/', '/.well-known/keysend/', 'nostr.json', '/lnurlp/hello/callback', etc.) with calls to this helper so tests at lines referenced reuse the same router logic and fixtures.
4-5: Use package entrypoints to keep this e2e test honest.At Line 5, importing from
../src/lnurlbypasses the package export surface, so export-map breakages can slip through while this test still passes.Proposed change
-import { LightningAddress } from '../src/lnurl' +import { LightningAddress } from '@getalby/lightning-tools/lnurl'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/lnurl-without-proxy.package.test.ts` around lines 4 - 5, The test currently imports internal modules directly (Event and LightningAddress) which bypasses the package export surface; update the imports so the e2e test uses the public package entrypoints instead of ../src paths: replace the import of LightningAddress (and the Event type) from internal src with imports from the package's public exports (the package root or the published lnurl entrypoint) so the test fails if the export-map is broken; locate the references to Event and LightningAddress in the e2e/lnurl-without-proxy.package.test.ts and switch them to the package's public import paths.e2e/fetch402.package.test.ts (2)
2-8: Avoid internal-source and shared-builder coupling in this e2e suite.At Lines 2-8, this test both bypasses package entrypoints and reuses production challenge builders, which can mask regressions (parser + builder changing together). Prefer package-path imports for
fetch402and static/canonical header fixtures in test code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/fetch402.package.test.ts` around lines 2 - 8, Replace direct internal-source imports and production builders with package entrypoint imports and static test fixtures: stop importing fetch402, makeL402AuthenticateHeader, encodeMppChargeRequest, makeMppWwwAuthenticateHeader or MppChargeRequest from src paths; instead import fetch402 via the package/public entrypoint export and create canonical, hard-coded header and MPP payload fixtures in the test (static strings/byte arrays) so the test asserts parser behavior independently of builder/encoder changes; update test imports to use the package export for fetch402 and remove use of makeL402AuthenticateHeader/encodeMppChargeRequest/makeMppWwwAuthenticateHeader, replacing their outputs with fixed fixture values.
44-50: Replace legacy base64 encoding withBufferfor modern Node.js compatibility.The
btoa()andunescape()pattern at lines 44–50 uses deprecated/non-standard globals. Node.js does not provideunescape()as a global, andbtoa()is a legacy compatibility layer. UseBufferinstead for cleaner, idiomatic code:Proposed change
function makePaymentRequiredHeader() { - return btoa( - unescape( - encodeURIComponent(JSON.stringify({ accepts: [X402_REQUIREMENTS] })) - ) - ) + return Buffer.from( + JSON.stringify({ accepts: [X402_REQUIREMENTS] }), + 'utf8' + ).toString('base64') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/fetch402.package.test.ts` around lines 44 - 50, The makePaymentRequiredHeader function uses legacy btoa/unescape/encodeURIComponent to base64-encode JSON; replace that with Buffer-based encoding to be Node.js-compatible: build the payload via JSON.stringify({ accepts: [X402_REQUIREMENTS] }) and encode it using Buffer.from(payload).toString('base64'), then return that value from makePaymentRequiredHeader.
🤖 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/fiat.package.test.ts`:
- Line 4: The test is importing internal source via a relative path
(getFiatCurrencies, getSatoshiValue) which bypasses package export resolution;
update the import to use the package's public subpath (the consumer-facing
export) instead of '../src/fiat/fiat' so the test validates the published API
surface (replace the relative import with the package subpath that exports
getFiatCurrencies and getSatoshiValue).
In `@e2e/index.smoke.test.ts`:
- Line 1: The test imports the internal module via "import * as pkg from
'../src/index'", which bypasses the package entrypoint; change the import to
pull from the package root (e.g., "import * as pkg from '..'" or the package
name) so the smoke test validates the real public entrypoint behavior; update
the import statement in e2e/index.smoke.test.ts to reference the package root
instead of ../src/index.
In `@e2e/invoice-verify.package.test.ts`:
- Line 4: Replace the source-relative import "import { Invoice } from
'../src/bolt11'" in the e2e test with the package export entry (e.g. import {
Invoice } from 'your-package' or 'your-package/bolt11') so the test exercises
published export/subpath resolution; update the test import to use the package
name and, if using a subpath, ensure the package.json "exports" covers that
subpath so the symbol Invoice resolves via the package entrypoints rather than
the ../src path.
In `@e2e/podcasting-boost.package.test.ts`:
- Line 1: The test is importing the internal source module '../src/podcasting2'
instead of the package consumer entrypoint; replace that local import with the
package subpath (the public consumer entrypoint) and keep the named import
sendBoostagram so the test validates the published entrypoint (i.e., change the
import that references podcasting2 to import sendBoostagram from the package's
public subpath/entry module instead).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 22: Separate the end-to-end test command `yarn test:e2e` into its own CI
job rather than running it inline with the existing job: create a new job (e.g.,
"e2e") in the workflow that runs the same setup steps required (checkout,
install/cache deps, build if needed) and then executes `yarn test:e2e`, and
configure job-level dependencies or concurrency (using needs: or separate
runners) so unit tests can run in parallel and failures are isolated; update the
original job to remove the `run: yarn test:e2e` step and, if desired, add a
conditional or needs relationship between the unit and e2e jobs.
In `@e2e/fetch402.package.test.ts`:
- Around line 2-8: Replace direct internal-source imports and production
builders with package entrypoint imports and static test fixtures: stop
importing fetch402, makeL402AuthenticateHeader, encodeMppChargeRequest,
makeMppWwwAuthenticateHeader or MppChargeRequest from src paths; instead import
fetch402 via the package/public entrypoint export and create canonical,
hard-coded header and MPP payload fixtures in the test (static strings/byte
arrays) so the test asserts parser behavior independently of builder/encoder
changes; update test imports to use the package export for fetch402 and remove
use of
makeL402AuthenticateHeader/encodeMppChargeRequest/makeMppWwwAuthenticateHeader,
replacing their outputs with fixed fixture values.
- Around line 44-50: The makePaymentRequiredHeader function uses legacy
btoa/unescape/encodeURIComponent to base64-encode JSON; replace that with
Buffer-based encoding to be Node.js-compatible: build the payload via
JSON.stringify({ accepts: [X402_REQUIREMENTS] }) and encode it using
Buffer.from(payload).toString('base64'), then return that value from
makePaymentRequiredHeader.
In `@e2e/fiat.package.test.ts`:
- Around line 15-23: The test currently only supplies mock responses via
fetchMock.mockResponseOnce but doesn't assert that the code calls the expected
endpoints, which can hide routing errors; update the test around
getFiatCurrencies and getSatoshiValue to assert the fetch target URLs (e.g.,
check fetchMock calls or use fetchMock.mockIf/matcher) so you verify the request
was made to the expected fiat list and USD rate endpoints before/after invoking
getFiatCurrencies and getSatoshiValue; reference the existing
fetchMock.mockResponseOnce usage and the functions getFiatCurrencies and
getSatoshiValue when adding assertions to ensure the correct URLs were
requested.
In `@e2e/fixtures/lightning-address-details.json`:
- Around line 2-25: This fixture duplicates the "lnurlp" and "keysend" payloads;
instead of hardcoding those objects in lightning-address-details.json, refactor
the test setup to compose this fixture from the canonical fixtures (e.g., the
shared lnurlp fixture and keysend fixture) so the "lnurlp" and "keysend"
properties are sourced from those single-source fixtures; update whatever
loader/creator (test bootstrap or fixture builder used by your tests) to merge
the canonical lnurlp and keysend objects into the lightning-address-details
structure and remove the duplicated JSON block to prevent drift.
In `@e2e/lnurl-without-proxy.package.test.ts`:
- Around line 23-45: Extract the duplicated fetch mock routing into a shared
helper (e.g., createFetchRouterMock or mockFetchRoutes) that accepts a mapping
of URL predicates to Response fixtures and returns a function suitable for
fetchMock.mockImplementation; replace each inline fetchMock.mockImplementation
block (the ones calling requestUrl and fixture and returning Responses for
'/.well-known/lnurlp/', '/.well-known/keysend/', 'nostr.json',
'/lnurlp/hello/callback', etc.) with calls to this helper so tests at lines
referenced reuse the same router logic and fixtures.
- Around line 4-5: The test currently imports internal modules directly (Event
and LightningAddress) which bypasses the package export surface; update the
imports so the e2e test uses the public package entrypoints instead of ../src
paths: replace the import of LightningAddress (and the Event type) from internal
src with imports from the package's public exports (the package root or the
published lnurl entrypoint) so the test fails if the export-map is broken;
locate the references to Event and LightningAddress in the
e2e/lnurl-without-proxy.package.test.ts and switch them to the package's public
import paths.
In `@e2e/podcasting-boost.package.test.ts`:
- Around line 40-42: The test currently uses expect.objectContaining({
'7629169': expect.stringContaining('value_msat') }) which is too permissive;
replace the stringContaining assertion for the '7629169' key with an assertion
that matches the exact serialized payload expected (the full string value that
should be present in customRecords['7629169']), keeping the outer shape via
expect.objectContaining if needed – locate the customRecords assertion in
e2e/podcasting-boost.package.test.ts and update the expect for the '7629169'
entry to compare against the exact expected serialized payload string (ensuring
types still satisfy the cast to Record<string,string>).
🪄 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: df79e44b-e800-4c2a-948b-aa5ccb36989b
📒 Files selected for processing (26)
.github/workflows/ci.yml.gitignoree2e/fetch402-edges.package.test.tse2e/fetch402.package.test.tse2e/fiat.package.test.tse2e/fixtures/fiat-currencies.jsone2e/fixtures/fiat-usd-rate.jsone2e/fixtures/generate-invoice.jsone2e/fixtures/keysend-well-known.jsone2e/fixtures/lightning-address-details.jsone2e/fixtures/lnurlp-callback-invoice.jsone2e/fixtures/lnurlp-pay-request.jsone2e/fixtures/nostr-well-known.jsone2e/index.smoke.test.tse2e/invoice-verify.package.test.tse2e/l402.package.test.tse2e/lnurl-bolt11.package.test.tse2e/lnurl-without-proxy.package.test.tse2e/mpp.package.test.tse2e/podcasting-boost.package.test.tse2e/subpath-exports.smoke.test.tse2e/x402.package.test.tsjest.config.tsjest.e2e.config.tspackage.jsontsconfig.e2e.json
Align integration tests with consumer entry points (package.json exports) per CodeRabbit; keep ../src only where no public subpath exists (e.g. mpp test helpers). Made-with: Cursor
Author
|
@rolznz Hi! I've added e2e tests. I've covered the main modules. I'd appreciate your feedback. |
Member
|
Hi @DSanich Sorry, I appreciate your effort, but this is a bit difficult to review as such a large PR. I also do not know what guarantee that things actually work if we are using these fixtures and mocks. We already have quite a few unit tests in this project. |
Author
|
@rolznz Thanks for feedback! I added this as a small integration layer on top of your unit tests, same public imports consumers use and a few happy paths. Mocks only mean “wiring + exports stay correct,” not that live APIs work I not trying to replace your existing unit coverage or real-network checks. Sorry if a big PR took a lot of your time to review, I can break it down into smaller ones. |
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 Jest-based integration (“e2e”) test suite for
@getalby/lightning-tools, aligned with how consumers import the package: real entry points, Node environment, andjest-fetch-mockfor HTTP. Unit tests (yarn test) stay separate and do not run files undere2e/. CI runsyarn test:e2eafteryarn test.This is not browser automation (no Playwright); it exercises library flows end-to-end at the module boundary.
What’s included
jest.e2e.config.ts— dedicated Jest config,testMatch: e2e/**/*.test.ts, sharedsetupJest.ts.tsconfig.e2e.json— extends the main TS config; addspathsso tests can import@getalby/lightning-tools/*like downstream apps; used by ts-jest +moduleNameMapperin the e2e Jest config.package.json— scripttest:e2e; Prettier globs extended fore2e/and e2e-related config files.jest.config.ts—testPathIgnorePatternsincludese2e/so defaultjestdoes not run integration tests twice..github/workflows/ci.yml— runyarn test:e2eafteryarn test(build unchanged)..gitignore—playwright-report/,test-results/(harmless if present locally).Test coverage (high level)
indexexports.package.jsonexports(bolt11,fiat,lnurl,402,402/l402,402/x402,402/mpp,podcasting).LightningAddress+ invoice);proxy: falsewith URL-routed mocks (generateInvoice,zapInvoice,zap,boost).getFiatCurrencies+getSatoshiValue.fetchWithL402,fetch402(L402 / MPP / x402 routing + unsupportedWWW-Authenticate+maxAmountguard),fetchWithX402,fetchWithMpp.Invoice.verifyPaymentsuccess (LNURL verify).sendBoostagramwith mocked WebLN.Fixtures live under
e2e/fixtures/.Summary by CodeRabbit