Skip to content

Replace unittests in helm chart tests by pure pytest [Wave-2] #26992

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.

Sign up for GitHub

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

Jump to bottom
Merged
jedcunningham merged 2 commits into from
Oct 19, 2022
Merged

Replace unittests in helm chart tests by pure pytest [Wave-2] #26992

jedcunningham merged 2 commits into from
Oct 19, 2022

Conversation

@Taragolis
Copy link
Contributor

@Taragolis Taragolis commented Oct 11, 2022

Attempt to replace unittests.TestCase by native pytest classes for all charts tests.

All changes are straight forward:

  • Get rid of unittests.TestCase
  • Replace decorator parameterized.expand by pytest.mark.parametrize. I cant see any benefits if compare to builtin pytest.
  • Rename ClassNameTest to TestClassName
  • Replace unittests.TestCase.assertRaises by pytest.raises

Except two modules

  • tests/charts/test_pod_template_file.py - move fixture out of the class
  • tests/charts/test_rbac.py - unittests.TestCase.assertCountEqual has no equivalent in pytest, so i replace by compare sorted lists

Tested locally in breeze and did not have any differences if compare to current (local) main

This PR (run 5 times)

792 passed, 1 warning in 226.74s (0:03:46)
792 passed, 1 warning in 226.18s (0:03:46)
792 passed, 1 warning in 226.10s (0:03:46)
792 passed, 1 warning in 226.78s (0:03:46)
792 passed, 1 warning in 227.63s (0:03:47)

Main (run 5 times)

792 passed, 1 warning in 225.29s (0:03:45)
792 passed, 1 warning in 225.06s (0:03:45)
792 passed, 1 warning in 226.00s (0:03:46)
792 passed, 1 warning in 225.51s (0:03:45)
792 passed, 1 warning in 225.99s (0:03:45)

Just reminder that this PR remove all usage pytest.mark.parametrize within the charts tests so it can affect any of current helm-chart PRs which not merged yet and vice versa.

@Taragolis
Copy link
Contributor Author

Taragolis commented Oct 11, 2022

Hmmm.... Tests / Python unit tests for Helm chart (pull_request) Skipped 👀

@uranusjr
Copy link
Member

uranusjr commented Oct 12, 2022

I think it’s because the tests are currently only run when something in the actual Helm Chart changes, but not when the tests change. Probably should fix those in the CI config. (The configs are in breeze/.../selective_checks.py (don’t recall the exact path to the module)

@Taragolis
Copy link
Contributor Author

Taragolis commented Oct 12, 2022

@uranusjr Thanks! Seems I found that breeze module

airflow/dev/breeze/src/airflow_breeze/utils/selective_checks.py

Lines 112 to 116 in d9db89a

FileGroupForCi.HELM_FILES: [
"^chart",
"^airflow/kubernetes",
"^tests/kubernetes",
],

airflow/dev/breeze/src/airflow_breeze/utils/selective_checks.py

Lines 467 to 469 in d9db89a

@cached_property
def needs_helm_tests(self) -> bool:
return self._should_be_run(FileGroupForCi.HELM_FILES) and self._default_branch == "main"

Should I extend the list within the FileGroupForCi.HELM_FILES group by add ^tests/charts?

@uranusjr
Copy link
Member

uranusjr commented Oct 12, 2022

I think it makes sense to add them to the list. (While you’re at it, let’s also convert them to raw-strings (r-prefix) and fix the API_CODEGEN_FILES above—the v1.yaml pattern there is missing an escape (it should be v1\.yaml instead)

@Taragolis Taragolis force-pushed the helm-chart-replace-unittest-by-pytest-wave-2 branch from 37bd75a to 7d577ed Compare October 16, 2022 14:24
uranusjr
uranusjr approved these changes Oct 17, 2022
View reviewed changes
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

🚀

@jedcunningham jedcunningham merged commit 03b879d into apache:main Oct 19, 2022
@Taragolis Taragolis deleted the helm-chart-replace-unittest-by-pytest-wave-2 branch October 19, 2022 16:23
@potiuk
Copy link
Member

potiuk commented Oct 26, 2022

❤️

@Taragolis Taragolis mentioned this pull request Nov 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uranusjr uranusjr uranusjr approved these changes

@jedcunningham jedcunningham jedcunningham approved these changes

@potiuk potiuk Awaiting requested review from potiuk potiuk is a code owner

@ashb ashb Awaiting requested review from ashb ashb is a code owner

Assignees

No one assigned

Labels

area:dev-tools

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Taragolis @uranusjr @potiuk @jedcunningham