Conversation
| "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.
|
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 |
elezar
left a comment
There was a problem hiding this comment.
Minor comment on the go modules, but otherwise this looks good.
|
I took the liberty of squashing after addressing the latest round of feedback. /hold cancel |
| 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 |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This reverts commit ddf4fdf.
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