test(js): add smoke tests for web bindings#1544
Conversation
Coverage Report for CI Build 25693470746Coverage remained the same at 85.294%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
xstoicunicornx
left a comment
There was a problem hiding this comment.
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.
9207722 to
ec752f2
Compare
|
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 ( 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? |
xstoicunicornx
left a comment
There was a problem hiding this comment.
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_KEYSandORIGINAL_PSBTto 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?
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
test/unit.web.test.tswith smoke tests for the web bindings, covering:package.jsonto include the new test file in thetestscriptApproach
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
.wasmbinary directly viareadFileSyncand pass it to theinitAsyncfunction, bypassing the Vite-specific asset import. This allows the existingnode:test+tsxinfrastructure 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:
AI
in the body of this PR.