[SDK] Unit tests for TrainingClient APIs - get_job_pod_names and update_job #2192
Conversation
… and update_job Signed-off-by: yelias <[email protected]>
Pull Request Test Coverage Report for Build 10511367867Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Signed-off-by: yelias <[email protected]>
|
can someone pls review this PR? |
There was a problem hiding this comment.
Sorry for the late review @YosiElias!
Please take a look
/assign @droctothorpe @kubeflow/wg-training-leads @Electronic-Waste
| LIST_RESPONSE = [ | ||
| {"metadata": {"name": "Dummy V1PodList-1"}}, | ||
| {"metadata": {"name": "Dummy V1PodList-2"}}, | ||
| ] |
There was a problem hiding this comment.
Why do you want to change it ?
There was a problem hiding this comment.
Just to test and present the possibility of returning more than one name (I can remove this case if you think its unnecessary)
There was a problem hiding this comment.
@andreyvelich is it fine by you to keep this case?
There was a problem hiding this comment.
I would suggest keep it as it is for old tests, since in that case we don't need to introduce SimpleNamespace from typing module.
| "valid flow with 2 pods", | ||
| { | ||
| "name": TEST_NAME, | ||
| "namespace": "2_pods", |
There was a problem hiding this comment.
Do you need to use different namespaces here ?
There was a problem hiding this comment.
same as above, this is kind of flag for this case with 2 names
|
Hi @YosiElias! Did you get a chance to check my comment here: #2192 (comment) ? |
…od obj Signed-off-by: yelias <[email protected]>
|
Hi @andreyvelich! Sorry for the delay, I was busy with other things. |
Signed-off-by: yelias <[email protected]>
|
Overall, looks good, thank you for doing this @YosiElias! |
Signed-off-by: yelias <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich 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 |
What this PR does / why we need it:
New unit test for get_job_pod_names and update_job functions
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #2161
Checklist: