Skip to content

Tests: add HTTP robust retries test - #121

Open
sbhowmikf5 wants to merge 1 commit into
nginx:masterfrom
sbhowmikf5:http-robust-retries
Open

sbhowmikf5 wants to merge 1 commit into
nginx:masterfrom
sbhowmikf5:http-robust-retries

Conversation

@sbhowmikf5

Copy link
Copy Markdown

Test retries after all upstream peers are exhausted with the "no_live" parameter, including retry limits and delayed retries controlled by "proxy_next_upstream_delay".

Issue - nginx/nginx#1263
Feature PR - nginx/nginx#1757

Checklist

Before creating a PR, run through this checklist and mark each as complete:

  • I have read the [contributing guidelines.
  • I have signed the [F5 Contributor License Agreement (CLA)]
  • If applicable, I have added tests that prove my fix is effective or that my feature works.
  • If applicable, I have checked that any relevant tests pass after adding my changes.

Co-authored-by: Sourav Bhowmik <s.bhowmik@f5.com>

@route443 route443 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @sbhowmikf5 ,
I slightly changed the approach and re-write your tests, adding you as a co-author:

  1. Split the changes into two files, since a new proxy_next_upstream_delay direcive was introduced.
  2. I added gating based on the config, though this will affect the proxy_next_upstream.t as well. I think that's acceptable.
  3. Since we are dealing with delays, splitting the tests into separate files will have a positive impact on CI runtime.
  4. Added comments for the conf as well
  5. Reviewed code coverage and expanded the tests (72% -> 91% by gcov)
  6. Right now (against commits 466f7ba and 5bf8a71), the tests are failing, which is expected. The issues need to be addressed, and in theory, the tests should then pass :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

3 participants