Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for flux trace command #1671

Merged
merged 1 commit into from
Aug 4, 2021
Merged

Conversation

allenporter
Copy link
Contributor

@allenporter allenporter commented Aug 1, 2021

Add tests for flux trace command that fake out the kubernetes client,
load objects from a yaml file and create them in the client, and
assert on the output of the trace command to an expected golden file.

Follow up from the suggestions in PR #1626 which suggested that additional
testing would be helpful. This test approach is modeled after the helm command tests.

This required some changes to the kubernetes client setup to make it possible to use a fake. If we agree this pattern makes sense, it can be applied to other commands (kube client creation, and test library setup could be moved to a common place). An alternative could be to use envtest which seemed much more heavyweight. Another alternative could be to declare all the objects in go code instead of in yaml.

To set your expectations: I'm new to go, flux, and k8s libraries so am not familiar with many existing conventions, but very happy to adjust as needed.

Ref: #1603

@allenporter allenporter marked this pull request as draft August 1, 2021 07:42
@allenporter allenporter marked this pull request as ready for review August 2, 2021 15:17
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! @allenporter can you please move the test files inside testdata/trace, this way we'll have a dir per command. Thanks

@stefanprodan
Copy link
Member

@allenporter can you rebase with main, squash both commits and signoff your commit please.

Add tests for flux trace command that fake out the kubernetes client,
load objects from a yaml file and create them in the client, and
assert on the output of the trace command to an expected golden file.

This is a follow up from the suggestions in PR fluxcd#1626 which suggested that additional
testing would be helpful. This test approach is modeled after the helm command tests.

This required some changes to the kubernetes client setup to make it
possible to use a fake. If we agree this pattern makes sense, it can be
applied to other commands.

Signed-off-by: Allen Porter <allen@thebends.org>
@allenporter
Copy link
Contributor Author

I see you also recommended following this pattern for the e2e tests too which is cool.

As I am thinking about adding coverage of more commands, wanted to share my early thoughts. In particular, just want to make sure this is easy to maintain:

  • Will it be easy to detect bugs in yaml object files? Seems worth it for now, but not sure how this plays out
  • Will maintaining the yaml files require lots of small changes across many files as new fields are added?
  • Should simple test cases just inline the errors instead of using a golden output? (e.g. "object kind is required (--kind)", "object apiVersion is required (--api-version", etc). I lean towards yes.

@stefanprodan
Copy link
Member

stefanprodan commented Aug 4, 2021

Will it be easy to detect bugs in yaml object files?

We could use kubeval along with the Flux API specs to validate the inputs, e.g. https://github.com/fluxcd/flux2-kustomize-helm-example/blob/main/scripts/validate.sh

Will maintaining the yaml files require lots of small changes across many files as new fields are added?

It depends on the test case, Flux APIs are stable, that means all new fields are either optional or have defaults. When adding a new field to a CRD, if that field is also added to the CLI create command, then yes, the golden file should be updated.

Should simple test cases just inline the errors instead of using a golden output? (e.g. "object kind is required (--kind)", "object apiVersion is required (--api-version", etc). I lean towards yes.

Yes I agree, no point in creating tones of one-line golden files. I think golden files should be used for when the output is a table view or a list view.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @allenporter 🥇

@stefanprodan stefanprodan merged commit 2290880 into fluxcd:main Aug 4, 2021
@stefanprodan
Copy link
Member

@allenporter FYI #1682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants