Conversation
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Swaps the JS client test runtime from a locally-spawned solana-test-validator (driven by @solana/kit-plugin-rpc / solanaLocalRpc) to in-process LiteSVM via @solana/kit-plugin-litesvm. The Makefile no longer starts/stops a validator around pnpm test, the now-unused scripts/restart-test-validator.sh and scripts/stop-test-validator.sh are deleted along with their README section, and advanceNonceAccount.test.ts calls client.svm.expireBlockhash() to compensate for LiteSVM not advancing blockhashes on its own.
Clean, well-scoped change — tests should now run without any external process, which is a nice DX and CI win.
Things to watch out for
client.svmescape hatch: the nonce test now reaches into a LiteSVM-specific handle on the client. That's fine and pragmatic, but it couples this one test to the LiteSVM plugin; if you ever want to run these tests against a real RPC again, that line will need to be guarded or abstracted. Probably not worth doing now — just worth being aware of.- CI cleanup (nit):
.github/workflows/main.yml'stest_client_jsjob still setssolana: truein the setup action, which installs the Solana CLI. With the validator gone frommake test-js, that input looks unnecessary now and could be dropped in this PR or a follow-up to speed up the job. - Lockfile churn: the
pnpm-lock.yamldiff is almost entirely the expected platform-specific native binary entries forlitesvm(litesvm-{darwin,linux}-{arm64,x64}[-gnu|-musl]) plus the new plugin resolution. Nothing unexpected.
Notes for subsequent reviewers
- Worth a quick local
make test-json a clean checkout to confirm nothing else implicitly relied on an RPC atlocalhost:8899. - Confirm the
NonceBlockhashNotExpiredbehaviour described in the new comment matches LiteSVM's actual semantics (i.e. thatexpireBlockhash()is the intended API and there isn't a more idiomatic "advance slot/block" call preferred by the plugin). - No changeset needed here — this is a test-infra / tooling change with no user-facing surface.
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review: the only delta since my last pass is dropping solana: true from the setup action in both .github/workflows/main.yml (test_client_js) and .github/workflows/publish-js-client.yml (publish_js) — which addresses the CI nit I flagged, and sensibly extends it to the publish workflow too. No more Solana CLI install on jobs that don't need it.
Rest of the diff is unchanged from the previous review. Still LGTM — approving.
joncinque
left a comment
There was a problem hiding this comment.
Very nice! Simple and clean
This PR removes the need for a local validator in the JS client tests by using LiteSVM as a test framework. It does this by simply replacing the
solanaLocalRpcplugin with thelitesvmone.This PR also removes the test validator scripts since they are no longer needed by any command in the Makefile.