Ensure we are testing scheme registration for termination handler #337
Conversation
|
/retest |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre 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 |
openshift-ci-robot
added
the
approved
|
/retest |
| httpHandler = nil | ||
| nodeName = "test-node" | ||
| httpHandler = newMockHTTPHandler(notFoundFunc) | ||
| stop = nil |
There was a problem hiding this comment.
I agree this should be the fix for unit flakes, re: #340 (comment)
Danil-Grigorev
left a comment
Danil-Grigorev
left a comment
There was a problem hiding this comment.
LGTM, just some comments on possible places for race conditions
pkg/termination/handler_test.go
Outdated
| Context("and the instance termination endpoint returns an unknown status", func() { | ||
| BeforeEach(func() { | ||
| httpHandler = newMockHTTPHandler(func(rw http.ResponseWriter, req *http.Request) { | ||
| if counter == 4 { |
There was a problem hiding this comment.
Shouldn't it use atomic here, same as on https://github.com/openshift/cluster-api-provider-aws/pull/337/files#diff-7b9c0d2d3679d7f69b6f96175bb1965aR117
There was a problem hiding this comment.
Good catch! This was actually in the existing code, I've just changed the indentation, but will fix
pkg/termination/handler_test.go
Outdated
| if counter == 4 { | ||
| rw.WriteHeader(500) | ||
| } else { | ||
| counter++ |
There was a problem hiding this comment.
Could we use atomic incrementer here as well?
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
6 participants
This PR ensures that we use an independent scheme for the test setup so that the scheme registration in the
NewHandlermethod is being tested.This also refactors the tests slightly to ensure that we don't start the handler for tests where we aren't checking it's running, because this was cause races.
Note for reviewers: This PR is best reviewed with whitespace changes turned off