Skip to content
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

Adding more unit tests for validation API and some model classes #335

Merged
merged 9 commits into from
Jan 7, 2022

Conversation

amitgalitz
Copy link
Member

Description

Added more unit testing for validation API as well as for some model classes that either didn't have any tests or were missing a unit test.

Split PR into two separate ones. This first one has all the additional validation testing, including some additional model testing. Second PR will have more integration tests, more tests in ad.model package and other files.

Check List

  • Commits are signed per the DCO using --signoff

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.

@amitgalitz amitgalitz requested a review from a team December 3, 2021 17:53
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2021

Codecov Report

Merging #335 (0f2c7a8) into main (e5294e6) will increase coverage by 1.66%.
The diff coverage is 34.78%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #335      +/-   ##
============================================
+ Coverage     77.54%   79.21%   +1.66%     
- Complexity     4018     4093      +75     
============================================
  Files           295      295              
  Lines         17188    17207      +19     
  Branches       1816     1826      +10     
============================================
+ Hits          13329    13631     +302     
+ Misses         3017     2676     -341     
- Partials        842      900      +58     
Flag Coverage Δ
plugin 79.21% <34.78%> (+1.66%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...a/org/opensearch/ad/model/ADEntityTaskProfile.java 57.77% <6.25%> (+48.53%) ⬆️
...rg/opensearch/ad/constant/CommonErrorMessages.java 97.91% <100.00%> (+0.13%) ⬆️
...a/org/opensearch/ad/model/DetectorProfileName.java 64.28% <100.00%> (+7.14%) ⬆️
...opensearch/ad/model/IntervalTimeConfiguration.java 100.00% <100.00%> (ø)
.../handler/AbstractAnomalyDetectorActionHandler.java 60.59% <100.00%> (+6.79%) ⬆️
.../java/org/opensearch/ad/util/RestHandlerUtils.java 81.25% <100.00%> (ø)
...ava/org/opensearch/ad/task/ADTaskCacheManager.java 88.36% <0.00%> (-0.96%) ⬇️
.../main/java/org/opensearch/ad/ml/CheckpointDao.java 69.55% <0.00%> (-0.65%) ⬇️
...ain/java/org/opensearch/ad/task/ADTaskManager.java 76.52% <0.00%> (+0.15%) ⬆️
...in/java/org/opensearch/ad/model/AnomalyResult.java 82.94% <0.00%> (+0.33%) ⬆️
... and 19 more

int totalHits = 9;
when(detectorResponse.getHits()).thenReturn(TestHelpers.createSearchHits(totalHits));

// extend NodeClient since its execute method is final and mockito does not allow to mock final methods
Copy link
Collaborator

@ylwu-amzn ylwu-amzn Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to use doAnswer instead of extending nodeClient

@@ -73,8 +75,10 @@ public static String getTooManyCategoricalFieldErr(int limit) {
public static String HISTORICAL_ANALYSIS_CANCELLED = "Historical analysis cancelled by user";
public static String HC_DETECTOR_TASK_IS_UPDATING = "HC detector task is updating";
public static String NEGATIVE_TIME_CONFIGURATION = "should be non-negative";
public static String INVALID_TIME_CONFIGURATION_UNITS = "Timezone %s is not supported";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message seems not correct from the first release. Change to "Time unit %s is not supported" ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! Changed this.

@ylwu-amzn
Copy link
Collaborator

Link Checker failed, can you check and fix?

@amitgalitz
Copy link
Member Author

Link Checker failed, can you check and fix?

Seems like this check has failed for the past week. After doing some research seems like a lot of other repos using the link-checker action have dropped the quotations mark surrounding args. A new PR from lychee-action team was released to add support for extra inputs. The new code adds quotes to the arguments so need to remove quotes we pass into args from now.

ylwu-amzn
ylwu-amzn previously approved these changes Dec 14, 2021
ohltyler
ohltyler previously approved these changes Jan 7, 2022
Copy link
Member

@ohltyler ohltyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
@amitgalitz amitgalitz merged commit 6975228 into opensearch-project:main Jan 7, 2022
ylwu-amzn pushed a commit to ylwu-amzn/anomaly-detection-2 that referenced this pull request Jan 11, 2022
ylwu-amzn pushed a commit that referenced this pull request Jan 12, 2022
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants