test: stabilize cost discount spec - #1938
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughTest refactoring in costs.spec.ts to improve discount-related test coverage. Introduces centralized mock management via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
Stabilizes calculateCosts unit tests by removing reliance on seeded DB discount state and explicitly controlling discount behavior via a Vitest module mock.
Changes:
- Mock
@llmgateway/db#getEffectiveDiscountincosts.spec.tsusing a hoisted mock to avoid module-load ordering issues. - Provide a default mock implementation that preserves hardcoded (model/provider mapping) discounts.
- Add an explicit assertion for a 10% OpenAI discount scenario.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -133,33 +151,31 @@ describe("calculateCosts", () => { | |||
| }); | |||
|
|
|||
| it("should apply discount when model has discount field", async () => { | |||
There was a problem hiding this comment.
The test name says "when model has discount field", but the test is actually validating that a discount returned by getEffectiveDiscount (DB/global provider discount) is applied. Renaming the test (and/or adjusting the description) would avoid confusion about what behavior is under test.
| it("should apply discount when model has discount field", async () => { | |
| it("should apply discount returned by getEffectiveDiscount (e.g., global provider discount)", async () => { |
| beforeEach(() => { | ||
| vi.resetAllMocks(); | ||
| vi.mocked(mockGetEffectiveDiscount).mockImplementation( | ||
| async (_organizationId, _provider, _model, hardcodedDiscount = 0) => ({ |
There was a problem hiding this comment.
The default mock implementation for getEffectiveDiscount only declares 4 parameters, but the real function takes a 5th (providerModelName). It works because extra args are ignored, but matching the full signature (even if unused) will keep the mock aligned with production code and make call expectations clearer.
| async (_organizationId, _provider, _model, hardcodedDiscount = 0) => ({ | |
| async ( | |
| _organizationId, | |
| _provider, | |
| _model, | |
| hardcodedDiscount = 0, | |
| _providerModelName, | |
| ) => ({ |
Summary
Verification
Summary by CodeRabbit