test(wasm-sdk): fix flaky tokenPaymentInfo document balance assertions#3771
Conversation
The tokenPaymentInfo document-flow tests asserted on a single proof-verified token-balance read taken immediately after a state transition. Those reads can be served by a node whose view briefly lags the block that committed the transition, so the nightly functional run failed intermittently on different lines with stale balances. Replace the immediate assertions with a polling helper that retries until the balance converges (or times out). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a polling helper ChangesToken Balance Polling in Document Tests
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
✅ Review complete (commit 96dc5f6) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/wasm-sdk/tests/functional/transitions/documents.spec.ts (1)
36-49: ⚡ Quick winConsider enhancing timeout error message for easier debugging.
The helper correctly polls until the balance converges or timeout expires. However, when a timeout occurs, the subsequent
expect(actual).to.equal(expected)assertion doesn't indicate that polling happened or how long it took. This can make debugging flaky tests more difficult, especially ifgetSingleTokenBalancereturnsundefined.♻️ Proposed enhancement with descriptive timeout error
const expectTokenBalance = async ( identityId: string, tokenId: string, expected: bigint, { timeoutMs = 30000, intervalMs = 500 } = {}, ) => { const deadline = Date.now() + timeoutMs; + const startTime = Date.now(); let actual = await getSingleTokenBalance(identityId, tokenId); while (actual !== expected && Date.now() < deadline) { await new Promise((resolve) => { setTimeout(resolve, intervalMs); }); actual = await getSingleTokenBalance(identityId, tokenId); } - expect(actual).to.equal(expected); + expect(actual).to.equal( + expected, + `Token balance did not converge after ${Date.now() - startTime}ms (expected: ${expected}, actual: ${actual})`, + ); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/wasm-sdk/tests/functional/transitions/documents.spec.ts` around lines 36 - 49, The expectTokenBalance helper currently retries via getSingleTokenBalance but on timeout the plain expect(actual).to.equal(expected) offers no context; update expectTokenBalance to throw or assert with a descriptive message when the deadline is exceeded (include identityId, tokenId, expected, actual, timeoutMs and elapsed time) so test failures show that polling timed out and the last observed value and duration; keep the polling loop and only replace the final expect call with an assertion that includes this contextual message (referencing expectTokenBalance and getSingleTokenBalance to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/wasm-sdk/tests/functional/transitions/documents.spec.ts`:
- Around line 36-49: The expectTokenBalance helper currently retries via
getSingleTokenBalance but on timeout the plain expect(actual).to.equal(expected)
offers no context; update expectTokenBalance to throw or assert with a
descriptive message when the deadline is exceeded (include identityId, tokenId,
expected, actual, timeoutMs and elapsed time) so test failures show that polling
timed out and the last observed value and duration; keep the polling loop and
only replace the final expect call with an assertion that includes this
contextual message (referencing expectTokenBalance and getSingleTokenBalance to
locate the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29bc5a1c-7199-4daf-92e5-aa6ea126e3c2
📒 Files selected for processing (1)
packages/wasm-sdk/tests/functional/transitions/documents.spec.ts
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Test-only change replaces immediate token-balance assertions with a 30s/500ms polling helper to address a documented read-after-write race in token balance proofs. Both agents found no in-scope defects. CodeRabbit's nitpick about the unhelpful timeout failure message is valid but low-priority and does not block.
💬 1 nitpick(s)
A timeout in the balance-convergence poll is the most likely failure mode, but it previously surfaced as a bare equality mismatch with no context. Attach the identity, token, expected value, elapsed time, and last read to the assertion message so a flaky CI failure is triageable from the log. Addresses CodeRabbit/thepastaclaw review nitpick on #3771; polling logic unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta is a one-commit, test-only follow-up that adds triage context to the expectTokenBalance timeout assertion in packages/wasm-sdk/tests/functional/transitions/documents.spec.ts. The prior nitpick from beced7f is FIXED in substance: timeout failures now include identityId, tokenId, expected value, elapsed ms, and last observed value. No new in-scope defects were identified at HEAD 96dc5f6; both Claude and Codex agree.
shumkov
left a comment
There was a problem hiding this comment.
Kept the polling helper (mirrors the platform test suite's waitForBalanceToChange) over a larger waitForPlatform sleep — the SDK already waits for ST execution, so the flake is a post-commit cross-node read lag that polling rides out without paying a fixed wait on every read. Nitpick about timeout triage context addressed in 96dc5f6. Approving.
Issue being fixed or feature implemented
The nightly
Testsworkflow (Packages functional tests / Run functional tests→Run WASM SDK functional tests) has been failing intermittently for several scheduled runs, e.g. run 26697658517.The failures are in the
tokenPaymentInfo document flowtests inpackages/wasm-sdk/tests/functional/transitions/documents.spec.ts. They are flaky, not deterministic — different nightly runs fail on different lines with different stale values:expected 911n to equal 916n, line 619expected 916n to equal 917nexpected 45n to equal 41n, line 619expected 905n to equal 917nThese tests pass on push-triggered runs only because the changed-package gating skips the WASM SDK functional suite there; the scheduled run exercises everything.
What was done?
Root cause: each balance check was a single proof-verified
getIdentityTokenBalancesread taken immediately after a document state transition. That balance proof can be served by a node whose view briefly lags the block that committed the transition, so an immediate read intermittently returns a stale balance. The existingwaitForPlatform()sleep sat after the assertions, so it never protected them.Replaced the immediate
expect(await getSingleTokenBalance(...)).to.equal(x)assertions with a newexpectTokenBalance(...)helper that polls the balance until it converges to the expected value (500ms interval, 30s timeout) before asserting. On a clean read it returns immediately; under lag it waits instead of failing.How Has This Been Tested?
Change is test-only and mechanical (assertion sites rewritten to the polling helper; the expected values are unchanged). The expected balances were already correct — the suite passes on most runs — so converging on them removes the read-after-write race. Verified the full diff; no production code is touched.
Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit