-
Notifications
You must be signed in to change notification settings - Fork 129
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
Add integ tests for model APIs #166
Conversation
@@ -283,7 +284,16 @@ protected void validateStats( | |||
} | |||
assertEquals(expectedTotalFailureCount, totalFailureCount); | |||
assertEquals(expectedTotalAlgoFailureCount, totalAlgoFailureCount); | |||
assertEquals(expectedTotalRequestCount, totalRequestCount); | |||
// ToDo: this line makes this test flaky as other tests makes the request count not predictable | |||
// assertEquals(expectedTotalRequestCount, totalRequestCount); |
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 think it's ok to remove this line, multiple tests against same cluster may have different total requests based on their execution sequence.
public class RestMLDeleteModelActionIT extends MLCommonsRestTestCase { | ||
|
||
public void testDeleteModelAPI_EmptyResources() throws Exception { | ||
TestHelper |
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.
To keep consistent with other code, how about using ExpectedException
?
@Rule
public ExpectedException exceptionRule = ExpectedException.none();
public void testGetModelAPI_EmptyResources() throws IOException {
exceptionRule.expect(ResponseException.class);
exceptionRule.expectMessage("index_not_found_exception");
TestHelper.makeRequest(client(), "DELETE", "/_plugins/_ml/models/111222333", null, "", null);
}
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.
Yep, updated to use ExpectedException.
Signed-off-by: Xun Zhang <xunzh@amazon.com>
|
||
public class RestMLGetModelActionTests extends OpenSearchTestCase { | ||
@Rule | ||
public ExpectedException thrown = ExpectedException.none(); |
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.
Minor: This is not being used.
You can keep the commit history, especially after first round of review. So review can review what changed only. That can save some time.
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.
LGTM.
Signed-off-by: Xun Zhang xunzh@amazon.com
Description
Add integration tests for all model APIs. Rebased on PR 167.
Issues Resolved
[List any issues this PR will resolve]
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.