Skip to content

test(js): add smoke tests for web bindings#1544

Open
caarloshenriq wants to merge 2 commits into
payjoin:masterfrom
caarloshenriq:test/js-web-bindings
Open

test(js): add smoke tests for web bindings#1544
caarloshenriq wants to merge 2 commits into
payjoin:masterfrom
caarloshenriq:test/js-web-bindings

Conversation

@caarloshenriq
Copy link
Copy Markdown
Contributor

@caarloshenriq caarloshenriq commented May 8, 2026

Closes #1526

This PR adds smoke tests for the JavaScript web bindings (ESModule/WASM target), which were missing after the web bindings were introduced in #1513.

What was done

  • Added test/unit.web.test.ts with smoke tests for the web bindings, covering:
    • URI parsing
    • Receiver/sender persistence (sync and async)
    • Receiver cancellation (sync and async)
    • Input validation
  • Updated package.json to include the new test file in the test script

Approach

The web bindings use WASM and are designed for browser/bundler environments (Vite). To test them in Node.js without introducing new dependencies, the tests load the .wasm binary directly via readFileSync and pass it to the initAsync function, bypassing the Vite-specific asset import. This allows the existing node:test + tsx infrastructure to be reused.

Notes

I used Claude Sonnet 4.6 to help me understand the codebase.

Pull Request Checklist

Please confirm the following before requesting review:

@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented May 8, 2026

Coverage Report for CI Build 25975151451

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.004%) to 85.297%

Details

  • Coverage increased (+0.004%) from the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • 7 coverage regressions across 1 file.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

7 previously-covered lines in 1 file lost coverage.

File Lines Losing Coverage Coverage
payjoin/src/core/receive/mod.rs 7 97.79%

Coverage Stats

Coverage Status
Relevant Lines: 13671
Covered Lines: 11661
Line Coverage: 85.3%
Coverage Strength: 395.75 hits per line

💛 - Coveralls

@spacebear21 spacebear21 requested a review from xstoicunicornx May 8, 2026 21:24
Copy link
Copy Markdown
Collaborator

@xstoicunicornx xstoicunicornx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Since it is possible to load the web wasm in this way could we instead load the web wasm in the existing test files and run the tests for each side by side? Maybe worth using Jest so we can run tests for each payjoin instance without duplicating code? This will make it a lot easier to keep our tests in sync. For example the sender async persistence test seems to be missing from your unit test.

Additionally we probably want integration tests to be in scope as well.

@caarloshenriq caarloshenriq force-pushed the test/js-web-bindings branch from 9207722 to ec752f2 Compare May 11, 2026 19:49
@caarloshenriq
Copy link
Copy Markdown
Contributor Author

Thanks for the feedback! I've updated the PR to address your points.

Code duplication: The web WASM is now loaded directly in unit.test.ts, and both nodejs and web bindings run the same tests side by side using a shared runUnitTests helper. The separate unit.web.test.ts file has been removed. The sender async persistence test that was missing is now included as well.

Jest: The current approach uses node:test + tsx, which are already present in the project and work well with ESM on Node 24 without any extra configuration. I didn't introduce Jest to avoid adding a new dependency and because node:test handles the "run the same tests for multiple providers" pattern cleanly with a simple loop. That said, I'm open to migrating to Jest if the team prefers it, the main benefit would be a richer API (test.each, describe.each) and wider ecosystem familiarity, at the cost of some configuration overhead since Jest's ESM support still requires --experimental-vm-modules.

Integration tests for web: I investigated this and found a path forward. The corepc_node library already exposes rpc_url() and get_cookie_values(), so we could add accessors to RpcClient in test-utils/src/lib.rs to expose the URL and credentials. This would allow a TypeScript FetchRpcClient to connect to the same bitcoind instance and run the integration tests for the web target as well. Would you prefer I implement this in this PR, or track it as a followup?

Copy link
Copy Markdown
Collaborator

@xstoicunicornx xstoicunicornx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I like the implementation but have few small feedback items:

  • Could you rebase against master? #1553 touched the in memory persisters in the FFI bindings
  • I really liked how you pulled out the OHTTP_KEYS and ORIGINAL_PSBT to be re-usable constants at the top, but I think the refactor to eliminate assignment of one time use variables hurts the readability a lot. Agree that the tests are a bit overly verbose, but IMO this is a good thing as it adds clarity to what is happening can allow the tests to act as clear, explicit documentation on usage. Also unnecessary refactors make it harder to review PR changes. Would like to see the unnecessary refactors removed (but please keep the ones regarding reused constants).
  • Love that there is pretty much no code duplication

