[Backport 1.3] Add signal/wait model for TestAuditlogImpl (#1758) #1796
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
* Add signal/wait model for TestAuditlogImpl I have been tracking test failures with testRestMethod very often showing failures. My theory is that the execution environment can impact the order of operations sometimes causing the audit log not to contain messages before it is checked. Adding a new method `doThenWaitForMessages(...)` this ensures the log queue is fresh, the triggering action completes, and the expected number of messages were recieved. There is a second long time window that allows for the messages to be flushed, this is likely more than enough - if the messages are recieved the count down latch immediately continues execution so the tests will not wait if they are ready to proceed. While this new method is much more reliable not all tests were encountering such issues, so I've keep the original convention. This can be migrated in one-offs or all at once if we see more troublesome behavoir. The previous methods/fields are depreciated to push future tests to follow the new pattern. Modifications to the rest helper not to throw exceptions were needed to keep the Runnable declaration clean and small. Signed-off-by: Peter Nied <[email protected]> (cherry picked from commit e213e9c)
Codecov Report
@@ Coverage Diff @@
## 1.3 #1796 +/- ##
============================================
- Coverage 64.64% 64.62% -0.03%
+ Complexity 3222 3220 -2
============================================
Files 247 247
Lines 17363 17363
Branches 3086 3086
============================================
- Hits 11225 11221 -4
- Misses 4590 4594 +4
Partials 1548 1548
Continue to review full report at Codecov.
|
|
@opensearch-project/security I've backported this PR to 1.3 as we will likely have many more patch releases and this will ensure our tests are more reliable. Lets discuss if anyone has concerns. |
|
I see "Files changed: 0". Did these changes already merge into 1.3 via another PR? |
|
@davidlago You are right, you know what I give up on this change. I'll live with flakier tests here for the moment. |
2 tasks
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.
Description
I have been tracking test failures with testRestMethod very often
showing failures. My theory is that the execution environment can
impact the order of operations sometimes causing the audit log not to
contain messages before it is checked.
Adding a new method
doThenWaitForMessages(...)this ensures thelog queue is fresh, the triggering action completes, and the expected
number of messages were recieved. There is a second long time window
that allows for the messages to be flushed, this is likely more than
enough - if the messages are recieved the count down latch immediately
continues execution so the tests will not wait if they are ready to
proceed.
While this new method is much more reliable not all tests were
encountering such issues, so I've keep the original convention. This
can be migrated in one-offs or all at once if we see more troublesome
behavoir. The previous methods/fields are depreciated to push future
tests to follow the new pattern.
Modifications to the rest helper not to throw exceptions were needed to
keep the Runnable declaration clean and small.
Signed-off-by: Peter Nied [email protected]
(cherry picked from commit e213e9c)
Issues Resolved
Check List
New functionality includes testingNew functionality has been documentedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.