Move versionchecker tests to test/integration#4728
Move versionchecker tests to test/integration#4728jetstack-bot merged 1 commit intocert-manager:masterfrom
Conversation
Since this test requires setup before it can successfully run, we define it as an integration test and move it here so that on a fresh checkout a user can always run `go test ./pkg/...` and expect that it would succeed. Also involves: - Exporting the VersionChecker and adding NewWithConfig to enable testing - Some comment changes - A change to the type returned by New(); see https://github.com/golang/go/wiki/CodeReviewComments#interfaces Ideally I'd not add `NewFromClient` but I think it's the most minimal change and is preferable to publicly exporting `VersionChecker.client`. Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
irbekrm
left a comment
There was a problem hiding this comment.
This makes sense to me and looks straightforward, I've ran go test ./pkg/... from this branch and now the only thing that fails is the dns integration test 👍🏼
- A change to the type returned by New(); see
https://github.com/golang/go/wiki/CodeReviewComments#interfaces
Thanks for linking this, I'd been wanting to find some 'official' place where this principle is written down
Ideally I'd not add
NewFromClientbut I think it's the most minimal
change and is preferable to publicly exportingVersionChecker.client.
Agree that there are probably more elegant ways to do this that aren't worth implementing for this one use case 👍🏼
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irbekrm, SgtCoDFish 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 |
Since some of these version checker tests require setup before they can successfully run, we define them as integration tests and move them here so that on a fresh checkout a user can always run
go test ./pkg/...and expect that it would succeed.There are other tests which need to be moved, but I've separated this one out.
Also involves:
VersionCheckerand addingNewFromClientto enable testing.Notes for Reviewers
NewFromClientbut I think it's the most minimal change and is preferable to publicly exportingVersionChecker.client.versionchecker.Interfaceneeds to exist but I don't want to go off track and remove it/kind cleanup