Good call not using Jest, I agree.

About integration tests, I don't have strong preference on whether to add to this PR or track as follow up, its up to you. I wasn't quite following what the blocker is for web tests using the same testUtils.RpcClient as the existing nodejs tests do though. Whats the issue you're trying to resolve?

@caarloshenriq caarloshenriq force-pushed the test/js-web-bindings branch from ec752f2 to 84ec691 Compare May 14, 2026 23:54
@caarloshenriq
Copy link
Copy Markdown
Contributor Author

  • Could you rebase against master? ffi: Extract shared InMemory persister helpers #1553 touched the in memory persisters in the FFI bindings
  • I really liked how you pulled out the OHTTP_KEYS and ORIGINAL_PSBT to be re-usable constants at the top, but I think the refactor to eliminate assignment of one time use variables hurts the readability a lot. Agree that the tests are a bit overly verbose, but IMO this is a good thing as it adds clarity to what is happening can allow the tests to act as clear, explicit documentation on usage. Also unnecessary refactors make it harder to review PR changes. Would like to see the unnecessary refactors removed (but please keep the ones regarding reused constants).
  • Love that there is pretty much no code duplication

Rebase: Done against master, picking up the changes from #1553.

Readability: Removed the unnecessary refactors, one-time-use variables are back. Kept OHTTP_KEYS and ORIGINAL_PSBT as reusable constants.

Integration tests for web: Implemented in this PR. The blocker I was thinking of turned out not to be a blocker at all, testUtils.RpcClient works fine for both targets since it runs in Node.js regardless of which payjoin binding is under test. The integration tests now run testFfiValidation and testIntegrationV2ToV2 for both nodejs and web bindings using the same RpcClient infrastructure.

The main change needed to support both bindings was passing payjoin as a parameter to all functions that used instanceof checks, so each function uses the correct module's classes for its comparisons.

@xstoicunicornx
Copy link
Copy Markdown
Collaborator

This sounds great! But it looks like somethings a bit off with the integration test in the updated commit, it seems to have just become the same as the unit tests?

@caarloshenriq caarloshenriq force-pushed the test/js-web-bindings branch from 84ec691 to 1b80afd Compare May 15, 2026 16:58
@caarloshenriq
Copy link
Copy Markdown
Contributor Author

But it looks like somethings a bit off with the integration test in the updated commit, it seems to have just become the same as the unit tests?

Sorry about the noise in the previous commit, the integration test file got accidentally overwritten with the unit test contents during an amend. Fixed now.

Copy link
Copy Markdown
Collaborator

@xstoicunicornx xstoicunicornx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Really like how you retrofitted everything so theres literally no duplicated code! Only hard blocker for me to approve is adding back the comments that were stripped from the integration test. Rest I just think would be nice to do.

Also just some general feedback, the refactor commit should probably be in a separate commit and not mashed in with the changes that contain substance. Right now the diff is terribly confused on how the old and new versions match up because its confusing refactor changes with the actual changes. This makes review more tedious and prone to errors.

Comment thread payjoin-ffi/javascript/test/integration.test.ts
Comment thread payjoin-ffi/javascript/test/integration.test.ts Outdated
Comment thread payjoin-ffi/javascript/test/integration.test.ts
Comment thread payjoin-ffi/javascript/test/integration.test.ts
@caarloshenriq
Copy link
Copy Markdown
Contributor Author

caarloshenriq commented May 17, 2026

All feedback addressed in 1b80afd. Restored the stripped comments, simplified the test runner to avoid the .then() chain, and moved webPayjoin to module scope for naming consistency with nodejsPayjoin.

Regarding the commit structure feedback, I agree the refactor and substance changes ended up mixed together. I will keep that in mind for future PRs

Copy link
Copy Markdown
Collaborator

@xstoicunicornx xstoicunicornx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 449d535

Covers web bindings with all the same test vectors as the nodejs bindings already have while also not introducing duplicative code that adds maintenance burden. Reviewed and tested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Javascript Web Bindings Need Tests

3 participants