test(trading): stop mocking suggestSlippageBps out of its own test

[gomesalexandre]

Does what it says on the box, mostly test-only, production code is unchanged.

What's wrong

suggestSlippageBps.test.ts mocked @cowprotocol/sdk-order-book's getQuoteAmountsWithCosts — that export doesn't exist anywhere in this codebase. The real function is getQuoteAmountsAndCosts (called at suggestSlippageBps.ts:35). The mock never connected to the code under test.

It also mocked percentageToBps as percent * 100, where the real function (packages/common/src/utils/math.ts:13) multiplies by 10_000n — it treats its input as a portion of 1, not an already-scaled percentage.

With getSlippagePercent, suggestSlippageFromFee and suggestSlippageFromVolume also fully mocked, the only logic any of the 7 existing tests actually exercised was the final Math.max(Math.min(...), lowerCap) clamp in suggestSlippageBps.ts:76. The isSell computation (suggestSlippageBps.ts:30) — which every one of those mocked functions consumes — was never actually tested.

Proof, mutation-check style: inverting isSell to quote.quote.kind !== OrderKind.SELL and running the existing suite: all 7 tests still passed.

Is this masking a real bug?

No — checked explicitly. getSlippagePercent's docstring states it "Returns a percentage as a portion of 1", and the real percentageToBps multiplies by 10_000, so 0.005 (0.5%) correctly becomes 50 bps. Production is self-consistent and correct; the two test mocks' errors (wrong function name, wrong scale) happened to cancel out in the old hardcoded expectations without ever touching real code.

Fix

Stops mocking the functions actually under test. getQuoteAmountsAndCosts, suggestSlippageFromFee, suggestSlippageFromVolume, getSlippagePercent and percentageToBps are all pure, deterministic, I/O-free — they now run for real.

Adds two end-to-end cases (SELL and BUY) with expected bps values hand-derived from source and confirmed by execution:

• SELL, sellAmount 100e18/1e18 fee → 100 bps
• BUY, sellAmount 50e18/1e18 fee → 150 bps (the buy branch had zero prior coverage — this is the case that closes the isSell blind spot)

Keeps the clamp-boundary tests (Lower/Upper bound clamping) using a jest.spyOn(slippageUtils, 'getSlippagePercent') — legitimate isolation for testing Math.max/Math.min behavior specifically — now using the real portion-of-1 convention (0.01 for 1%, not 1) so expected bps values actually match what the real percentageToBps would produce.

Receipts

Mutation check re-run against the new suite:

$ (invert isSell) && npx jest suggestSlippageBps.test.ts
✕ computes the real slippage for a BUY order
  Expected: 150
  Received: 148
Tests: 1 failed, 9 passed, 10 total

The BUY case now genuinely catches it, where none of the old 7 caught it at all.

$ npx jest suggestSlippageBps.test.ts   # unmutated
Tests: 10 passed, 10 total

$ npx jest   # full trading package
Test Suites: 22 passed, 22 total
Tests: 2 skipped, 258 passed, 260 total

$ npx tsc --noEmit -p .
(clean)

$ npx eslint packages/trading/src/suggestSlippageBps.test.ts
(clean)

$ npx prettier --check packages/trading/src/suggestSlippageBps.test.ts
All matched files use Prettier code style!

suggestSlippageBps.ts itself is untouched — git diff --stat shows only the test file changed.

Codex review

Attempted a synchronous adversarial Codex pass; it hit a transient "model at capacity" error twice in a row (not a stall — exited cleanly both times, first attempt had already done real investigation confirming module resolution and compiled dist output before erroring). Falling back to self-review, backed by the empirical evidence above rather than just reasoning:

• Hand-derived expected values (100/150) are confirmed by the real code actually running and producing them, not just arithmetic on paper.
• The jest.spyOn module-resolution concern is directly proven, not assumed: all 7 clamp tests pass with results matching the specific mocked values (-0.01, 0.0001, 0.01, 0.02, 1.5, 2, 1) — if the spy weren't intercepting the call suggestSlippageBps.ts makes, those tests would get the real computed slippage instead and almost certainly fail on the hardcoded expectations.
• Full package suite (22/22, 258 passing) confirms nothing else in the package depended on this file's old structure.

Summary by CodeRabbit

• Tests
  • Expanded slippage coverage with end-to-end scenarios for buying and selling.
  • Added validation for distinct buy and sell results, including expected slippage values.
  • Improved boundary testing for minimum and maximum slippage limits.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 6d24a919-c1a0-4152-9b10-99f2e6a7f7e3

📥 Commits

Reviewing files that changed from the base of the PR and between f1eddc8 and 23f3554.

📒 Files selected for processing (1)

• packages/trading/src/suggestSlippageBps.test.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The slippage tests now run real calculation paths for SELL and BUY quotes. They add a quote factory and use targeted spies for clamping tests. The suite verifies lower bounds, EthFlow defaults, upper bounds, and distinct results for both order kinds.

Changes

Slippage calculation test coverage

Layer / File(s)	Summary
Test data and calculation setup packages/trading/src/suggestSlippageBps.test.ts	The tests remove broad module mocks, add a reusable quote factory, import OrderKind and slippage utilities, and set buyTokenDecimals to 6.
Unmocked SELL and BUY calculations packages/trading/src/suggestSlippageBps.test.ts	The tests execute real slippage calculations and expect 100 BPS for SELL, 150 BPS for BUY, and different results between order kinds.
Lower and upper bound clamping packages/trading/src/suggestSlippageBps.test.ts	The tests spy on getSlippagePercent and verify lower-bound, EthFlow default, and 10000 BPS upper-bound behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 23f35

This test-only change exercises real SELL and BUY slippage calculations and preserves clamp coverage without changing production behavior. No actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: the test stops mocking suggestSlippageBps within its own test and adds the trading scope.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}