Test refactor in preparation for change authz header initialization #1127
Conversation
| } | ||
|
|
||
| func (f *fakeHTTPClient) Do(h *http.Request) (*http.Response, error) { | ||
| // Record the request for later test assertions. | ||
| f.requests = append(f.requests, h) | ||
| if f.userAgent != "" && h.Header.Get("User-Agent") != f.userAgent { | ||
| return nil, fmt.Errorf("Wrong user agent: %s", h.Header.Get("User-Agent")) |
There was a problem hiding this comment.
Since we're recording the requests now - and can assert on the actual requests in the context of the test, we could simplify this fake Do implementation, removing the test assertions like this one? That is, rather than the test not actually doing any assertion (other than that an error was not returned), we can assert the user-agent was set as expected in the context of the test, rather than in this fake code (which has no access to the t.Testing to call t.Errorf etc). See what you think - I won't do it in this PR, but just a thought.
There was a problem hiding this comment.
that sounds good to me, the only reason for doing the assertion here is that we couldn't do it in the test. If we can I am happy to do it.
| @@ -420,15 +424,13 @@ func (f *fakeHTTPClient) Do(h *http.Request) (*http.Response, error) { | |||
| // Return fake chart index (not customizable per repo) | |||
| body, err := json.Marshal(*f.index) | |||
| if err != nil { | |||
| fmt.Printf("Error! %v", err) | |||
| return nil, err | |||
There was a problem hiding this comment.
Doesn't seem right to be printing to stdout and continuing on with a 200 response in this case, so verified that tests still pass if I return an error here.
| } | ||
| return &http.Response{StatusCode: 200, Body: ioutil.NopCloser(bytes.NewReader(body))}, nil | ||
| } | ||
| } | ||
| for _, chartURL := range f.chartURLs { | ||
| if h.URL.String() == chartURL { | ||
| // Simulate download time | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
Couldn't see any reason why we'd need to slow down our unit tests like this? I don't think this fake is being used for anything other than the unit-tests?
There was a problem hiding this comment.
TBH I don't remember the reason for this.
| chartURLs: chartURLs, | ||
| index: index, | ||
| userAgent: userAgent, | ||
| }, |
There was a problem hiding this comment.
Just switched to labeling the composite literal fields to avoid adding nil on the end for the requests (which aren't required for initialization)
There was a problem hiding this comment.
I was unaware of that change of behavior, can you add a comment? If you initialize it like this request is an empty slice instead?
There was a problem hiding this comment.
Yep, from https://golang.org/doc/effective_go.html#composite_literals "However, by labeling the elements explicitly as field:value pairs, the initializers can appear in any order, with the missing ones left as their respective zero values."
There's no change of behaviour here - I added the requests slice in this PR (and it should get a nil value on initialization). I just don't like specifying nil in an initialization without a label as you then need to scroll to find out what it was being initialized to nil to see if it's relevant, whereas with a labelled initialization, I can assume all the relevant info is there.
| } | ||
|
|
||
| func (f *fakeHTTPClient) Do(h *http.Request) (*http.Response, error) { | ||
| // Record the request for later test assertions. | ||
| f.requests = append(f.requests, h) | ||
| if f.userAgent != "" && h.Header.Get("User-Agent") != f.userAgent { | ||
| return nil, fmt.Errorf("Wrong user agent: %s", h.Header.Get("User-Agent")) |
There was a problem hiding this comment.
that sounds good to me, the only reason for doing the assertion here is that we couldn't do it in the test. If we can I am happy to do it.
| } | ||
| return &http.Response{StatusCode: 200, Body: ioutil.NopCloser(bytes.NewReader(body))}, nil | ||
| } | ||
| } | ||
| for _, chartURL := range f.chartURLs { | ||
| if h.URL.String() == chartURL { | ||
| // Simulate download time | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
TBH I don't remember the reason for this.
| chartURLs: chartURLs, | ||
| index: index, | ||
| userAgent: userAgent, | ||
| }, |
There was a problem hiding this comment.
I was unaware of that change of behavior, can you add a comment? If you initialize it like this request is an empty slice instead?
Since the client will be able to pass an
AppRepositoryname instead of the actualAuthobject, I'll need to initialise theauthHeaderduringInitNetClientwhen the custom resource will be fetched (as we already fetch the CustomCA there too).In preparation for that, I want to move the existing code which fetches the auth header from the
GetChartcall toInitNetClient, so both will be consistent - that'll be the next PR. This PR just does a little preparation, refactoring theGetCharttests so that I can add the auth check cleanly in the next PR.A few notes inline.
Ref #1110