-
Notifications
You must be signed in to change notification settings - Fork 0
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
Speed up RequestLib
tests
#162
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
StuAA78
added
the
housekeeping
Refactoring, tidying up or other work which supports the project
label
Mar 13, 2023
Currently some of our tests for `RequestLib` are slow. This PR aims to fix this.
Cruikshanks
force-pushed
the
speed-up-request-lib-tests
branch
from
December 19, 2023 17:04
02b6f78
to
8ba86ff
Compare
If it helps, comparing the unit test times on this branch compared to others it looks like this change will cut 20ish secs of build time 🤷🎉 |
Cruikshanks
requested review from
Demwunz,
robertparkinson,
Jozzey and
Beckyrose200
December 19, 2023 17:56
robertparkinson
approved these changes
Dec 20, 2023
Jozzey
approved these changes
Dec 20, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DEFRA/water-abstraction-team#98
For those paying attention when running our unit tests, they appear to stall when it comes to
test/lib/request.lib.test.js
. This is because we have tests that ensure the module behaves as expected when requests to other APIs time out or fail.One of the reasons we love Got is because retries are built-in and easy to configure. But that means we have to allow Got to retry failures when running the unit tests.
The reason for the stalling is because the time between retries is based on a 'computed value';
This means the delay increases exponentially. That's great for what we do in the app. A 'blip' when talking to an external API is smoothed over and all the user sees is something takes a few seconds longer than normal. But in our unit tests it causes some to appear to stall.
Thankfully, the brilliant @StuAA78 spotted that we can override the computed value by setting the backoffLimit.
If we set this when running the unit tests the time between retries will be limited to 50ms.
So, this change updates
RequestLib
to make it easy to access the default options it uses so we can then applybackofflimit
to the retry options.Before changes
Average 40077 ms (40 secs)
After changes
Average 16972 ms (16 secs)