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

[FEATURE]Improve test coverage for RemoteModel.java #3205

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

akolarkunnu
Copy link
Contributor

Description

Added new tests for missing coverage. Mainly coverage was missing for catching exceptions in the methods initModel() and asyncPredict(). Also renamed some tests to match with testing methods.

Related Issues

Resolves #1382

Check List

  • [ X] New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • [X ] Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

Added new tests for missing coverage. Mainly coverage was missing for catching exceptions in the methods initModel() and asyncPredict().
Also renamed some tests to match with testing methods.

Resolves opensearch-project#1382

Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>
@pyek-bot
Copy link
Contributor

pyek-bot commented Nov 8, 2024

Thanks for the contribution! Appreciate it. Can you share a coverage report similar to the one in issue to see if it actual covers those exceptions?

Added new tests for missing coverage. Mainly coverage was missing for catching exceptions in the methods initModel() and asyncPredict().
Also renamed some tests to match with testing methods.

Resolves opensearch-project#1382

Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>
@akolarkunnu akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval November 8, 2024 06:04 — with GitHub Actions Failure
@akolarkunnu akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval November 8, 2024 06:04 — with GitHub Actions Failure
@akolarkunnu akolarkunnu closed this Nov 8, 2024
@akolarkunnu
Copy link
Contributor Author

Thanks for the contribution! Appreciate it. Can you share a coverage report similar to the one in issue to see if it actual covers those exceptions?

I was expecting that code coverage report generation is part of CI, similar to other projects, eg: OpenSearch core, Security, etc. I verified the coverage by putting explicit traces in each exception blocks. Can you please share, how can I run the coverage tool and share the report links here?

@akolarkunnu akolarkunnu reopened this Nov 8, 2024
@akolarkunnu akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval November 8, 2024 08:58 — with GitHub Actions Failure
@akolarkunnu akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval November 8, 2024 08:58 — with GitHub Actions Failure
@pyek-bot
Copy link
Contributor

pyek-bot commented Nov 8, 2024

Thanks for the contribution! Appreciate it. Can you share a coverage report similar to the one in issue to see if it actual covers those exceptions?

I was expecting that code coverage report generation is part of CI, similar to other projects, eg: OpenSearch core, Security, etc. I verified the coverage by putting explicit traces in each exception blocks. Can you please share, how can I run the coverage tool and share the report links here?

  • Run the jacocoTestReport gradle task for the particular package (ml-algorithms)
  • Run python3 -m http.server 3000
  • Open localhost:3000 on your browser
  • Navigate to the ml-algorithms/build/reports/jacoco/test/html
  • Open your file and check coverage

It will be locally available for you, you can just verify if those lines are hit and maybe share a screenshot. Changes look good to me!

Let me know if this helps!

Added new tests for missing coverage. Mainly coverage was missing for catching exceptions in the methods initModel() and asyncPredict().
Also renamed some tests to match with testing methods.

Resolves opensearch-project#1382

Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>
@akolarkunnu akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval November 11, 2024 11:02 — with GitHub Actions Failure
@akolarkunnu akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval November 11, 2024 11:02 — with GitHub Actions Failure
@akolarkunnu
Copy link
Contributor Author

Thanks for the contribution! Appreciate it. Can you share a coverage report similar to the one in issue to see if it actual covers those exceptions?

I was expecting that code coverage report generation is part of CI, similar to other projects, eg: OpenSearch core, Security, etc. I verified the coverage by putting explicit traces in each exception blocks. Can you please share, how can I run the coverage tool and share the report links here?

  • Run the jacocoTestReport gradle task for the particular package (ml-algorithms)
  • Run python3 -m http.server 3000
  • Open localhost:3000 on your browser
  • Navigate to the ml-algorithms/build/reports/jacoco/test/html
  • Open your file and check coverage

It will be locally available for you, you can just verify if those lines are hit and maybe share a screenshot. Changes look good to me!

Let me know if this helps!

Yes this helped. All exceptions were covered, but below line was not covered, so added one more test case to cover that:

