Testenv test refactor

[darkowlzz]

Introduces testenv based tests.
Refactors all the existing tests to use testenv.

⚠️ target branch of the PR is reconcilers-dev.

[darkowlzz]

Regarding this, it looks like this test was changed in 4fde199#diff-1168fadffa18bd096582ae7f8b6db744fd896bd5600ee1d1ac6ac4474af251b9L292-L334 which removed the actual steps of this test case.
I couldn't figure out why this was removed from the commit and the associated PR.
Thought of adding it separately and leaving a TODO for now.

[squaremo]

Yep that's pretty random. The tests got rearranged in that commit so they could be run with both git implementations; all I can think of it is that I killed those lines, expecting to yank them elsewhere, and neglected to do the second part.

[squaremo]

TODO for me to check:

controllers/update_test.go

These use testWithRepoAndImagePolicy:

• ImageUpdateAutomation
• initialises Git OK (tested implicitly in the factored out testWithRepoAndImagePolicy)
• commit spec
  • formats the commit message as in the template (TestImageUpdateAutomation_commit_message)
  • has the commit author as given (TestImageUpdateAutomation_commit_message)
• update path
  • updates only the deployment in the specified path (TestImageUpdateAutomation_update_path)
• commit signing ()
  • signs the commit with the generated GPG key (TestImageUpdateAutomation_signed_commit)

The following are run by looping over (git repo URL) protos and (git client library) impls. The original has a test "template" that is called with those two arguments, which has a few conditional blocks within to account for e.g., which git server bits to start and shutdown. The rewritten test has similar conditionals but differs in how it deals with shutting the git server down.

• Using go-git with {HTTP, SSH} x {libgit2, go-git}

  • with PushSpec
    • creates and pushes the push branch (in t.Run("update with PushSpec", ...))
    • pushes another commit to the push branch (in t.Run("push branch gets updated" ...))
    • still pushes to the push branch after it's merged (in t.Run("still pushes to push branch after it's merged", ...)`; NB this has a small difference because it's run in sequence with the others, so it has to make another change to the latest image -- the dependence on ordering is something to watch for)
  • with Setters
    • updates to the most recent image (in t.Run("with update strategy setters", ...))
    • stops updating when suspended (in t.Run("no reconciliation when object is suspended", ...))

• defaulting

  • defaults .spec.update to {strategy: Setters} (TestImageUpdateAutomation_defaulting)

controllers/git_test.go

(already in normal form)

controllers/git_error_test.go

(already in normal form)

pkg/test/files_test.go

• when no differences (withhttps://github.com/Correct directory diffing test and algorithm darkowlzz/image-automation-controller#1)
  • matches when given the same directory
  • matches when given equivalent directories
• with differences
  • finds files in expected from a/ but not in actual from b/
  • find files in actual a/ that weren't expected from b/
  • finds files that are different in a/ and b/

pkg/update/filereader_test.go

• load YAMLs with ScreeningLocalReader
  • loads only the YAMLS containing the token

pkg/update/result_test.go

• image ref
  • gives each component of an image ref
  • deals with hostnames and digests
• update results
  • deduplicates images
  • collects images by object

pkg/update/update_test.go

• Update image via kyaml/setters2
  • updates the image marked with the image policy (setter) ref
  • gives the result of the update

[squaremo]

To be honest, I prefer the previous formulation (using ginkgo), which was more readable. To wit: I'm finding it difficult to check whether the same things are tested -- it's fairly obvious in the ginkgo version because the setting up and tearing down is separate to the assertions, and the assertions are separate to each other.

Sometimes the reliance of Ginkgo on closures makes it hard to see what's going on, it's true. But flattening it all out is two steps backward for one step forward, I reckon. If ginkgo absolutely must be removed, then I'd like at least to see some of the same structure preserved, where setup/teardown code is shared.

[darkowlzz]

To be honest, I prefer the previous formulation (using ginkgo), which was more readable. To wit: I'm finding it difficult to check whether the same things are tested -- it's fairly obvious in the ginkgo version because the setting up and tearing down is separate to the assertions, and the assertions are separate to each other.

Sometimes the reliance of Ginkgo on closures makes it hard to see what's going on, it's true. But flattening it all out is two steps backward for one step forward, I reckon. If ginkgo absolutely must be removed, then I'd like at least to see some of the same structure preserved, where setup/teardown code is shared.

I had that feeling too initially while refactoring it and felt like it'd be better to restructure the tests in a completely different way. But being my first contribution to this code base, I tried to stick to the existing test layout as much as possible to keep the changes familiar and avoid dropping something entirely different. Was hoping to restructure more later once we move completely away from ginkgo and as I get more familiar with the code base. I believe it can be refactored in ways to make it better with helpers for common tasks. The checklist above would be very helpful in restructuring it and ensuring all the checks are retained. Now that we know what a direct conversion of the tests look like, I can give it another try with something different to help make it more readable and easy to maintain 🙂 .
Thanks for the review and very helpful feedback.

[hiddeco]

If ginkgo absolutely must be removed, [..]

It is not a strict requirement (nothing really is – I am just trying to advise and align). However, the kustomize-controller which has been ported awhile ago and seen multiple attributions since, has proven that once the tests are restructured it is much easier to add new tests, especially for new contributors.

[squaremo]

I believe it can be refactored in ways to make it better with helpers for common tasks.

I think we can get something better than both ginkgo and "flat" tests, by factoring out some bits (which I found awkward with ginkgo) and using closures judiciously to share setup code.

The particular file I was looking at when I wrote the comment above was controllers/update_test.go, which mixes a lot of what were individual tests together into one long section in TestImageAutomation_commit_spec. I think the commit message, path and signing should really have their own tests, and just share a bit of setup.

TestImageAutomation_e2e after that uses a table appropriately, I think -- though it's still very long and could surely have some bits factored out (this was true of the ginkgo code too, it's not a new problem :-).

