Add utility function for creating service policy #5053
Conversation
There was a problem hiding this comment.
The reason why these tests are so complicated is that there are multiple ways for the circuit to break (or not break) depending on the options given. I spent a couple of hours trying to rewrite these tests so that they were simpler, but ended up returning to this structure each time because I feel like it made the most sense thinking about scenarios that would produce the results we care about.
It's probably best to run this command to view the test names from a high level:
yarn workspace @metamask/controller-utils run jest packages/controller-utils/src/create-service-policy.test.ts --no-coverage
| await expect(promise).rejects.toThrow(error); | ||
| }); | ||
|
|
||
| it('does not call the onBreak callback, since the max number of consecutive failures is never reached', async () => { |
There was a problem hiding this comment.
Note that this is probably not what would happen in production code. If the policy is intended to be reused across multiple executions, then eventually onBreak would get called once the total number of attempts exceeds the max number of consecutive failures. The vast majority of these tests only use policy once.
I spent a bit of time attempting to add more tests to address this case and reverting them because it would make this test more difficult to maintain. I figured that this case was already tested within Cockatiel. However, if we feel that they are worth it then I can add them back.
| }); | ||
|
|
||
| describe(`using the default circuit break duration (${DEFAULT_CIRCUIT_BREAK_DURATION})`, () => { | ||
| it('returns what the service returns if it is successfully called again after the circuit break duration has elapsed', async () => { |
There was a problem hiding this comment.
This (and other tests like it) is where we ensure that once the circuit enters a half-open state it can resolve to closed again after the circuit break duration has elapsed.
| }); | ||
|
|
||
| describe('if the initial run + retries is greater than the max number of consecutive failures', () => { | ||
| it('throws a BrokenCircuitError before the service can succeed', async () => { |
There was a problem hiding this comment.
This (and other tests like it) is where we ensure that once the circuit is open, and before the circuit break duration has elapsed, any subsequent executions are essentially refused.
| it('has expected JavaScript exports', () => { | ||
| expect(Object.keys(allExports)).toMatchInlineSnapshot(` | ||
| Array [ | ||
| "createServicePolicy", |
There was a problem hiding this comment.
This is where we are adding the new export.
mcmire
force-pushed
the
extract-create-service-policy
branch
from
963e2fa to
83ca925
Compare
mcmire
changed the title
We would like to use the Cockatiel library in our service classes to ensure that requests are retried using the circuit breaker pattern. Some of our service classes do this already, but we are copying and pasting the code around. This commit extracts the boilerplate code to a new function in the `@metamask/controller-utils` package, `createServicePolicy`, so that we no longer have to do this.
mcmire
force-pushed
the
extract-create-service-policy
branch
from
627b096 to
530bc4c
Compare
| * The maximum number of times that the action is allowed to fail before pausing | ||
| * further retries. | ||
| */ | ||
| export const DEFAULT_MAX_CONSECUTIVE_FAILURES = (1 + DEFAULT_MAX_RETRIES) * 3; |
There was a problem hiding this comment.
Question: Should this be dynamically calculated based on the maxRetries passed? I took this from the existing implementations but I don't know if it was intentional that it was static.
| * ``` | ||
| */ | ||
| export function createServicePolicy({ | ||
| maxRetries = DEFAULT_MAX_RETRIES, |
There was a problem hiding this comment.
Should this be called retries instead of maxRetries? (They both mean the same thing but retries is shorter.) Alternatively, should this be called maxAttempts instead of maxRetries (in which case the user would have to specify 1 more than the number of retries they expect)?
|
This PR is superseded by #5141 (and future PRs). I will leave this open so as not to accidentally remove the branch until those other PRs are created, but will convert to draft so this doesn't get merged. |
Explanation
We would like to use the Cockatiel library in our service classes to ensure that requests are retried using the circuit breaker pattern. Some of our service classes do this already, but we are copying and pasting the code around. This commit extracts the boilerplate code to a new function in the
@metamask/controller-utilspackage,createServicePolicy.References
Progresses #4994.
Changelog
(Updated in PR.)
Checklist