Add token-based ratelimit IT scenarios for empty token-limit arrays#1235
Conversation
WalkthroughAdds extensive token-based rate-limit test scenarios and a new basic rate-limit feature, a ratelimit overage scenario, and updates many gateway default policy versions; no functional policy logic changes are made. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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.
🧹 Nitpick comments (1)
gateway/it/features/token-based-ratelimit.feature (1)
1389-1389: Consider adding a response body assertion after 429 for consistency.Scenarios at lines 152–153, 692–693, and 997 verify
And the response body should contain "Rate limit exceeded"after each 429. All three new scenarios skip this step. Adding it would close the assertion gap and make the failure mode fully verifiable beyond just the status code.✏️ Suggested addition (same pattern for all three scenarios)
Then the response status code should be 429 + And the response body should contain "Rate limit exceeded"Also applies to: 1498-1498, 1607-1607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/it/features/token-based-ratelimit.feature` at line 1389, Add a response body assertion after the existing "Then the response status code should be 429" step in the feature scenarios that currently stop at the status check (token-based-ratelimit.feature occurrences around the shown diff); append the step 'And the response body should contain "Rate limit exceeded"' immediately after the 429 assertion in each of the affected scenarios so the failure mode is verified by both status and body content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gateway/it/features/token-based-ratelimit.feature`:
- Line 1389: Add a response body assertion after the existing "Then the response
status code should be 429" step in the feature scenarios that currently stop at
the status check (token-based-ratelimit.feature occurrences around the shown
diff); append the step 'And the response body should contain "Rate limit
exceeded"' immediately after the 429 assertion in each of the affected scenarios
so the failure mode is verified by both status and body content.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/it/features/basic-ratelimit.feature (1)
241-318: Commented-out scope-based scenario — consider tracking as a follow-up.This commented-out scenario tests API-level vs route-level policy scope interaction, which is a valuable edge case. If this is intentionally deferred, consider adding a tracking issue or TODO reference so it doesn't get lost.
Would you like me to open an issue to track enabling this scope-based rate limit scenario?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/it/features/basic-ratelimit.feature` around lines 241 - 318, The commented-out scenario "Rate limit scope based on policy attachment level" is a valuable test left disabled; either re-enable it or add a tracking TODO/issue so it isn't lost. Create a short tracking ticket (e.g., "enable scope-based rate limit scenario") and then add a one-line TODO above the commented block referencing that ticket ID (or add a comment with the issue URL and the scenario title) so reviewers can find and prioritize it later; alternatively, if you plan to enable now, uncomment the scenario and ensure test data and limits in the scenario content match current policy behavior before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gateway/it/features/basic-ratelimit.feature`:
- Around line 241-318: The commented-out scenario "Rate limit scope based on
policy attachment level" is a valuable test left disabled; either re-enable it
or add a tracking TODO/issue so it isn't lost. Create a short tracking ticket
(e.g., "enable scope-based rate limit scenario") and then add a one-line TODO
above the commented block referencing that ticket ID (or add a comment with the
issue URL and the scenario title) so reviewers can find and prioritize it later;
alternatively, if you plan to enable now, uncomment the scenario and ensure test
data and limits in the scenario content match current policy behavior before
committing.
| @@ -1,5 +1,5 @@ | |||
| name: advanced-ratelimit | |||
| version: v0.1.4 | |||
| version: v0.3.0 | |||
There was a problem hiding this comment.
We are going with 0.3.0 right?
@Krishanx92 @Tharsanan1
Summary
promptTokenLimitsandcompletionTokenLimitsare explicitly empty arrays429after quota exhaustion in each caseValidation
IT_FEATURE_PATHS=features/token-based-ratelimit.feature make test(13/13 scenarios passed)Summary by CodeRabbit