[darkowlzz]

This is still not finished, but any feedback in the new structure would be very helpful.
I've added some helper functions for the common tasks and structured the tests in two parts. The first part sets up the git server, initializes a repo, creates GitRepository resource for the git repo and creates ImagePolicy. The second part is the actual tests in subtests where depending on the test case, ImageUpdateAutomation object is created and scenario specific operations are performed.
I can also try taking the common code of the first part and create a function out of it, but avoiding that for now, unless we are sure that such helper function will be very helpful and not hide too much to affect the readability.

Most of the checks in the checklist for update_test.go should be clear and easy to verify with this change.

I would recommend reading https://github.com/fluxcd/image-automation-controller/blob/435171316d0f331590e8047559aa7e1ec95ca5e2/controllers/update_test.go instead of the diff. To help myself, I've added a lot of comments. Will remove them once I'm finished.

[darkowlzz]

This is ready for another review. I hope this is much easier than before to read. But I'm open for more restructuring and simplification.

The DCO check is failing with:

Commit sha: 3917048, Author: Michael Bridgen, Committer: Sunny; Expected "Michael Bridgen mikeb@squaremobius.net", but got "Michael Bridgen mikeb@weave.works".

Is it okay if I edit @squaremo 's commit signoff message myself?

[squaremo]

Is it okay if I edit @squaremo 's commit signoff message myself?

That was me doing the PR in a directory where I hadn't explicitly set the commit author, sorry about that. Probably better if the author is corrected (to "Michael Bridgen michael@weave.works", rather than the sign-off.

[darkowlzz]

Probably better if the author is corrected (to "Michael Bridgen michael@weave.works", rather than the sign-off.

Changed it. Thanks.

[squaremo]

The first part sets up the git server, initializes a repo, creates GitRepository resource for the git repo and creates ImagePolicy. The second part is the actual tests in subtests where depending on the test case, ImageUpdateAutomation object is created and scenario specific operations are performed.

It looks like this scheme is working well in controllers/update_test.go. The first few tests do exactly the same setup (I imagine this is why you mentioned more factoring-out!); perhaps you could use a callback:

func TestSignedCommit(t *testing.T) {
  testWithRepoAndImagePolicy(NewWithT(t), func(tmp, localRepo) {
    policyKey := types.NamespacedName{
      Name:      imagePolicyName,
      Namespace: namespace,
    }
    commitInRepo(g, repoURL, branch, "Install setter marker", func(tmp string) {
      g.Expect(replaceMarker(tmp, policyKey)).To(Succeed())
    })
    // ...
  })
}

(I've glossed over a lot of detail there of course, it's possible that the callback would need too many arguments)

[darkowlzz]

Consolidated the common test setup code into testWithRepoAndImagePolicy() which also accepts a testFunc callback to run the test assertions after the setup.
It did result in passing a lot of arguments, but I guess it's okay the way it is now, manageable.

Ready for a review.

[squaremo]

I'm satisfied that the rewritten suite contains the same tests (thanks for making that pretty easy to check!). The new structure, with subtests sharing setup and common funcs factored out, is at least as good as the structure when using ginkgo I think, and has less magic and fewer lines 👍

I made a PR with a couple of suggestions, but I don't consider them prerequisites to merging this PR.

Thanks for persisting with this work, Sunny 🍍

[darkowlzz]

Rebased, merged the readability improvements by @squaremo and updated as per the last few suggestions to make the e2e tests more independent.