[SDK] test: add unit test for list_jobs method of the training_client #2267
Conversation
Pull Request Test Coverage Report for Build 11155962731Details
💛 - Coveralls |
|
@seanlaii Please rebase your PR. |
seanlaii
force-pushed
the
unit_test-TrainingClient-list_jobs
branch
from
7b04d0a
to
522e8d9
Compare
Rebased. Please help take a look. Thank you. |
There was a problem hiding this comment.
Thank you for this @seanlaii!
I left a few comments.
/cc @kubeflow/wg-training-leads @Electronic-Waste
| TimeoutError, | ||
| ), | ||
| ( | ||
| "invalid flow with runtime error", |
There was a problem hiding this comment.
Do you check this test case here: runtime error case ?
There was a problem hiding this comment.
You are right. Let me remove it.
| if args[2] == "timeout": | ||
| raise multiprocessing.TimeoutError() | ||
| elif args[2] == "runtime": |
There was a problem hiding this comment.
Please can you use constants here:
Additionally, we can change them here:
seanlaii
force-pushed
the
unit_test-TrainingClient-list_jobs
branch
4 times, most recently
from
4d8f77e
to
c3f087c
Compare
| print("Executing test: ", test_name) | ||
|
|
||
| try: | ||
| training_client.list_jobs(**kwargs) |
There was a problem hiding this comment.
Additionally, should we also verify the output of the list function, similar to how we verify output for get_job_pods API: https://github.com/kubeflow/training-operator/blob/c3f087c22e95fbe9082a5fd9b90117f1f115f800/sdk/python/kubeflow/training/api/training_client_test.py#L298C9-L298C22 ?
There was a problem hiding this comment.
Yes, thanks for the suggestion! I should test the output as well. Please help review when you are available.
Signed-off-by: wei-chenglai <[email protected]>
seanlaii
force-pushed
the
unit_test-TrainingClient-list_jobs
branch
from
c3f087c
to
9f07ba8
Compare
|
Thanks for the update @seanlaii! |
|
[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 list_jobs function
To verify run:
Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Ref: #2161
Checklist: