-
Notifications
You must be signed in to change notification settings - Fork 805
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
Reduce flakiness on workflow-ID-specific ratelimit test #5986
Conversation
Specifically, some tests are failing with the following.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Pull Request Test Coverage Report for Build 018f5614-bc7b-44eb-a2cf-dea6e7a9db0aDetails
💛 - Coveralls |
…flow#5986) This test had three issues: 1. it's *very* time-sensitive, spending >=200ms across all StartWorkflowExecution calls will allow one (or more) of the "should be limited" calls to succeed. This is now more permissive. 2. it seems to misunderstand / be misleading about how ratelimits work. The reason the first 5 calls can be done "immediately" is due to the burst value, not the RPS itself. We just init the burst with that value. 3. if `assert.ErrorAs` failed, the next line would panic because the error value was nil. `require` would work too, but switching to `if assert.ErrorAs(...) {...}` lets the rest of the checks continue, and it's relatively simple. These are now fixed.
This test had three issues:
assert.ErrorAs
failed, the next line would panic because the error value was nil.require
would work too, but switching toif assert.ErrorAs(...) {...}
lets the rest of the checks continue, and it's relatively simple.These are now fixed.