Conversation
📝 WalkthroughWalkthroughThis PR adds test infrastructure for credits functionality, introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
ded4d44 to
c40936a
Compare
c40936a to
15f5957
Compare
12f8c47 to
1bdca94
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/credits/mockledger.go (1)
24-30: Consider usingalpacadecimal.Decimalinstead offloat64for monetary tracking.Using
float64for monetary values (customerCredits,customerPromotionalCredits,invoiceAccruals,receivables) can introduce precision issues over time. While this is test code, usingDecimalwould better mirror production behavior and catch precision-related bugs.That said, for a mock ledger in tests, this might be acceptable if you're okay with the trade-off.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credits/mockledger.go` around lines 24 - 30, Replace the MockLedger's float64 money fields with alpacadecimal.Decimal to avoid precision issues: change customerCredits, customerPromotionalCredits, invoiceAccruals, and receivables from float64 to alpacadecimal.Decimal in the MockLedger struct, update any initialization/assignments and arithmetic in methods that use these fields (e.g., constructors, Add/Charge/Balance helpers) to use Decimal creation/operations, and import the alpacadecimal package so tests mirror production behavior while keeping tests consistent.test/credits/sanity_test.go (2)
404-422: Validation method should be on value receiver for consistency.
createCreditPurchaseIntentInput.Validate()(line 502) uses value receiver, butcreateMockChargeIntentInput.Validate()uses pointer receiver. For consistency (and since neither mutates state), consider using value receivers for both.Proposed change
-func (i *createMockChargeIntentInput) Validate() error { +func (i createMockChargeIntentInput) Validate() error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credits/sanity_test.go` around lines 404 - 422, The Validate method for createMockChargeIntentInput uses a pointer receiver but should use a value receiver for consistency with createCreditPurchaseIntentInput.Validate and because it does not mutate state; change the method signature from func (i *createMockChargeIntentInput) Validate() error to use a value receiver (func (i createMockChargeIntentInput) Validate() error) and keep the validation logic the same, ensuring any references/calls still compile after changing the receiver.
339-343: Make credit realization assertions more explicit about credit types.The test currently relies on array indices to identify credit types—
CreditRealizations[0]for promotional and[1]for customer. While the order is intentional (the handler appends promotional credits first, then customer), the test would be clearer and less fragile by asserting on the credit type directly rather than relying on position:for _, realization := range updatedFlatFeeCharge.State.CreditRealizations { if realization.Type == "promotional" { s.Equal(float64(30), realization.Amount.InexactFloat64()) } else if realization.Type == "customer" { s.Equal(float64(50), realization.Amount.InexactFloat64()) } }This makes the test's intent clearer and less dependent on handler implementation details.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credits/sanity_test.go` around lines 339 - 343, The current assertions index into updatedFlatFeeCharge.State.CreditRealizations which makes the test fragile; change the test to iterate the CreditRealizations slice (updatedFlatFeeCharge.State.CreditRealizations) and assert amounts based on realization.Type (e.g., "promotional" -> 30, "customer" -> 50) so each realization is checked by its Type rather than by position; update the assertions in the test function where promotionalCreditRealization and customerCreditRealization are read to use a loop and conditional checks on realization.Type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/credits/mockledger.go`:
- Around line 142-145: The Reset() on MockLedger currently only zeroes
customerCredits and customerPromotionalCredits but leaves invoiceAccruals and
receivables populated; update MockLedger.Reset to also clear invoiceAccruals and
receivables by reinitializing them to their zero/empty values (e.g., set maps to
nil or make(empty maps) or set slices to nil/empty slices) so no stale data
remains between tests; locate the Reset method on MockLedger and add assignments
that reset invoiceAccruals and receivables alongside customerCredits and
customerPromotionalCredits.
- Around line 57-70: The block can append a zero-amount credit when
totalToAllocate>0 but l.customerCredits==0; update the logic in the loop
handling totalToAllocate and l.customerCredits so you only create a
creditrealization.CreateInput when creditsToAllocate > 0 (i.e., compute
creditsToAllocate := math.Min(totalToAllocate, l.customerCredits) and if
creditsToAllocate <= 0 skip appending), ensure Amount uses
alpacadecimal.NewFromFloat(creditsToAllocate) only for positive values, and
resume correctly adjusting l.customerCredits (and totalToAllocate if that
variable is intended to be decremented) to avoid emitting invalid zero-amount
inputs.
---
Nitpick comments:
In `@test/credits/mockledger.go`:
- Around line 24-30: Replace the MockLedger's float64 money fields with
alpacadecimal.Decimal to avoid precision issues: change customerCredits,
customerPromotionalCredits, invoiceAccruals, and receivables from float64 to
alpacadecimal.Decimal in the MockLedger struct, update any
initialization/assignments and arithmetic in methods that use these fields
(e.g., constructors, Add/Charge/Balance helpers) to use Decimal
creation/operations, and import the alpacadecimal package so tests mirror
production behavior while keeping tests consistent.
In `@test/credits/sanity_test.go`:
- Around line 404-422: The Validate method for createMockChargeIntentInput uses
a pointer receiver but should use a value receiver for consistency with
createCreditPurchaseIntentInput.Validate and because it does not mutate state;
change the method signature from func (i *createMockChargeIntentInput)
Validate() error to use a value receiver (func (i createMockChargeIntentInput)
Validate() error) and keep the validation logic the same, ensuring any
references/calls still compile after changing the receiver.
- Around line 339-343: The current assertions index into
updatedFlatFeeCharge.State.CreditRealizations which makes the test fragile;
change the test to iterate the CreditRealizations slice
(updatedFlatFeeCharge.State.CreditRealizations) and assert amounts based on
realization.Type (e.g., "promotional" -> 30, "customer" -> 50) so each
realization is checked by its Type rather than by position; update the
assertions in the test function where promotionalCreditRealization and
customerCreditRealization are read to use a loop and conditional checks on
realization.Type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 402ce9ef-d808-4f46-9957-9f5a13acab41
📒 Files selected for processing (2)
test/credits/mockledger.gotest/credits/sanity_test.go
Overview
The mock ledger is a bad ledger implementation.
The testcase contains
// LEDGER[galexi]:comments where it is described what happens and which callbacks are invoked.Notes for reviewer
Summary by CodeRabbit
Release Notes