rollouts: add unit test and fake client for simple reconcileRollouts case #3894
Conversation
natasha41575
changed the title
| targetClusters, | ||
| []packagediscovery.DiscoveredPackage{discoveredPackage}, | ||
| ) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Are we using this style of assertions in other parts of kpt project ? otherwise I have found omega matchers to be very good for this.
There was a problem hiding this comment.
There are a number of places in kpt repo that we are using the testify/assert and testify/require packages to do similar assertions in unit tests. require is a subtly different flavor of assert, except that require exits the test immediately, while assert continues running the rest of the test even when it encounters an error.
That said, I am happy to convert to omega matchers if you prefer!
There was a problem hiding this comment.
Didn't see usage of require before. But the failure behavior of require does make more sense.
I am fine using require here, so let's ship it.
| targetClusters, | ||
| []packagediscovery.DiscoveredPackage{discoveredPackage}, | ||
| ) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Didn't see usage of require before. But the failure behavior of require does make more sense.
I am fine using require here, so let's ship it.
This PR adds a unit test for a simple case for
reconcileRollouts. To support the unit test, this PR adds a fake client and refactors a few things out ofreconcileRollout. I'd like to add the remaining unit tests in a followup, so that the review here can focus on structure of the test, fake client code, and refactoring, and the followup PR can focus on code coverage.I looked at using https://github.com/golang/mock for the fake client, but felt that this fake client is simple enough that another framework wasn't needed. Happy to revisit this idea though.
Related issue: #3856