actionType = ((RemoteInferenceInputDataSet) mlInput.getInputDataset()).getActionType();

Now it is 100% branch and line, please see the screenshots.

Package_Level
Code_1
Code_2

@pyek-bot
Copy link
Contributor

Looks good to me! Thank you!

@ylwu-amzn
Copy link
Collaborator

Thanks @akolarkunnu. can you run ./gradlew spotlessApply to format code ? I see such error in CI

* What went wrong:
Execution failed for task ':opensearch-ml-algorithms:spotlessJavaCheck'.

> The following files had format violations:
      src\test\java\org\opensearch\ml\engine\algorithms\remote\RemoteModelTest.java
Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.
          @@ -14,8 +14,8 @@
           import�static�org.mockito.Mockito.when;
           
           import�java.util.Arrays;

          +import�java.util.Collections;
           import�java.util.Map;
You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
          -import�java.util.Collections;
           
           import�org.junit.Assert;
           import�org.junit.Before;
  Run 'gradlew.bat :opensearch-ml-algorithms:spotlessApply' to fix these violations.

Added new tests for missing coverage. Mainly coverage was missing for catching exceptions in the methods initModel() and asyncPredict().
Also renamed some tests to match with testing methods.

Resolves opensearch-project#1382

Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>
@akolarkunnu
Copy link
Contributor Author

Thanks @akolarkunnu. can you run ./gradlew spotlessApply to format code ? I see such error in CI

* What went wrong:
Execution failed for task ':opensearch-ml-algorithms:spotlessJavaCheck'.

> The following files had format violations:
      src\test\java\org\opensearch\ml\engine\algorithms\remote\RemoteModelTest.java
Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.
          @@ -14,8 +14,8 @@
           import�static�org.mockito.Mockito.when;
           
           import�java.util.Arrays;

          +import�java.util.Collections;
           import�java.util.Map;
You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
          -import�java.util.Collections;
           
           import�org.junit.Assert;
           import�org.junit.Before;
  Run 'gradlew.bat :opensearch-ml-algorithms:spotlessApply' to fix these violations.

yes, I forgot to run it this time. Ran it and updated.

@akolarkunnu akolarkunnu had a problem deploying to ml-commons-cicd-env-require-approval November 12, 2024 02:55 — with GitHub Actions Error
@akolarkunnu akolarkunnu temporarily deployed to ml-commons-cicd-env-require-approval November 12, 2024 02:55 — with GitHub Actions Inactive
@akolarkunnu akolarkunnu temporarily deployed to ml-commons-cicd-env-require-approval November 12, 2024 05:09 — with GitHub Actions Inactive
@akolarkunnu
Copy link
Contributor Author

Can any reviewers please review this?

@dhrubo-os dhrubo-os merged commit 45ff4f5 into opensearch-project:main Nov 14, 2024
7 of 8 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 14, 2024
* [FEATURE]Improve test coverage for RemoteModel.java

Added new tests for missing coverage. Mainly coverage was missing for catching exceptions in the methods initModel() and asyncPredict().
Also renamed some tests to match with testing methods.

Resolves #1382

Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>

* [FEATURE]Improve test coverage for RemoteModel.java

Added new tests for missing coverage. Mainly coverage was missing for catching exceptions in the methods initModel() and asyncPredict().
Also renamed some tests to match with testing methods.

Resolves #1382

Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>

* [FEATURE]Improve test coverage for RemoteModel.java

Added new tests for missing coverage. Mainly coverage was missing for catching exceptions in the methods initModel() and asyncPredict().
Also renamed some tests to match with testing methods.

Resolves #1382

Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>

* [FEATURE]Improve test coverage for RemoteModel.java

Added new tests for missing coverage. Mainly coverage was missing for catching exceptions in the methods initModel() and asyncPredict().
Also renamed some tests to match with testing methods.

Resolves #1382

Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>

---------

Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>
(cherry picked from commit 45ff4f5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]Improve test coverage for RemoteModel.java
5 participants