Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #132: does not sleep when it should, approach 2 #134

Merged
merged 11 commits into from
Aug 9, 2023

Conversation

marco-m-pix4d
Copy link
Contributor

@marco-m-pix4d marco-m-pix4d commented Aug 8, 2023

As mentioned, this is approach 2 at fixing #132. See also approach 1 at #133.

Compared with #133, it keeps the tests running as fast as possible and does not introduce test flakiness since it does not measure time.

To do this, I added an injectable sleep function, which I then substituted with a spy in the tests.

Best reviewed commit-per-commit, since commit 1 adds the repro and commit 2 adds the fix.

Fixes #132.

Copy link
Contributor

@aliculPix4D aliculPix4D left a comment

Choose a reason for hiding this comment

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

So I prefer if we continue with the approach shown in this PR. I am not approving yet only because there is still an issue with un-necessary sleep at max retries and a missing test for it.
I am just puzzled why the test with max retries is missing, when I am 100% sure I had it at the time.

#132: retry GitHub HTTP request when rate limited: does not sleep when
it should

We use ghStatus.SetSleepFn(sleepSpy) to spy on the requests to sleep
for the success case.

This causes, on purpose, all the test cases of
TestGitHubStatusSuccessMockAPI to fail, in the assert

    assert.DeepEqual(t, haveSleeps, tc.wantSleeps)
#132: retry GitHub HTTP request when rate limited: does not sleep when
it should

We use ghStatus.SetSleepFn(sleepSpy) to spy on the requests to sleep
for the failure case.

his causes, on purpose, all the test cases of
TestGitHubStatusFailureMockAPI to fail, in the assert

  assert.DeepEqual(t, haveSleeps, tc.wantSleeps)
Now that we have virtual sleep, we can use in the tests, as much as
possible, the same values of prod. This reduces the cognitive load to
compare tests and prod and convince oneself that it is the correct
behavior.
This is a good example of how they are more explicit and leave no doubt
Parameters rateLimitRemaining and rateLimitReset are not taken into
consideration when the HTTP status code is a transient error.

Since settings these values in the test cases gives the impression that
they influence the test, we remove them for clarity.
Test case "Any other error" covers the same logic of test case
"non transient error, stop at first attempt", so we remove it.
@marco-m-pix4d
Copy link
Contributor Author

marco-m-pix4d commented Aug 9, 2023

@Pix4D/integration: ready for re-review.
Given the refactoring, I suggest to review commit-per-commit; the commit message is there to help understanding the reason of each change.

Thanks @aliculPix4D for pointing out yet another corner case. It has been longer than expected, but I think now that we can trust the tests. I also think that the retry loop is now self-evident.

Using a virtual sleep reduced the test running time from 4.5 sec to circa 1.1 sec, although we went from 215 to 224 test cases :-)

Copy link
Contributor

@aliculPix4D aliculPix4D left a comment

Choose a reason for hiding this comment

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

Pre-approving. I would just handle the case where jitter>0 also.

@marco-m-pix4d marco-m-pix4d merged commit 3f5ac0f into master Aug 9, 2023
2 checks passed
@marco-m-pix4d marco-m-pix4d deleted the fix-shadowing-2 branch August 9, 2023 12:58
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.

retry GitHub HTTP request when rate limited: does not sleep when it should
4 participants