search

nextcloud / helm

A community maintained helm chart for deploying Nextcloud on Kubernetes.
GNU Affero General Public License v3.0
325 stars 264 forks source link

helm unittests #462

Open MichaelSp opened 11 months ago

MichaelSp commented 11 months ago

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

provokateurin commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

MichaelSp commented 11 months ago

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

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 10 months ago

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 commented 10 months ago

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

jessebot commented 10 months ago

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

provokateurin commented 8 months ago

@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 commented 4 months ago

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

provokateurin commented 4 months ago

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 commented 4 months ago

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.

jessebot commented 2 months ago

@provokateurin do we still want to move this forward? 🤔 I know we have other ci tests, so not sure if we still want this. Up to you!

provokateurin commented 2 months ago

I think this might still be a good idea to prevent any accidental issues with the templating that otherwise might go undetected.

jessebot commented 2 months ago

Okie dokie, we'll need the conflicts resolved, and then we can try it by doing a temporary test commit here like we did on the last PR?

Update: Fixed conflicts and now it should run. I also still think we should have a section in the CONTRIBUTING.md on how to update the tests if they get out of date. @MichaelSp if could do that, I can get this merged, otherwise, I'll circle back to this PR and do it when I have a free moment 🙏

jessebot commented 2 months ago

Oh, another thing before we merge, the 3 test scenarios we want to check were: