Basic Unit Test Infrastructure #283
Conversation
* Helm Python Testing * helm change * add back args Partial Commit Extracted From: https://github.com/apache/airflow
* All k8s resources should have global labels * All k8s object must comply with JSON Schema Partial Commit Extracted From: https://github.com/apache/airflow
…iguration in Helm Chart (apache/airflow#12164) * Enable provisionning of extra secrets and configmaps in helm chart Added 2 new values: * extraSecrets * extraConfigMaps Those values enable the provisionning of ConfigMaps and secrets directly from the airflow chart. Those objects could be used for storing airflow variables or (secret) connections info for instance (the plan is to add support for extraEnv and extraEnvFrom later). Docs and tests updated accordingly. * Add support for extra env and envFrom items in helm chart Added 2 new values: * extraEnv * extraEnvFrom Those values will be added to the defintion of airflow containers. They are expected to be string (they can be templated). Those new values won't be supported by "legacy" kubernetes executor configuration (you must use the pod template). Therefore, the value 'env' is also deprecated as it's kind of a duplicate for extraEnv. Docs and tests updated accordingly. Partial Commit Extracted From: https://github.com/apache/airflow
…#13183) Partial Commit Extracted From: https://github.com/apache/airflow
The indentation under `web.annotations` was wrong (6 leading spaces
on first line, 4 on the rest) leading to
Error converting YAMl to JSON: yaml: line 32: did not find expected key
when you run helm chart with value `ingres.web.annotations`
Partial Commit Extracted From: https://github.com/apache/airflow
…3577) Calling `yaml.load_all()` without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details. `yaml.full_load_all` instead use load_all() with Fullloader Partial Commit Extracted From: https://github.com/apache/airflow
…ache/airflow#13209) * Upgrade KEDA to v2.0.0 and add support for worker persistence KEDA 2.0.0 introduces support for StatefulSets, so we can now remove the constraint that worker persistence must be disabled. Co-authored-by: Daniel Standish <dstandish@users.noreply.github.com> Partial Commit Extracted From: https://github.com/apache/airflow
* Drop support for SequentialExecutor in Helm Chart * fixup! Drop support for SequentialExecutor in Helm Chart * fixup! fixup! Drop support for SequentialExecutor in Helm Chart * fixup! fixup! fixup! Drop support for SequentialExecutor in Helm Chart Partial Commit Extracted From: https://github.com/apache/airflow
* Allow helm chart tests to run in parallel The helm chart tests are pretty slow when run sequentially. Modifying them so they can be run in parallel saves a lot of time, from 10 minutes to 3 minutes on my machine with 8 cores. The only test that needed modification was `test_pod_template_file.py`, as it temporarily moves a file into the templates directory which was causing other tests to fail as they weren't expecting any objects from that temporary file. This is resolved by giving the pod_template_file test an isolated chart directory it can modify. `helm dep update` also doesn't work when it is called in parallel, so the fixture responsible for running it now ensures we only run it one at a time. * Enable parallelism for helm unit tests in CI Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com> Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com> Partial Commit Extracted From: https://github.com/apache/airflow
- Add `INSTALL`, `LICENSE` and `NOTICE` files as requried by ASF. - Updated Docs to point to Soon To Be Published Docs Partial Commit Extracted From: https://github.com/apache/airflow
[The k8s schema repository that has been used for chart pytest has gone stale with no updates in 14 months](https://github.com/instrumenta/kubernetes-json-schema). There are no new updates beyond 1.18.1, and [PRs for updates are not being merged](https://github.com/instrumenta/kubernetes-json-schema/pulls). Airflow is using a more active fork [in pre-commit](https://github.com/apache/airflow/blob/main/.pre-commit-config.yaml#L571), so this change uses [that updated fork](https://github.com/yannh/kubernetes-json-schema) in chart pytests too. This updated fork's latest schema is 1.21.1, and has had changes within the last month. Partial Commit Extracted From: https://github.com/apache/airflow
We've agreed during the voting process that Pylint support should be disabled: https://lists.apache.org/thread.html/r9e2cc385db8737ec0874ad09872081bd083593ee29e8303e58d21efb%40%3Cdev.airflow.apache.org%3E This PR: * removes all # pylint comments * removes pylint pre-commits and related scripts/files * removes CI jobs running pylint checks * removes documentation about pylint * removes unnecessary #noga (adds pre-commit for that) * fixes some remaining pydocstyle errors after removing #noqa's Partial Commit Extracted From: https://github.com/apache/airflow
We were on 6.3.12 and the current latest version is 10.5.3. We have dropped support for Helm 2 already so Helm 3 users won't be affected. Secondly this postgres should only used for development, not production. Partial Commit Extracted From: https://github.com/apache/airflow
Partial Commit Extracted From: https://github.com/apache/airflow
…flow#18964) This PR adds codespell to the pre-commit hooks. This will specifically help us a bit in resolving sphinx errors. From the project page: It does not check for word membership in a complete dictionary, but instead looks for a set of common misspellings. Therefore it should catch errors like "adn", but it will not catch "adnasdfasdf". This also means it shouldn't generate false-positives when you use a niche term it doesn't know about. This means the sphinx errors are not solved completely. Partial Commit Extracted From: https://github.com/apache/airflow
…#19379) Partial Commit Extracted From: https://github.com/apache/airflow
Without this, one cannot use namespaces whose names consist of only numbers. Partial Commit Extracted From: https://github.com/apache/airflow
pull-request-size
bot
added
the
size/XL
|
Please note: I added 100 lines in python files that I authored. I also added 15 of the lines in NOTICE and all 49 lines of the gha workflow. The other 374 lines (201 in the Apache 2.0 LICENSE file) were copied from apache/airflow. So, if the pull-request-size bot only evaluated the lines I wrote, this would be a |
There was a problem hiding this comment.
Thanks for putting the experiment together and ideas here!
While I understand the approach Apache Airflow has chosen, I believe they're driven by different needs. Just quickly take a look at the https://github.com/apache/airflow/tree/main/chart/templates and development velocity.
A somewhat similar approach would be going with the https://github.com/instrumenta/helm-conftest + https://github.com/open-policy-agent/conftest via Open Policy Agent (https://www.openpolicyagent.org/). That would be a true cloud-native CNCF approach. In the Airflow PR, they mention terratest which is relevant to this approach, too.
However, both golang and python are overkill for this specific stackstorm-ha project.
Let's compare these 2 PRs:
- https://github.com/jenkinsci/helm-charts/pull/451/files
- https://github.com/apache/airflow/pull/21484/files
Adding a testing case for the templates shouldn't be harder than writing the templates themselves.
I believe, our situation is that we have different needs and priorities:
- Low development velocity. After stabilizing the helm chart there will be very few things to add.
- Simplicity & Maintainability
- No really complex scenarios
- Creating unit tests should be easy as possible
- show by example (a copy-paste from the other file)
- applies to external contributors
- Adding unit tests is a bonus. The chart is already successful in its state, even if we don't add the unit testing framework here.
And so navigating all the unit test infrastructure after, for example, 6 months of not touching this codebase should be straightforward. Requesting a contributor to add a quick unit test based on the examples we have should be easy as copy-pasting a couple of lines.
We don't need all the possible flexibility. We need to really add a few YAML lines in deployment and connect them with a few respective YAML lines in the tests. Mentally, it's very easy and does the job.
https://github.com/quintush/helm-unittest is known as a working fork for helm unittest plugin in the K8s community, - that should work fine. See https://github.com/quintush/helm-unittest/blob/master/test/data/v3/basic/tests/ and https://github.com/jenkinsci/helm-charts/tree/main/charts/jenkins/unittests/. They provide a good overview of examples.
| def test_annotations( | ||
| chart_resources: List[Dict[str, Any]], expected_annotations: Dict[str, str] | ||
| ) -> None: | ||
| """Make sure that annotations are working""" | ||
| # based on apache/airflow:chart/tests/tests_annotations::test_annotations_are_added | ||
|
|
||
| # there's only one resource because we sepecified show_only. | ||
| assert len(chart_resources) == 1 | ||
| service_account = chart_resources[0] | ||
|
|
||
| for name, value in expected_annotations.items(): | ||
| assert name in service_account["metadata"]["annotations"] | ||
| assert value == service_account["metadata"]["annotations"][name] |
There was a problem hiding this comment.
I had experience working with somewhat similar Unit and Integration Tests written for K8s in another project for the LF exams.
While it looks powerful at the first though, I don't think it's the best fit for a simple project like stackstorm-ha.
The disadvantage to me is spaghetti and confusion when debugging, reading these tests, maintaining them or even asking someone to create a test. The amount of indirection and complexity is excessive.
At this point, I'd better go with the simplest possible approach. Even doing helm template, yq or jq with jsonpath that could be ran from the CLI would be a better, when it's much easier to debug and connect the dots from Helm template itself to the Helm template test.
Yaml to Yaml is simple as that.
There was a problem hiding this comment.
Doing things from the CLI is not simpler! It's a royal pain to maintain tests in bash and I hate writing tests that way.
There was a problem hiding this comment.
So, yet again, you and I have different definitions of simple.
|
Found where I saw the The advantage I think there is no need to leave the CLI and easy to reproduce and debug. Connecting dots from the Helm template to the test results is easy and so the room for error is lower. And we're already using BATS for integration & smoke testing. https://github.com/quintush/helm-unittest represents things a bit nicer and more user-friendly with YAML. That's Python vs Golang unit tests would be |
Yes. This is a big problem. We MUST speed up the velocity. It is far too slow and takes far too long to get things reviewed and merged. The slower it is the less community engagement we can get. |
Nope. I can't get the features I'm trying to add merged until you're confident everything is covered by unit tests. It is not a bonus. We need it so we can speed up velocity. |
|
🙇 I opened #284 to replace this one. |
This is part of #28.
Using
pytest, this introduces a way to unit test the rendered templates.For now, there is only one test showing how the infra works.
After this is merged, we can start adding tests for other template features.
Why not helm-unittest?
These links explain why apache/airflow decided to abandon helm-unittest in favor of a pytest-based chart testing approach:
In short, I agree with their logic. StackStorm is a python-based solution. Helm offers some integration-style testing apparatus, but unit tests are rather difficult.
Different people have been maintaining helm-unittest, so there is no one canonical "upstream" repo for it. You have to search and figure out which one is the most maintained.
Plus, the YAML-based tests will be something else to learn and using python to test should allow for more expressive tests.
Why pytest?
nosetest is not well maintained and is less well known. Eventually I want to replace nosetest with pytest in the main st2 repo. I've done quite a bit of preparatory work in that direction.
I do not want to introduce yet another unittest dependecy that will have to be migrated in the future.
I also strongly dislike the unittest-style
self.assert*and I find debugging unittest tests very annoying.Pytest-enhanced raw asserts are much more straightforward.
apache/airflow has an odd combo of unittest classes that they run with pytest. I avoid Unittest wherever possible, so I created pytest fixtures in this PR.
tests/helm_template_generator.py
tests/helm_template_generator.py was developed by apache/airflow.
That project is Apache 2.0 licensed, so we can reuse it here.
To maintain authorship data, I imported the git history (using this) for this file:
https://github.com/apache/airflow/blob/main/chart/tests/helm_template_generator.py
I also included the LICENSE file to make sure licensing is clear in our chart repo, and a copy of the NOTICE file from the apache/airflow repo.