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

Reduced jacoco exclusions and added more tests #446

Merged
merged 3 commits into from
Mar 22, 2022

Conversation

amitgalitz
Copy link
Member

@amitgalitz amitgalitz commented Mar 16, 2022

Signed-off-by: Amit Galitzky [email protected]

Description

This PR adds some tests to various classes that had low coverage as well as removes any unnecessary files from the jacocoExclusion list on build.gradle. We had some files there that were either already above the coverage minimum or didn’t exist anymore. Specifically .../model.* was listed and with additional testing added here and in previous PR (#335) 34 classes were taken off the exclusion list and now only 2 classes from model package are still excluded.

I wont close the related issue (#424) yet since more work needs to be done addressing the TODOs, potentially reducing coverage minimums and finishing rest of model package testing.

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 March 16, 2022 00:05
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2022

Codecov Report

Merging #446 (455a07c) into main (b0f6475) will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #446      +/-   ##
============================================
+ Coverage     78.11%   78.32%   +0.20%     
- Complexity     4141     4170      +29     
============================================
  Files           296      296              
  Lines         17657    17657              
  Branches       1879     1879              
============================================
+ Hits          13792    13829      +37     
+ Misses         2971     2948      -23     
+ Partials        894      880      -14     
Flag Coverage Δ
plugin 78.32% <100.00%> (+0.20%) ⬆️

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

Impacted Files Coverage Δ
.../ad/rest/handler/ModelValidationActionHandler.java 0.00% <ø> (ø)
...ava/org/opensearch/ad/model/EntityProfileName.java 100.00% <100.00%> (+25.00%) ⬆️
...java/org/opensearch/ad/task/ADBatchTaskRunner.java 81.76% <0.00%> (-3.65%) ⬇️
...ava/org/opensearch/ad/task/ADHCBatchTaskCache.java 88.88% <0.00%> (-2.47%) ⬇️
...rch/ad/transport/AnomalyResultTransportAction.java 80.13% <0.00%> (-0.69%) ⬇️
...ain/java/org/opensearch/ad/task/ADTaskManager.java 76.52% <0.00%> (-0.61%) ⬇️
...opensearch/ad/indices/AnomalyDetectionIndices.java 72.28% <0.00%> (-0.57%) ⬇️
.../main/java/org/opensearch/ad/ml/CheckpointDao.java 70.19% <0.00%> (+0.64%) ⬆️
.../java/org/opensearch/ad/AnomalyDetectorPlugin.java 96.53% <0.00%> (+1.73%) ⬆️
...ain/java/org/opensearch/ad/model/ModelProfile.java 72.72% <0.00%> (+1.81%) ⬆️
... and 12 more

@amitgalitz amitgalitz added the infra Changes to infrastructure, testing, CI/CD, pipelines, etc. label Mar 16, 2022
ylwu-amzn
ylwu-amzn previously approved these changes Mar 16, 2022
Copy link
Collaborator

@ylwu-amzn ylwu-amzn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding more tests!

@@ -479,15 +479,16 @@ List<String> jacocoExclusions = [
// code for configuration, settings, etc is excluded from coverage
'org.opensearch.ad.AnomalyDetectorPlugin',

// rest layer is tested in integration testing mostly, difficult to mock all of it
Copy link
Collaborator

Choose a reason for hiding this comment

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

Refer to this PR to write unit test for REST actions opensearch-project/ml-commons#228?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will take a look! Will try to add those in a separate PR if thats fine

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, that's fine.

@amitgalitz amitgalitz requested review from ohltyler and kaituo March 22, 2022 19:58
@amitgalitz amitgalitz merged commit 2505add into opensearch-project:main Mar 22, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 22, 2022
Signed-off-by: Amit Galitzky <[email protected]>
(cherry picked from commit 2505add)
amitgalitz added a commit that referenced this pull request Mar 22, 2022
Signed-off-by: Amit Galitzky <[email protected]>
(cherry picked from commit 2505add)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.x infra Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants