helm unittests

[MichaelSp]

Pull Request

Description of the change

run unittests for the helm-chart during PR checks

Benefits

better quality

Possible drawbacks

??

Applicable issues

no

Additional information

Checklist

• DCO has been signed off on the commit.

[provokateurin]

Please explain what this does exactly. I'm not familiar with helm-unittest and don't even know what the tool is supposed to do.

[MichaelSp]

https://github.com/helm-unittest/helm-unittest is a tool to help write unit tests for helm charts. It allows to write unit tests which help ensure software quality on the unit level.

Here is a very good article on what unit-tests actually are: https://martinfowler.com/bliki/UnitTest.html

btw: I also added a check that runs during PR actions

[provokateurin]

I know what unit tests are, but what is tested in the context of Helm Charts?

[MichaelSp]

In this https://github.com/nextcloud/helm/blob/2c871d9bdfbddc748cee7a0aa575fbc32a04b0f2/charts/nextcloud/tests/defaults_test.yaml I test the output of the helm-chart for a default value file. I only provide values to

• Chart.Version - https://github.com/nextcloud/helm/blob/2c871d9bdfbddc748cee7a0aa575fbc32a04b0f2/charts/nextcloud/tests/defaults_test.yaml#L7C3-L7C10
• Chart.appVersion -

  helm/charts/nextcloud/tests/defaults_test.yaml

  Line 8 in 2c871d9

  appVersion: 1.2.3

• and metrics.token -

  helm/charts/nextcloud/tests/defaults_test.yaml

  Line 12 in 2c871d9

  metrics.token: "abcd"

This is necessary to generate stable output (it should not change between test runs even if the appVersion changes.
The output is matched and compared to the snapshot to make sure future changes to the templates or values don't change the output in an unexpected way.

If a future change results in a desired change of the output, the snapshot can be updated: helm unittest charts/nextcloud -u.

This is only a very basic test. You can increase coverage by defining more tests and more test-suites with different combinations of values to cover the logic in the templates and make sure use-cases like nginx, cron, mail, etc result in the correct configuration.

Using helm unittest is a best-practice in helm-charts.

[provokateurin]

Sounds like a good idea, @jessebot @tvories what do you think? I think for merging this it would be good to have a bit more coverage of features

[jessebot]

Using helm unittest is a best-practice in helm-charts.

I agree tests are good, but I look at quite a few helm charts for a living and I haven't actually seen this tool in use before now.

I feel like a test of just installing on k3s/k3d makes more sense and is easier to understand since we still don't actually have that across k8s distros other than kind. It would also be another thing that would then need to pass before we can merge things, and we already have some trouble doing that due to our other requirements such as DCO, linting, chart version bump, and base installation on kind.

If a future change results in a desired change of the output, the snapshot can be updated: helm unittest charts/nextcloud -u.

The other issue is that we have to keep it up to date, though we could add this to the contributing docs?

Still OK to merge this, because I agree, more feature test coverage would be nice, but wanted to note some very minor downsides. I'd have to spend some time with it before I could help add anything else.

If the helm unit-tests are common enough, I guess we could put out some GitHub issues asking other community members to help us add specific features tests, but if we do that, there's also the question of how we run only necessary unit tests after each push? Otherwise, every push will take like 5+ minutes as we add more feature unit tests, which will also make the back and forth on PRs take a bit longer.

I approved it for now, so that I am not blocking, but will wait till others chime in before merging.

Update: after thinking about it more, it does make sense to have more tests though. I think I was just on about how long it can sometimes take to get things merged. Tests make sense though.

[MichaelSp]

there's also the question of how we run only necessary unit tests after each push? Otherwise, every push will take like 5+ minutes as we add more feature unit tests, which will also make the back and forth on PRs take a bit longer.

The unit-tests even for larger test suites are pretty fast (<5sec). I bet they will be always faster than starting a kind-cluster and deploy it.

[provokateurin]

I assume the unit tests just run helm template with a given values file and compare the output with the fixture? Then that would be very fast and scalable indeed.

[jessebot]

awesome, then that really solve the bulk of my issues :)

[jessebot]

if/when this is merged, I've created #469 for a contributing doc to be updated as we need :)

[provokateurin]

@jessebot @MichaelSp what is the status? I think it would be good to start adding tests soon, even if there are just a few in the beginning.

[jessebot]

I haven't tested this locally, but let me try!

[provokateurin]

I'd say the charts/__snapshots__ dir needs to be added to the filter so that the CI runs when they change to ensure everything is still correct.

[jessebot]

I regenerated the snapshot for today and rebased off of main, but I have one last comment below to take care of before we can merge this.

[jessebot]

We also probably need to add a note about these tests to the CONTRIBUTING doc. It should include at least how to test locally, and how to regenerate the snapshots.

[MichaelSp]

Finally I found the time to come back to this and:

• add the requested hint to CONTRIBUTING.md
• Test case 0: No change - this PR
• Test case 1: a valid helm template change
• Test case 2: invalid helm template change

[volker-raschek]

Hello @provokateurin ,
I have experienced frequent recursions in my cluster regarding the Kubernetes helm chart.

Just recently I wrote a helm chart for the prometheus-postgres-exporter which covers some unit tests with the helm plugin unittest. Furthermore, it also includes linters, a generated README and other good tooling. The plugin is also used on other projects like gitea, where I recently opened a PR.

For this reason I can only recommend the endeavour of @MichaelSp and wonder why the plugin unittest was not looked at more closely a year ago, to avoid recursion.

[MichaelSp]

anything missing @provokateurin @jessebot @tvories ?

[MichaelSp]

Thanks for you approval @jessebot
Anything else missing @tvories @provokateurin ?

[MichaelSp]

Hey @wrenix can you pls have a look at this PR? AFAIK there is only a review missing.

[MichaelSp]

re-based again. Please check and approve

[MichaelSp]

@wrenix let me know if you want me to rebase again....

[volker-raschek]

Hi @MichaelSp,
can you please rebase your PR? The logs are no longer available.

Alternatively, can you or @wrenix retrigger the CI pipeline?

[MichaelSp]

Happy to see some movement with this. Thanks @volker-raschek