Sleep within activity was too aggressive resulting in test flakiness #635
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
ActivityHeartbeat test sets a 4 second timeout on the activity and then gives it 5 seconds to timeout. This is pretty aggressive specially on a slow machine where it might take server more than a second to timeout activity due to heartbeat timeout. This resulted in flakiness of this test.
markmark206
reviewed
Jul 30, 2020
| if err == nil { | ||
| s.Logger.Error("Activity expected to time out but got completed instead") | ||
| } else { | ||
| s.Logger.Info("Activity processing completed with error", tag.Error(err)) |
There was a problem hiding this comment.
just thinking out loud, i wonder if it is easy / worth it to detect that the error is a timeout (which is expected), and, if so, make that clear in the log message that the failure is expected, something along the lines of
if err == nil {
s.Logger.Error("omg this didn't timeout, unexpected!)
// + maybe fail the test
} else if is_a_timeout(err) {
s.Logger.Info("very legal very cool, timed out, as expected")
} else {
s.Logger.Error("omg failed in unexpected ways!")
// + fail the test
}
markmark206
approved these changes
Jul 30, 2020
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.
What changed?
Increased sleep time in activity handler to 8 seconds to give more time for
activity to timeout.
Why?
ActivityHeartbeat test sets a 4 second timeout on the activity and
then gives it 5 seconds to timeout. This is pretty aggressive
specially on a slow machine where it might take server more than a
second to timeout activity due to heartbeat timeout. This resulted
in flakiness of this test.
How did you test it?
Potential risks