-
Notifications
You must be signed in to change notification settings - Fork 85
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
feature: ZENKO-846 Add retry test #233
Conversation
079220b
to
1987b4f
Compare
tests/backbeat/package.json
Outdated
@@ -15,12 +15,13 @@ | |||
"aws-sdk": "2.28.0", | |||
"azure-storage": "^2.10.0", | |||
"@google-cloud/storage": "^1.6.0", | |||
"mocha": "2.3.4", | |||
"mocha": "^5.2.0", |
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.
Need to update mocha version so that we can use this.retries
.
I think test is failing because the get request is made before data is available in redis. We can set a timeout before making the get request |
I agree @philipyoo, there is definitely a race condition there. I've refactored that test to continually make the GET request in an |
tests/backbeat/tests/retry/retry.js
Outdated
const { results } = body.completions; | ||
// We have reached an expiration of metrics. | ||
if (results.count < prevCompletions.count) { | ||
return callback(new Error('Invalid test case')); |
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'm not sure but a bug in the code may well trigger this case (bug in metrics returning less metrics than expected), so instead maybe use a regular assert
?
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.
Very good point. Updated.
tests/backbeat/tests/retry/retry.js
Outdated
assert.strictEqual((failures.results.size - prevFailures.size), 0); | ||
} | ||
|
||
function performRetries(N, keys, done) { |
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.
minor: I think you could get rid of parameter N
and use keys.length
instead
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.
Nice! Updated to remove that param.
722da87
to
af4337f
Compare
af4337f
to
96df558
Compare
Adds the following test cases:
Automating these tests is proving especially challenging since we need to update the backbeat pod such that exponential back-off is very short (i.e. one second), so requires a unique backbeat pod with its configuration updated as such. Also, the test relies on deleting and recreating the destination bucket to create failures. This use of buckets is unreliable when multiple tests are using that same bucket. Ideas for strategies to automate these tests are welcome!