Skip to content

Fix flaky Retry-After Header Test#5943

Merged
knative-prow-robot merged 1 commit into
knative:mainfrom
travis-minke-sap:flaky-retry-after-test
Dec 1, 2021
Merged

Fix flaky Retry-After Header Test#5943
knative-prow-robot merged 1 commit into
knative:mainfrom
travis-minke-sap:flaky-retry-after-test

Conversation

@travis-minke-sap
Copy link
Copy Markdown
Contributor

Fixes #5937
(hopefully - unable to reproduce locally - deducing timing mismatch root cause indirectly ; )

Proposed Changes

  • 🐛 Refactor the RetryAfterValidationServer used in test for tracking request intervals.

Pre-review Checklist

  • At least 80% unit test coverage
  • E2E tests for any new behavior. (n/a changes are to unit test only)
  • Docs PR for any user-facing impact (n/a changes are to unit test only)
  • Spec PR for any new API feature (n/a changes are to unit test only)
  • Conformance test for any change to the spec (n/a RetryAfter support is an alpha level experimental-feature and does not yet have Conformance tests.)

Dev Notes

The test/cases in question have never been seen to fail locally so the root cause is not 100% certain. The changes to the test are an attempt to address the timing concerns in a more robust manner.

The test case in question involves returning the Retry-After headers in the RFC850 date format. This formatting drops/truncates milliseconds and thus requires special handling in the test.

The prior implementation was rounding the request receipt times in an effort to work around this. This is risky as either request (current/previous) might round towards or away from each other making the interval checking difficult.

The new approach instead uses the actual request receipt times, and simply adjusts the acceptable time-window when dealing with these RFC850 date test cases. This should be more robust as it is not impacted by all the various rounding permutations.

@google-cla google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 30, 2021
@knative-prow-robot knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 30, 2021
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 30, 2021

Codecov Report

Merging #5943 (7a90a9b) into main (e73a69b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5943   +/-   ##
=======================================
  Coverage   82.23%   82.23%           
=======================================
  Files         220      220           
  Lines        7572     7572           
=======================================
  Hits         6227     6227           
  Misses        911      911           
  Partials      434      434           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e73a69b...7a90a9b. Read the comment docs.

Copy link
Copy Markdown
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2021
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, travis-minke-sap

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2021
@knative-prow-robot knative-prow-robot merged commit 24e133d into knative:main Dec 1, 2021
@pierDipi
Copy link
Copy Markdown
Member

pierDipi commented Dec 1, 2021

Thanks!

@travis-minke-sap travis-minke-sap deleted the flaky-retry-after-test branch December 1, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[flaky] pkg/kncloudevents.TestHTTPMessageSenderSendWithRetriesWithRetryAfter/small_max_429_with_Retry-After_date

4 participants