Add some retry delay logic to Search integration tests to hopefully deflake them#1774
Closed
Add some retry delay logic to Search integration tests to hopefully deflake them#1774
Conversation
fisx
approved these changes
Sep 16, 2021
akshaymankar
requested changes
Sep 16, 2021
Comment on lines
-285
to
+293
| results <- searchResults <$> executeSearch brig searcher searchedWord | ||
| let resultUIds = map contactQualifiedId results | ||
| let expectedOrder = [handleMatch, handlePrefixMatch] | ||
| let dbg = "results: " <> show results <> "\nsearchedWord: " <> cs searchedWord | ||
| -- possibly there is some delay in refreshing of the index writes being | ||
| -- visible for subsequent reads, so we try this a few times. | ||
| results <- aFewTimes 12 (toIds <$> executeSearch brig searcher searchedWord) (== expectedOrder) | ||
| liftIO $ | ||
| assertEqual | ||
| ("Expected order: handle match, handle prefix match.\n\nSince this test fails sporadically for unknown reasons here here is some debug info:\n" <> dbg) | ||
| "If this test fails again, maybe we should consider deleting it and living with some more uncertainty" | ||
| expectedOrder | ||
| resultUIds | ||
| results |
Member
There was a problem hiding this comment.
This test is not broken anymore. The comment is stale.
Comment on lines
+267
to
+274
| -- possibly there is some delay in refreshing of the index writes being | ||
| -- visible for subsequent reads, so we try this a few times. | ||
| results <- aFewTimes 12 (toIds <$> executeSearch brig searcher searchedWord) (== expectedOrder) | ||
| liftIO $ | ||
| assertEqual | ||
| ("Expected order: name match, name prefix match.\n\nSince this test fails sporadically for unknown reasons here is some debug info:\n" <> dbg) | ||
| "If this test fails again, maybe we should consider deleting it and living with some more uncertainty" | ||
| expectedOrder | ||
| resultUIds | ||
| results |
Member
There was a problem hiding this comment.
This won't actually fix the test. The problem is in the production code. So, if you really want to fix it by running again, you should generate the name again.
pcapriotti
added a commit
that referenced
this pull request
Sep 16, 2021
This sorts the result list after the fact by putting exact handle and name searches at the beginning, so we don't have to rely on ES being precise with the ordering of exact matches. Hopefully this fixes the flakyness of the `order-name` integration test, and makes #1774 unnecessary.
4 tasks
Member
Author
|
Fair enough 😄 , closing this in favour of #1776 |
4 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Another flaky instance of this test was seen on the CI run from #1770, quoted here
Perhaps using retry over a maximum of 4 seconds alleviates this problem somewhat. We care about eventual consistency I suppose, not immediate consistency.
Checklist
changelog.d