Skip to content

test(sdk): add deterministic SubRosaClient unit tests and include in … - #15

Closed
Ce-Royals wants to merge 2 commits into
Sub-Rosa-Issue:mainfrom
Ce-Royals:Add_isolated_unit_coverage
Closed

test(sdk): add deterministic SubRosaClient unit tests and include in …#15
Ce-Royals wants to merge 2 commits into
Sub-Rosa-Issue:mainfrom
Ce-Royals:Add_isolated_unit_coverage

Conversation

@Ce-Royals

Copy link
Copy Markdown

closes #2

PR description
Title

Add deterministic, network-free unit tests for SubRosaClient and update SDK test script
Summary

Adds a deterministic, network-free unit test suite for SubRosaClient covering state-changing methods, read-only views, conversions, and both direct RPC and injected TransactionSubmitter submission paths. Updates the @sub-rosa/sdk test script so pnpm sdk:test runs the new tests.
Files changed

client.test.ts — new tests for SubRosaClient (createRound, commit, openReveal/reveal, clear/settle/void flows via mocks; read-only views; error wrapping; injected submitter behaviors).
package.json — include src/client.test.ts in the test script so pnpm sdk:test runs the suite.

@drips-wave

drips-wave Bot commented Jun 23, 2026

Copy link
Copy Markdown

@Ce-Royals Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@karagozemin

Copy link
Copy Markdown
Collaborator

Thanks for the PR. I tested the latest head (4cdea8b) in a clean merge simulation against current main, and I cannot merge it yet because pnpm sdk:test hangs/fails.

Command run:

pnpm sdk:test

Observed result: src/client.test.ts stayed pending for over 75s and had to be interrupted. Node then reported:

✖ src/client.test.ts
'Promise resolution is still pending but the event loop has already resolved'

The likely culprit is the timeout test: it sets Date.now to a constant large value while _sleep is a no-op. Since the SDK computes deadline = Date.now() + confirmTimeout, a constant Date.now() never advances past the deadline, so the poll loop never exits. Please make the fake clock advance deterministically or configure a timeout path that actually terminates without real sleeps.

Also, #2 asks for isolated coverage for every SubRosaClient call path. The current suite covers some important pieces, but it still needs more explicit coverage for:

  • openReveal, reveal, clear, settle, and void method wiring
  • default bidder behavior in commit, not only missing-source behavior
  • direct successful signAndSend, not only wrapped failure
  • injected submitter timeout without hanging

After pnpm sdk:test and pnpm sdk:typecheck pass deterministically, I will review again.

@karagozemin

Copy link
Copy Markdown
Collaborator

Thanks for the update. I re-tested the current head (ad7d80c) against current main.

I cannot merge this yet because the PR no longer merges cleanly after the SDK configuration coverage work that landed on main.

Merge simulation result:

  • packages/sdk/package.json has a content conflict.
  • packages/sdk/src/client.test.ts has an add/add conflict.

Please rebase this branch on current main and keep the SDK config validation tests that are already on main. After resolving the overlap, please re-run:

  • pnpm sdk:test
  • pnpm sdk:typecheck

I will review again once the branch merges cleanly and the isolated SubRosaClient call-path tests still pass deterministically.

@Ce-Royals
Ce-Royals force-pushed the Add_isolated_unit_coverage branch from ad7d80c to e16c49c Compare June 29, 2026 14:27
@karagozemin

Copy link
Copy Markdown
Collaborator

Thanks for rebasing and for the deterministic timeout update. I rechecked the current head against #2. The branch is now mergeable, but I still cannot merge because the required SDK checks fail.\n\nCommands run:\n\n- pnpm --filter @sub-rosa/sdk test\n- pnpm --filter @sub-rosa/sdk typecheck\n- git diff --check origin/main...HEAD\n\ngit diff --check passes, but the SDK test command fails in src/client.test.ts:\n\n- commit defaults to the configured bidder when one is not supplied fails with SubRosaClientConfigError: auditor blob must not be empty.\n- direct RPC signAndSend errors are wrapped in SubRosaSubmitError also hits auditor blob must not be empty instead of reaching the mocked submit failure.\n\nThe SDK typecheck also fails with:\n\n- src/client.test.ts(208,27): error TS2339: Property 'operator' does not exist on type 'never'.\n- same never issue for reveal_round, commit_deadline, reveal_deadline, item_ref, and auditor_pubkey.\n\nPlease update the tests to use a non-empty auditorBlob where the test needs to reach commit submission, and adjust the typed capture for seenArgs so the test file typechecks. After those two SDK commands pass, I can re-review again.

VeronicDev pushed a commit to VeronicDev/sub-rosa-drips that referenced this pull request Jul 1, 2026
Landed from Sub-Rosa-Issue#15 by @DevSolex (relayer/phase6 files excluded — unrelated v1 stub).

- Fuzz + invariant tests in contracts/test/foundry/
- Foundry CI job in contracts workflow
- forge-std vendored under contracts/lib/

Co-authored-by: Cursor <cursoragent@cursor.com>
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.

Add isolated unit coverage for every SubRosaClient call path

2 participants