add unit tests #70
add unit tests #70
Conversation
k8s-ci-robot
added
cncf-cla: yes
| "testing" | ||
| ) | ||
|
|
||
| func TestGpuConfigValidate(t *testing.T) { |
There was a problem hiding this comment.
One test case that is not covered is that if both TimeSlicing and SpacePartitioning are defined, an invalid SpacePartitioning config is ignored.
This might not be intended behaviour, but it may be behaviour that users start to depend on. (although since this is an example driver it's probably not critical) cc @pohly @klueska
There was a problem hiding this comment.
Agree, that seems worth recording in a test, even if just to signal to authors of real drivers that that's something to look out for and potentially not recreate.
k8s-ci-robot
added
the
needs-rebase
|
Pushed a merge commit to fix the conflict in case that's easier to review than a rebase, but I'll plan to squash before merging this. /hold |
k8s-ci-robot
added
do-not-merge/hold
|
I took the liberty of squashing after addressing the latest round of feedback. /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
| Sharing: &GpuSharing{}, | ||
| }, | ||
| expected: "unknown GPU sharing strategy: ", | ||
| }, |
There was a problem hiding this comment.
I'd also add this test case:
"unknown GPU sharing strategy": {
gpuConfig: &GpuConfig{
Sharing: &GpuSharing{
Strategy: "unknown",
},
},
expected: errors.New("unknown GPU sharing strategy: unknown"),
},
BTW, this test case shows that this code is unreachable.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elezar, nojnhuh 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 |
k8s-ci-robot
added
the
approved
This PR adds unit tests for at least the low-hanging fruit and enables the tests in CI. I think what I didn't include here already has decent coverage from the e2e tests, but please let me know if I missed something that would be valuable to add unit tests for.
Fixes #41