Skip to content

e2e: ratelimit test not rely on the count of request#8472

Merged
zirain merged 3 commits intoenvoyproxy:mainfrom
zirain:e2e/ratelimit
Mar 13, 2026
Merged

e2e: ratelimit test not rely on the count of request#8472
zirain merged 3 commits intoenvoyproxy:mainfrom
zirain:e2e/ratelimit

Conversation

@zirain
Copy link
Copy Markdown
Member

@zirain zirain commented Mar 10, 2026

fixes: #8360 #8419

Signed-off-by: zirain <zirain2009@gmail.com>
@zirain zirain requested a review from a team as a code owner March 10, 2026 06:04
@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 10, 2026

Deploy Preview for cerulean-figolla-1f9435 ready!

Name Link
🔨 Latest commit 4afa7ec
🔍 Latest deploy log https://app.netlify.com/projects/cerulean-figolla-1f9435/deploys/69b345fc05aae400086d7a7e
😎 Deploy Preview https://deploy-preview-8472--cerulean-figolla-1f9435.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@zirain zirain changed the title e2e: ratelimit test not reply the count of request e2e: ratelimit test not rely on the count of request Mar 10, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.12%. Comparing base (a470576) to head (4afa7ec).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8472      +/-   ##
==========================================
- Coverage   74.13%   74.12%   -0.02%     
==========================================
  Files         242      242              
  Lines       37579    37603      +24     
==========================================
+ Hits        27859    27873      +14     
- Misses       7778     7785       +7     
- Partials     1942     1945       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: zirain <zirain2009@gmail.com>
Copy link
Copy Markdown
Member

@rudrakhp rudrakhp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a proper E2E without checking request count? If we expect rate limit config to work and just expect it to be there in the proxy, why not simply verify existence of rate limit header? So only MakeRequestAndExpectEventuallyConsistentResponseExceptErrors call is sufficient?

@zirain
Copy link
Copy Markdown
Member Author

zirain commented Mar 10, 2026

Is it a proper E2E without checking request count? If we expect rate limit config to work and just expect it to be there in the proxy, why not simply verify existence of rate limit header? So only MakeRequestAndExpectEventuallyConsistentResponseExceptErrors call is sufficient?

expectOkResp.Response.Headers["X-Ratelimit-Limit"] = "3, 3;w=3600" make sure that the ratelimit service return the correct ratelimit setting, then check 429 response code to make sure that rate limit happen in real.

rudrakhp
rudrakhp previously approved these changes Mar 10, 2026
Copy link
Copy Markdown
Member

@rudrakhp rudrakhp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess its better to check 429 behavior, but I am still not sure if disregarding count for rate limit tests will be fine for a proper e2e. I am ok with having this before we have a better solution.
@envoyproxy/gateway-maintainers wdyt?

Comment thread test/e2e/tests/ratelimit.go Outdated
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain zirain merged commit 13bb3cf into envoyproxy:main Mar 13, 2026
57 of 59 checks passed
@zirain zirain deleted the e2e/ratelimit branch March 13, 2026 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FAIL: TestE2E/RateLimitMethodMatch

3 participants