daemon: Add ReadFile to FsClient #139
Conversation
Signed-off-by: Steve Milner <smilner@redhat.com>
|
Yay! @ashcrow Q: line 424 also uses ioutils.ReadAll. Do we need to add that too or will we not need to mock it bc we will/ intend to have dummy resp.Body input that we can test with? |
|
@kikisdeliveryservice both methods will be updated once this merges :-). The only one I think we can't change at the present is |
|
@ashcrow sounds good! |
|
/retest |
|
|
||
| // ReadFile provides a mocked implemention | ||
| func (f FsClientMock) ReadFile(filename string) ([]byte, error) { | ||
| returnValues := f.ReadFileReturns[0] |
There was a problem hiding this comment.
Shouldn't this be under the if statement to make sure the array is not empty first? E.g.
if len(f....) > 0 {
returnValues := f....[0]
f.... = f.ReadFileReturns[1:]
return returnValues
}
panic("no more values!")?
I guess, as it is it'll panic anyway with index out of range, which I suppose was the intent since the unit test knows exactly how many times it should be called? But then it seems unnecessary to do the len check since if we got to there it means there definitely is at least one element. So an alternative is to just simplify it to:
returnValues := f....[0]
f.... = f.ReadFileReturns[1:]
return returnValues? (Though I still prefer the explicitness of the first alternative!)
There was a problem hiding this comment.
The idea behind this is that:
- If you provide nothing it panics
- If you provide N returns and use N+X then X+1 will return the same last element (for example if I want everything to return no error I can just give it one nil, not 2, 5, 50, etc..)
There was a problem hiding this comment.
I'm happy to do a follow on PR to make the panic explicit :-).
Signed-off-by: Steve Milner <smilner@redhat.com>
openshift-ci-robot
added
size/M
|
OK, with that last commit, that looks sane to me. 👍 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashcrow, jlebon, kikisdeliveryservice 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 |
…tion Add node-selector annotation to namespace
6 participants
No description provided.