ENG-151: Comprehensive test suite for cash ledger posting pipeline invariants#246
Merged
Connorbelez merged 5 commits intomainfrom Mar 22, 2026
Merged
ENG-151: Comprehensive test suite for cash ledger posting pipeline invariants#246Connorbelez merged 5 commits intomainfrom
Connorbelez merged 5 commits intomainfrom
Conversation
…variants Add 78 tests across 3 new test files, 1 shared utility module, and 2 modified files covering all 9 pipeline steps, 11 entry types, and 6 financial invariants (net-zero CONTROL, non-negative payable, point-in-time replay, idempotent posting, append-only corrections, reversal traceability). New files: - testUtils.ts: shared harness, seeders, constants - postEntry.test.ts: 35 unit tests for pipeline steps - entryTypes.test.ts: 26 tests for all 11 entry types + rejections - financialInvariants.test.ts: 16 tests for 6 financial invariants Modified files: - integration.test.ts: +3 edge case tests (zero, negative, same-account) - constraintsAndBalanceExemption.test.ts: +1 SUSPENSE_ESCALATED exemption Validates REQ-246 through REQ-251. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Connorbelez, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a shared test utilities module and a broad suite of cash-ledger tests (unit, entry-type coverage, financial invariants, and integration edge cases), plus specification docs; only one trivial whitespace edit to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Connorbelez has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Pull request overview
This PR adds a broad Vitest suite to validate the Convex cash ledger posting pipeline across entry types and several financial invariants, plus shared testing utilities to reduce duplication.
Changes:
- Added a shared cash-ledger test harness/utilities module and three new test suites (pipeline step unit tests, entry-type coverage, and financial invariants).
- Expanded existing cash-ledger integration/constraint tests with additional edge cases.
- Minor formatting cleanup in dispersal entry creation.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-151/tasks.md | Planning checklist for ENG-151 test coverage work. |
| specs/ENG-151/chunks/manifest.md | Chunk/task manifest for the ENG-151 work breakdown. |
| specs/ENG-151/chunks/chunk-01-shared-test-utils/tasks.md | Tasks for shared test utilities chunk. |
| specs/ENG-151/chunks/chunk-01-shared-test-utils/context.md | Context/spec for shared test utilities. |
| specs/ENG-151/chunks/chunk-02-pipeline-unit-tests/tasks.md | Tasks for pipeline unit test chunk. |
| specs/ENG-151/chunks/chunk-02-pipeline-unit-tests/context.md | Context/spec for pipeline unit test chunk. |
| specs/ENG-151/chunks/chunk-03-entry-type-coverage/tasks.md | Tasks for entry-type coverage chunk. |
| specs/ENG-151/chunks/chunk-03-entry-type-coverage/context.md | Context/spec for entry-type coverage chunk. |
| specs/ENG-151/chunks/chunk-04-financial-invariants/tasks.md | Tasks for financial invariants chunk. |
| specs/ENG-151/chunks/chunk-04-financial-invariants/context.md | Context/spec for financial invariants chunk. |
| specs/ENG-151/chunks/chunk-05-existing-mods-verification/tasks.md | Tasks for modifying existing tests + verification. |
| specs/ENG-151/chunks/chunk-05-existing-mods-verification/context.md | Context/spec for existing test modifications. |
| convex/payments/cashLedger/tests/testUtils.ts | New shared cash-ledger test utilities (harness, seeders, account/entry helpers). |
| convex/payments/cashLedger/tests/postEntry.test.ts | New unit tests for key postCashEntryInternal pipeline steps and behaviors. |
| convex/payments/cashLedger/tests/entryTypes.test.ts | New tests posting all entry types + rejection coverage. |
| convex/payments/cashLedger/tests/financialInvariants.test.ts | New tests intended to validate higher-level financial invariants and replay/idempotency properties. |
| convex/payments/cashLedger/tests/integration.test.ts | Added integration-level edge case tests for invalid post inputs. |
| convex/payments/cashLedger/tests/constraintsAndBalanceExemption.test.ts | Added a SUSPENSE_ESCALATED balance-check exemption test. |
| convex/dispersal/createDispersalEntries.ts | Removed an extra blank line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…sertion - testUtils.ts: re-read account after db.patch to return fresh object - constraintsAndBalanceExemption.test.ts: import shared constants from testUtils - integration.test.ts: import shared constants/harness from testUtils - financialInvariants.test.ts: replace tautological double-entry assertion with proper per-account debit/credit sum comparison Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
convex/payments/cashLedger/__tests__/testUtils.ts (1)
199-214: Returns stale account after patching initial balances.When
initialDebitBalanceorinitialCreditBalanceare provided, the account is patched but the original (pre-patch)accountobject is returned. Callers would seecumulativeDebits: 0n/cumulativeCredits: 0ninstead of the patched values, which could lead to confusing test assertions.♻️ Proposed fix to refetch after patch
if ( spec.initialDebitBalance !== undefined || spec.initialCreditBalance !== undefined ) { await ctx.db.patch(account._id, { ...(spec.initialDebitBalance !== undefined && { cumulativeDebits: spec.initialDebitBalance, }), ...(spec.initialCreditBalance !== undefined && { cumulativeCredits: spec.initialCreditBalance, }), }); + const updated = await ctx.db.get(account._id); + if (!updated) { + throw new Error("Failed to refetch account after patch"); + } + return updated; } return account;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/cashLedger/__tests__/testUtils.ts` around lines 199 - 214, The returned account is stale after you call ctx.db.patch to set spec.initialDebitBalance/spec.initialCreditBalance; after patching (in the block that updates cumulativeDebits/cumulativeCredits) refetch the updated record (e.g., via ctx.db.get(account._id) or the equivalent DB read used elsewhere in this file) and return that fresh account object instead of the original pre-patch account so callers see the patched balances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/payments/cashLedger/__tests__/constraintsAndBalanceExemption.test.ts`:
- Around line 343-373: The test currently can pass even if SUSPENSE_ESCALATED
exemption regresses because BORROWER_RECEIVABLE is already family-exempt and
SUSPENSE isn't forced negative; modify the test so SUSPENSE has a negative
starting balance before calling postCashEntryInternal for "SUSPENSE_ESCALATED"
(e.g., create a prior entry via postCashEntryInternal that produces a negative
balance on the suspenseAccount or use whatever helper/param on
getOrCreateCashAccount to set an initial negative balance), then assert the
SUSPENSE_ESCALATED post succeeds only because the entry-type exemption is
honored; target the test "SUSPENSE_ESCALATED skips balance check (like
REVERSAL/CORRECTION)" and the functions getOrCreateCashAccount and
postCashEntryInternal when making this change.
In `@specs/ENG-151/chunks/chunk-01-shared-test-utils/context.md`:
- Around line 4-8: The chunk references a helper file named testUtils.test.ts
but the implemented module is convex/payments/cashLedger/__tests__/testUtils.ts;
fix by choosing one canonical name and making both the doc and code match:
either rename the implemented file to
convex/payments/cashLedger/__tests__/testUtils.test.ts, or update every
reference in this chunk (including the require/import mentions around lines
noted and the usages in context.md) to
convex/payments/cashLedger/__tests__/testUtils.ts so imports and the doc are
consistent; ensure the filename/extension you pick is used everywhere in the
chunk and in any require/import calls (e.g., test utils imports in the
cashLedger tests).
---
Nitpick comments:
In `@convex/payments/cashLedger/__tests__/testUtils.ts`:
- Around line 199-214: The returned account is stale after you call ctx.db.patch
to set spec.initialDebitBalance/spec.initialCreditBalance; after patching (in
the block that updates cumulativeDebits/cumulativeCredits) refetch the updated
record (e.g., via ctx.db.get(account._id) or the equivalent DB read used
elsewhere in this file) and return that fresh account object instead of the
original pre-patch account so callers see the patched balances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1847b940-4fe5-4bcb-8985-c520add9a366
📒 Files selected for processing (19)
convex/dispersal/createDispersalEntries.tsconvex/payments/cashLedger/__tests__/constraintsAndBalanceExemption.test.tsconvex/payments/cashLedger/__tests__/entryTypes.test.tsconvex/payments/cashLedger/__tests__/financialInvariants.test.tsconvex/payments/cashLedger/__tests__/integration.test.tsconvex/payments/cashLedger/__tests__/postEntry.test.tsconvex/payments/cashLedger/__tests__/testUtils.tsspecs/ENG-151/chunks/chunk-01-shared-test-utils/context.mdspecs/ENG-151/chunks/chunk-01-shared-test-utils/tasks.mdspecs/ENG-151/chunks/chunk-02-pipeline-unit-tests/context.mdspecs/ENG-151/chunks/chunk-02-pipeline-unit-tests/tasks.mdspecs/ENG-151/chunks/chunk-03-entry-type-coverage/context.mdspecs/ENG-151/chunks/chunk-03-entry-type-coverage/tasks.mdspecs/ENG-151/chunks/chunk-04-financial-invariants/context.mdspecs/ENG-151/chunks/chunk-04-financial-invariants/tasks.mdspecs/ENG-151/chunks/chunk-05-existing-mods-verification/context.mdspecs/ENG-151/chunks/chunk-05-existing-mods-verification/tasks.mdspecs/ENG-151/chunks/manifest.mdspecs/ENG-151/tasks.md
💤 Files with no reviewable changes (1)
- convex/dispersal/createDispersalEntries.ts
- Rename "nets to zero" test to accurately describe one-sided debit accumulation behavior (all entries debit CONTROL, none credit it) - Sort entries by sequenceNumber before asserting monotonicity to avoid flaky failures when index returns timestamp-ordered results Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update chunk-01 context.md to reference testUtils.ts (not .test.ts). The .ts extension is required because Biome noExportsInTest prohibits exports from .test.ts files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SUSPENSE_ESCALATED tests were not proving the balance-check exemption because debiting a debit-normal SUSPENSE account from zero can't make it negative. Now seed SUSPENSE with cumulativeCredits: 100_000n so the balance is genuinely negative and posting would fail without the exemption. Applied to constraintsAndBalanceExemption.test.ts, postEntry.test.ts, and entryTypes.test.ts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 24, 2026
Merged
Merged
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
…variants (#246) Add 78 tests across 3 new test files, 1 shared utility module, and 2 modified files covering all 9 pipeline steps, 11 entry types, and 6 financial invariants (net-zero CONTROL, non-negative payable, point-in-time replay, idempotent posting, append-only corrections, reversal traceability). New files: - testUtils.ts: shared harness, seeders, constants - postEntry.test.ts: 35 unit tests for pipeline steps - entryTypes.test.ts: 26 tests for all 11 entry types + rejections - financialInvariants.test.ts: 16 tests for 6 financial invariants Modified files: - integration.test.ts: +3 edge case tests (zero, negative, same-account) - constraintsAndBalanceExemption.test.ts: +1 SUSPENSE_ESCALATED exemption Validates REQ-246 through REQ-251. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded test coverage for cash ledger operations, including input validation, idempotency behavior, and financial invariant verification across entry types and balance constraints. * Added shared test utilities and fixtures to support cash ledger testing. * **Code Quality** * Minor formatting cleanup. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add 78 tests across 3 new test files, 1 shared utility module, and 2 modified
files covering all 9 pipeline steps, 11 entry types, and 6 financial invariants
(net-zero CONTROL, non-negative payable, point-in-time replay, idempotent
posting, append-only corrections, reversal traceability).
New files:
Modified files:
Validates REQ-246 through REQ-251.
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit
Tests
Documentation
Code Quality