Add tests for search top anomaly results API #325
Conversation
…rt request/response Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #325 +/- ##
============================================
+ Coverage 75.87% 77.70% +1.83%
- Complexity 3950 4020 +70
============================================
Files 295 295
Lines 17182 17188 +6
Branches 1812 1816 +4
============================================
+ Hits 13036 13356 +320
+ Misses 3315 2987 -328
- Partials 831 845 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Are you planning on adding any integration tests with security |
Not as part of this PR. Seems there is a lot of missing API tests for security so seems it would be beneficial to do all of these together as a separate effort. |
src/test/java/org/opensearch/ad/transport/SearchTopAnomalyResultActionTests.java
Show resolved
Hide resolved
src/test/java/org/opensearch/ad/transport/SearchTopAnomalyResultActionTests.java
Show resolved
Hide resolved
src/test/java/org/opensearch/ad/transport/SearchTopAnomalyResultResponseTests.java
Show resolved
Hide resolved
Signed-off-by: Tyler Ohlsen <[email protected]>
| if (indexExists(CommonName.ANOMALY_RESULT_INDEX_ALIAS)) { | ||
| deleteIndex(CommonName.ANOMALY_RESULT_INDEX_ALIAS); | ||
| } | ||
| TestHelpers.createEmptyAnomalyResultIndex(client()); |
There was a problem hiding this comment.
Seems this is not the normal process from user's perspective. We don't allow user to create default AD result index directly. Actually ANOMALY_RESULT_INDEX_ALIAS is alias not index.
How about change to run historical analysis to create AD result index, then search top anomaly result?
There was a problem hiding this comment.
The purpose of this test was about having the index exist, but not return any results, so I just create the empty index so the API can query it, but still return empty results. Because this API doesn't interact with anything other than reading the results index, I still think this is ok. Same strategy is used in the below tests, as well as the existing search results tests.
I still prefer this than to run a historical analysis, so as to keep the test more lightweight, faster, and not increase the chances of it becoming flaky. Let me know if you still have concerns.
There was a problem hiding this comment.
Got it. Sounds good.
Another way to generate an empty default result index is run historical analysis on a short period (so it can run quickly), then delete all results with delete result REST API. But like you said, these steps will make the test heavier and more flaky likely.
Signed-off-by: Tyler Ohlsen <[email protected]>
Signed-off-by: Tyler Ohlsen <[email protected]>
Description
Add unit and integ tests for search top anomaly results API. Meets the jacoco coverage bar for all classes added as part of the API, removed from the exclusion list in
build.gradle.Check List
By 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.