-
Notifications
You must be signed in to change notification settings - Fork 324
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
feat: disable rate limiter in test #6146
Conversation
scheduleGc?: boolean | ||
} | ||
|
||
export class InMemoryRateLimiter { |
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.
I retained some of the changes I made in the original PR so I could just port over the unit test directly.
|
||
let rateLimiter: RateLimiter | ||
let rateLimiter: InMemoryRateLimiter |
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.
I found the name InMemoryRateLimiter a bit clearer, but I can change this back to just RateLimiter
if you'd like.
@@ -14,39 +14,38 @@ interface Options { | |||
perHour: number | |||
} | |||
|
|||
const rateLimit = <TSource = any, TContext = GQLContext, TArgs = any>({ |
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.
Prettier rewrote a bunch of this, I called out the actual change below.
if (process.env.NODE_ENV === 'test') { | ||
return | ||
} |
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.
This is the only change - the requested if
statement to disable in test.
LGTM! |
This PR supersedes #6063. Instead of choosing between a StubRateLimiter and an InMemoryRateLimiter, we just disable it when
NODE_ENV
istest
. See https://github.com/ParabolInc/parabol/pull/6063/files#r814362740 for details.Relates to #6018