-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[AWS SageMaker] Add more unit tests #3783
Conversation
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
@@ -56,7 +57,7 @@ def create_parser(): | |||
|
|||
def main(argv=None): | |||
parser = create_parser() | |||
args = parser.parse_args() | |||
args = parser.parse_args(argv) |
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.
why do we need this change with line 77 ?
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.
in main, parse_args() automatically takes argv input if we don't specify anything.
But when we mock the function it doesn’t.
So we have to explicitly specify the parameter for unit tests to run
/ok-to-test |
/retest |
dfb76f2
to
f3a9416
Compare
Could you please run these with the automated unit and integration test CodeBuild jobs to ensure they all still pass successfully? Otherwise I think I'm happy for this to be merged. |
will do 👍🏻 |
a1d2cf0
to
59dcd38
Compare
59dcd38
to
f367652
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RedbackThomson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
1 similar comment
/retest |
/test tide |
@akartsky: The specified target(s) for
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest |
* add more tests for deploy and ground_truth components * add more tests for workteam component * add unit tests for model component * add more unit tests for batchTransform component * add more tests * add 'request' function tests * add more unit tests for ground truth
Added more tests for following components :
Deploy (98% coverage)
Ground_truth (98% coverage)
Workteam (96% coverage)
Model (97% coverage)
Batch Transform (98% coverage)
#3782
These tests check : (for 1 inout)
These things can be done more extensively to verify every combination of input