-
Notifications
You must be signed in to change notification settings - Fork 143
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
Updated Testcases for RemoteModel.java #1535
Conversation
Signed-off-by: Divit Rawal <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1535 +/- ##
============================================
- Coverage 78.40% 78.33% -0.07%
+ Complexity 2378 2374 -4
============================================
Files 195 195
Lines 9588 9588
Branches 964 964
============================================
- Hits 7517 7511 -6
- Misses 1633 1639 +6
Partials 438 438
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@dhrubo-os do you know why coverage went down with an addition of a test case? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you are testing the right thing here.
Connector connector = createConnector(ImmutableMap.of("Authorization", "Bearer ${credential.key}")); | ||
when(mlModel.getConnector()).thenReturn(connector); | ||
remoteModel.initModel(mlModel, ImmutableMap.of(), encryptor); | ||
Assert.assertTrue(remoteModel.isModelReady()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to be flaky due to timing?
@@ -65,6 +73,15 @@ public void predict_NullConnectorExecutor() { | |||
remoteModel.predict(mlInput); | |||
} | |||
|
|||
@Test | |||
public void predict_ModelPredictSuccess() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"testXYZ" and no underscore.
when(mlModel.getConnector()).thenReturn(connector); | ||
remoteModel.initModel(mlModel, ImmutableMap.of(), encryptor); | ||
Assert.assertTrue(remoteModel.isModelReady()); | ||
when(remoteConnectorExecutor.executePredict(mlInput)).thenReturn(new ModelTensorOutput(new ArrayList<>())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you testing??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to close this PR if there's still no response to Austin's comments. @divitr
Description
Updated Testcases for RemoteModel.java
Issues Resolved
Resolves #1382
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.