[GSOC] added Unknown distribution and convertDistribution in suggestion client #2403
Conversation
google-oss-prow
bot
requested review from
anencore94,
gaocegege and
johnugeorge
added unit tests Signed-off-by: Shashank Mittal <[email protected]>
shashank-iitbhu
force-pushed
the
feat/suggestionclient-update
branch
from
a3e6c72 to
3a3540e
Compare
|
|
||
| for _, tc := range tcs { | ||
| actualFeasibleSpace := convertFeasibleSpace(tc.inFeasibleSpace) | ||
| if !compareFeasibleSpaces(actualFeasibleSpace, tc.expectedFeasibleSpace) { |
There was a problem hiding this comment.
Instead of this custom function, could you use the https://github.com/google/go-cmp?
You can find some guidance for developing here: https://github.com/kubeflow/katib/blob/master/docs/developer-guide.md#go-development
There was a problem hiding this comment.
Thanks, removed the helper function and used cmp.Diff instead.
| expectedFeasibleSpace *suggestionapi.FeasibleSpace | ||
| testDescription string | ||
| }{ | ||
| { |
There was a problem hiding this comment.
I don't think that all 3 cases should be useful.
So, could you add only 2 cases, "distribution is Distribution_DISTRIBUTION_UNKNOWN", "distribution is not Distribution_DISTRIBUTION_UNKNOWN"?
There was a problem hiding this comment.
Yes, removed the extra test cases.
Signed-off-by: Shashank Mittal <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tenzen-y 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:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #2374
Checklist: