From ce561b6bc4bfebeb93785b3e9542a8f221388974 Mon Sep 17 00:00:00 2001 From: Vincent Beck Date: Thu, 20 Apr 2023 11:26:07 -0600 Subject: [PATCH 1/9] Merge local pre-commit hooks in one list --- .pre-commit-config.yaml | 56 ++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4a152a6c1dd7a..6408ce8ec0582 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -153,35 +153,6 @@ repos: name: Run black (Python formatter) args: [--config=./pyproject.toml] exclude: ^.*/.*_vendor/|^airflow/contrib/ - - repo: local - hooks: - - id: update-common-sql-api-stubs - name: Check and update common.sql API stubs - entry: ./scripts/ci/pre_commit/pre_commit_update_common_sql_api_stubs.py - language: python - files: ^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.pyi?$ - additional_dependencies: ['rich>=12.4.4', 'mypy==1.2.0', 'black==22.12.0', 'jinja2'] - pass_filenames: false - require_serial: true - - id: update-black-version - name: Update black versions everywhere - entry: ./scripts/ci/pre_commit/pre_commit_update_black_version.py - language: python - files: ^.pre-commit-config.yaml$ - additional_dependencies: ['pyyaml'] - pass_filenames: false - require_serial: true - - id: ruff - name: ruff - language: python - require_serial: true - pass_filenames: true - # Since ruff makes use of multiple cores we _purposefully_ don't run this in docker so it can use the - # host CPU to it's fullest - entry: ruff --fix --no-update-check --force-exclude - additional_dependencies: ['ruff==0.0.226'] - files: \.pyi?$ - exclude: ^.*/.*_vendor/|^tests/dags/test_imports.py - repo: https://github.com/asottile/blacken-docs rev: 1.13.0 hooks: @@ -285,6 +256,33 @@ repos: - --exclude-file=.codespellignorelines - repo: local hooks: + - id: update-common-sql-api-stubs + name: Check and update common.sql API stubs + entry: ./scripts/ci/pre_commit/pre_commit_update_common_sql_api_stubs.py + language: python + files: ^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.pyi?$ + additional_dependencies: [ 'rich>=12.4.4', 'mypy==1.2.0', 'black==22.12.0', 'jinja2' ] + pass_filenames: false + require_serial: true + - id: update-black-version + name: Update black versions everywhere + entry: ./scripts/ci/pre_commit/pre_commit_update_black_version.py + language: python + files: ^.pre-commit-config.yaml$ + additional_dependencies: [ 'pyyaml' ] + pass_filenames: false + require_serial: true + - id: ruff + name: ruff + language: python + require_serial: true + pass_filenames: true + # Since ruff makes use of multiple cores we _purposefully_ don't run this in docker so it can use the + # host CPU to it's fullest + entry: ruff --fix --no-update-check --force-exclude + additional_dependencies: [ 'ruff==0.0.226' ] + files: \.pyi?$ + exclude: ^.*/.*_vendor/|^tests/dags/test_imports.py - id: replace-bad-characters name: Replace bad characters entry: ./scripts/ci/pre_commit/pre_commit_replace_bad_characters.py From 15fd059b8ec5264301e5e185af108395a243c9e7 Mon Sep 17 00:00:00 2001 From: Vincent Beck Date: Thu, 20 Apr 2023 11:39:41 -0600 Subject: [PATCH 2/9] Add pre-commit hook to ban usage of unittest.TestCase in unit tests --- .pre-commit-config.yaml | 7 +++ .../pre_commit_unittest_testcase.py | 59 +++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100755 scripts/ci/pre_commit/pre_commit_unittest_testcase.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 6408ce8ec0582..b863fbe609329 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -943,4 +943,11 @@ repos: pass_filenames: false files: ^airflow/migrations/versions/.*\.py$|^docs/apache-airflow/migrations-ref\.rst$ additional_dependencies: ['rich>=12.4.4'] + - id: check-tests-unittest-testcase + name: Check that unit tests are not inheriting from unittest.TestCase + entry: ./scripts/ci/pre_commit/pre_commit_unittest_testcase.py + language: python + pass_filenames: true + files: ^tests/.*\.py$ + additional_dependencies: [ 'rich>=12.4.4' ] ## ONLY ADD PRE-COMMITS HERE THAT REQUIRE CI IMAGE diff --git a/scripts/ci/pre_commit/pre_commit_unittest_testcase.py b/scripts/ci/pre_commit/pre_commit_unittest_testcase.py new file mode 100755 index 0000000000000..951681f76c178 --- /dev/null +++ b/scripts/ci/pre_commit/pre_commit_unittest_testcase.py @@ -0,0 +1,59 @@ +#!/usr/bin/env python +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import re +import sys +from pathlib import Path + +from rich.console import Console + +if __name__ not in ("__main__", "__mp_main__"): + raise SystemExit( + "This file is intended to be executed as an executable program. You cannot use it as a module." + f"To run this script, run the ./{__file__} command [FILE] ..." + ) + + +console = Console(color_system="standard", width=200) + + +def _check_file(file: Path) -> list: + content = file.read_text() + return re.findall(r"class[^(]+\(unittest.TestCase\)\:", content) + + +def _join_with_newline(list_): + return "\n".join(list_) + + +if __name__ == "__main__": + error_list = [] + for file in sys.argv[1:]: + matches = _check_file(Path(file)) + if matches: + error_list.append((file, matches)) + if error_list: + error_message = "\n".join([f"{f}: \n{_join_with_newline(m)}" for f, m in error_list]) + console.print( + f""" +[red]Found tests inheriting from unittest.TestCase. Please use pytest instead.[/] +{error_message} +""" + ) + sys.exit(1) From 9b8daad4a3762195678138f42349495625397fcc Mon Sep 17 00:00:00 2001 From: Vincent Beck Date: Thu, 20 Apr 2023 12:01:36 -0600 Subject: [PATCH 3/9] Add the pre-commit check to the documentation --- .pre-commit-config.yaml | 2 +- STATIC_CODE_CHECKS.rst | 2 + .../src/airflow_breeze/pre_commit_ids.py | 1 + images/breeze/output-commands-hash.txt | 2 +- images/breeze/output_static-checks.svg | 108 +++++++++--------- 5 files changed, 59 insertions(+), 56 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index b863fbe609329..155984f9b2b25 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -944,7 +944,7 @@ repos: files: ^airflow/migrations/versions/.*\.py$|^docs/apache-airflow/migrations-ref\.rst$ additional_dependencies: ['rich>=12.4.4'] - id: check-tests-unittest-testcase - name: Check that unit tests are not inheriting from unittest.TestCase + name: Check that unit tests do not inherit from unittest.TestCase entry: ./scripts/ci/pre_commit/pre_commit_unittest_testcase.py language: python pass_filenames: true diff --git a/STATIC_CODE_CHECKS.rst b/STATIC_CODE_CHECKS.rst index 2daab5128a8e8..a8f28cac79663 100644 --- a/STATIC_CODE_CHECKS.rst +++ b/STATIC_CODE_CHECKS.rst @@ -227,6 +227,8 @@ require Breeze Docker image to be build locally. +-----------------------------------------------------------+------------------------------------------------------------------+---------+ | check-system-tests-tocs | Check that system tests is properly added | | +-----------------------------------------------------------+------------------------------------------------------------------+---------+ +| check-tests-unittest-testcase | Check that unit tests do not inherit from unittest.TestCase | * | ++-----------------------------------------------------------+------------------------------------------------------------------+---------+ | check-urlparse-usage-in-code | Don't use urlparse in code | | +-----------------------------------------------------------+------------------------------------------------------------------+---------+ | check-xml | Check XML files with xmllint | | diff --git a/dev/breeze/src/airflow_breeze/pre_commit_ids.py b/dev/breeze/src/airflow_breeze/pre_commit_ids.py index 236525e31481f..ce33a62ed739f 100644 --- a/dev/breeze/src/airflow_breeze/pre_commit_ids.py +++ b/dev/breeze/src/airflow_breeze/pre_commit_ids.py @@ -67,6 +67,7 @@ "check-start-date-not-used-in-defaults", "check-system-tests-present", "check-system-tests-tocs", + "check-tests-unittest-testcase", "check-urlparse-usage-in-code", "check-xml", "codespell", diff --git a/images/breeze/output-commands-hash.txt b/images/breeze/output-commands-hash.txt index 4e9f491898968..5c8e1e195c565 100644 --- a/images/breeze/output-commands-hash.txt +++ b/images/breeze/output-commands-hash.txt @@ -55,7 +55,7 @@ setup:version:123b462a421884dc2320ffc5e54b2478 setup:56a2ef337c354362760d247df5d05365 shell:ab07ac2d57253e25367a7200ce686703 start-airflow:5e8460ac38f8e9ea2a0ac7e248fd7bc9 -static-checks:543f0c776d0f198e80a0f75058445bb2 +static-checks:806eafbc99a76ebba6178b0b461b3499 stop:e5aa686b4e53707ced4039d8414d5cd6 testing:docker-compose-tests:b86c044b24138af0659a05ed6331576c testing:helm-tests:936cf28fd84ce4ff5113795fdae9624b diff --git a/images/breeze/output_static-checks.svg b/images/breeze/output_static-checks.svg index 04b6730555456..f9dd25c95aa60 100644 --- a/images/breeze/output_static-checks.svg +++ b/images/breeze/output_static-checks.svg @@ -35,8 +35,8 @@ .breeze-static-checks-r1 { fill: #c5c8c6;font-weight: bold } .breeze-static-checks-r2 { fill: #c5c8c6 } .breeze-static-checks-r3 { fill: #d0b344;font-weight: bold } -.breeze-static-checks-r4 { fill: #68a0b3;font-weight: bold } -.breeze-static-checks-r5 { fill: #868887 } +.breeze-static-checks-r4 { fill: #868887 } +.breeze-static-checks-r5 { fill: #68a0b3;font-weight: bold } .breeze-static-checks-r6 { fill: #98a84b;font-weight: bold } .breeze-static-checks-r7 { fill: #8d7b39 } @@ -223,61 +223,61 @@ -Usage: breeze static-checks [OPTIONS] [PRECOMMIT_ARGS]... +Usage: breeze static-checks [OPTIONS] [PRECOMMIT_ARGS]... Run static checks. -╭─ Pre-commit flags ───────────────────────────────────────────────────────────────────────────────────────────────────╮ -│--type-tType(s) of the static checks to run.                                                    │ -│(all | black | blacken-docs | check-airflow-config-yaml-consistent |                    │ -│check-airflow-provider-compatibility | check-apache-license-rat |                       │ -│check-base-operator-partial-arguments | check-base-operator-usage |                     │ -│check-boring-cyborg-configuration | check-breeze-top-dependencies-limited |             │ -│check-builtin-literals | check-changelog-has-no-duplicates |                            │ -│check-core-deprecation-classes | check-daysago-import-from-utils |                      │ -│check-decorated-operator-implements-custom-name | check-docstring-param-types |         │ -│check-example-dags-urls | check-executables-have-shebangs |                             │ -│check-extra-packages-references | check-extras-order | check-for-inclusive-language |   │ -│check-hooks-apply | check-incorrect-use-of-LoggingMixin | check-init-decorator-arguments│ -│| check-lazy-logging | check-links-to-example-dags-do-not-use-hardcoded-versions |      │ -│check-merge-conflict | check-newsfragments-are-valid |                                  │ -│check-no-providers-in-core-examples | check-no-relative-imports |                       │ -│check-only-new-session-with-provide-session |                                           │ -│check-persist-credentials-disabled-in-github-workflows |                                │ -│check-pre-commit-information-consistent | check-provide-create-sessions-imports |       │ -│check-provider-yaml-valid | check-providers-init-file-missing |                         │ -│check-providers-subpackages-init-file-exist | check-pydevd-left-in-code |               │ -│check-revision-heads-map | check-safe-filter-usage-in-html | check-setup-order |        │ -│check-start-date-not-used-in-defaults | check-system-tests-present |                    │ -│check-system-tests-tocs | check-urlparse-usage-in-code | check-xml | codespell |        │ -│compile-www-assets | compile-www-assets-dev | create-missing-init-py-files-tests |      │ -│debug-statements | detect-private-key | doctoc | end-of-file-fixer | fix-encoding-pragma│ -│| flynt | identity | insert-license | lint-chart-schema | lint-css | lint-dockerfile |  │ -│lint-helm-chart | lint-json-schema | lint-markdown | lint-openapi | mixed-line-ending | │ -│mypy-core | mypy-dev | mypy-docs | mypy-providers | pretty-format-json |                │ -│python-no-log-warn | replace-bad-characters | rst-backticks | ruff | shellcheck |       │ -│trailing-whitespace | ts-compile-format-lint-www | update-black-version |               │ -│update-breeze-cmd-output | update-breeze-readme-config-hash |                           │ -│update-common-sql-api-stubs | update-er-diagram | update-extras |                       │ -│update-in-the-wild-to-be-sorted | update-inlined-dockerfile-scripts |                   │ -│update-installed-providers-to-be-sorted | update-local-yml-file |                       │ -│update-migration-references | update-providers-dependencies |                           │ -│update-spelling-wordlist-to-be-sorted | update-supported-versions |                     │ -│update-vendored-in-k8s-json-schema | update-version | yamllint)                         │ -│--file-fList of files to run the checks on.(PATH)│ -│--all-files-aRun checks on all files.│ -│--show-diff-on-failure-sShow diff for files modified by the checks.│ -│--last-commit-cRun checks for all files in last commit. Mutually exclusive with --commit-ref.│ -│--commit-ref-rRun checks for this commit reference only (can be any git commit-ish reference).        │ -│Mutually exclusive with --last-commit.                                                  │ -│(TEXT)                                                                                  │ -│--github-repository-gGitHub repository used to pull, push run images.(TEXT)[default: apache/airflow]│ -╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ -╭─ Common options ─────────────────────────────────────────────────────────────────────────────────────────────────────╮ -│--verbose-vPrint verbose information about performed steps.│ -│--dry-run-DIf dry-run is set, commands are only printed, not executed.│ -│--help-hShow this message and exit.│ -╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ +╭─ Pre-commit flags ───────────────────────────────────────────────────────────────────────────────────────────────────╮ +│--type-tType(s) of the static checks to run.                                                    │ +│(all | black | blacken-docs | check-airflow-config-yaml-consistent |                    │ +│check-airflow-provider-compatibility | check-apache-license-rat |                       │ +│check-base-operator-partial-arguments | check-base-operator-usage |                     │ +│check-boring-cyborg-configuration | check-breeze-top-dependencies-limited |             │ +│check-builtin-literals | check-changelog-has-no-duplicates |                            │ +│check-core-deprecation-classes | check-daysago-import-from-utils |                      │ +│check-decorated-operator-implements-custom-name | check-docstring-param-types |         │ +│check-example-dags-urls | check-executables-have-shebangs |                             │ +│check-extra-packages-references | check-extras-order | check-for-inclusive-language |   │ +│check-hooks-apply | check-incorrect-use-of-LoggingMixin | check-init-decorator-arguments│ +│| check-lazy-logging | check-links-to-example-dags-do-not-use-hardcoded-versions |      │ +│check-merge-conflict | check-newsfragments-are-valid |                                  │ +│check-no-providers-in-core-examples | check-no-relative-imports |                       │ +│check-only-new-session-with-provide-session |                                           │ +│check-persist-credentials-disabled-in-github-workflows |                                │ +│check-pre-commit-information-consistent | check-provide-create-sessions-imports |       │ +│check-provider-yaml-valid | check-providers-init-file-missing |                         │ +│check-providers-subpackages-init-file-exist | check-pydevd-left-in-code |               │ +│check-revision-heads-map | check-safe-filter-usage-in-html | check-setup-order |        │ +│check-start-date-not-used-in-defaults | check-system-tests-present |                    │ +│check-system-tests-tocs | check-tests-unittest-testcase | check-urlparse-usage-in-code |│ +│check-xml | codespell | compile-www-assets | compile-www-assets-dev |                   │ +│create-missing-init-py-files-tests | debug-statements | detect-private-key | doctoc |   │ +│end-of-file-fixer | fix-encoding-pragma | flynt | identity | insert-license |           │ +│lint-chart-schema | lint-css | lint-dockerfile | lint-helm-chart | lint-json-schema |   │ +│lint-markdown | lint-openapi | mixed-line-ending | mypy-core | mypy-dev | mypy-docs |   │ +│mypy-providers | pretty-format-json | python-no-log-warn | replace-bad-characters |     │ +│rst-backticks | ruff | shellcheck | trailing-whitespace | ts-compile-format-lint-www |  │ +│update-black-version | update-breeze-cmd-output | update-breeze-readme-config-hash |    │ +│update-common-sql-api-stubs | update-er-diagram | update-extras |                       │ +│update-in-the-wild-to-be-sorted | update-inlined-dockerfile-scripts |                   │ +│update-installed-providers-to-be-sorted | update-local-yml-file |                       │ +│update-migration-references | update-providers-dependencies |                           │ +│update-spelling-wordlist-to-be-sorted | update-supported-versions |                     │ +│update-vendored-in-k8s-json-schema | update-version | yamllint)                         │ +│--file-fList of files to run the checks on.(PATH)│ +│--all-files-aRun checks on all files.│ +│--show-diff-on-failure-sShow diff for files modified by the checks.│ +│--last-commit-cRun checks for all files in last commit. Mutually exclusive with --commit-ref.│ +│--commit-ref-rRun checks for this commit reference only (can be any git commit-ish reference).        │ +│Mutually exclusive with --last-commit.                                                  │ +│(TEXT)                                                                                  │ +│--github-repository-gGitHub repository used to pull, push run images.(TEXT)[default: apache/airflow]│ +╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ +╭─ Common options ─────────────────────────────────────────────────────────────────────────────────────────────────────╮ +│--verbose-vPrint verbose information about performed steps.│ +│--dry-run-DIf dry-run is set, commands are only printed, not executed.│ +│--help-hShow this message and exit.│ +╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ From e30601da341d27904153b86eda3907f34358c0cf Mon Sep 17 00:00:00 2001 From: Vincent Beck Date: Thu, 20 Apr 2023 12:02:59 -0600 Subject: [PATCH 4/9] Fix documentation --- STATIC_CODE_CHECKS.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/STATIC_CODE_CHECKS.rst b/STATIC_CODE_CHECKS.rst index a8f28cac79663..af8b68e6d5694 100644 --- a/STATIC_CODE_CHECKS.rst +++ b/STATIC_CODE_CHECKS.rst @@ -227,7 +227,7 @@ require Breeze Docker image to be build locally. +-----------------------------------------------------------+------------------------------------------------------------------+---------+ | check-system-tests-tocs | Check that system tests is properly added | | +-----------------------------------------------------------+------------------------------------------------------------------+---------+ -| check-tests-unittest-testcase | Check that unit tests do not inherit from unittest.TestCase | * | +| check-tests-unittest-testcase | Check that unit tests do not inherit from unittest.TestCase | | +-----------------------------------------------------------+------------------------------------------------------------------+---------+ | check-urlparse-usage-in-code | Don't use urlparse in code | | +-----------------------------------------------------------+------------------------------------------------------------------+---------+ From d7bf634b26d491eb228467e7e8a0fd35d74af1df Mon Sep 17 00:00:00 2001 From: Vincent Beck Date: Fri, 21 Apr 2023 09:48:08 -0600 Subject: [PATCH 5/9] Revert "Merge local pre-commit hooks in one list" This reverts commit ce561b6bc4bfebeb93785b3e9542a8f221388974. --- .pre-commit-config.yaml | 56 +++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 155984f9b2b25..1d472e4cfd50a 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -153,6 +153,35 @@ repos: name: Run black (Python formatter) args: [--config=./pyproject.toml] exclude: ^.*/.*_vendor/|^airflow/contrib/ + - repo: local + hooks: + - id: update-common-sql-api-stubs + name: Check and update common.sql API stubs + entry: ./scripts/ci/pre_commit/pre_commit_update_common_sql_api_stubs.py + language: python + files: ^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.pyi?$ + additional_dependencies: ['rich>=12.4.4', 'mypy==1.2.0', 'black==22.12.0', 'jinja2'] + pass_filenames: false + require_serial: true + - id: update-black-version + name: Update black versions everywhere + entry: ./scripts/ci/pre_commit/pre_commit_update_black_version.py + language: python + files: ^.pre-commit-config.yaml$ + additional_dependencies: ['pyyaml'] + pass_filenames: false + require_serial: true + - id: ruff + name: ruff + language: python + require_serial: true + pass_filenames: true + # Since ruff makes use of multiple cores we _purposefully_ don't run this in docker so it can use the + # host CPU to it's fullest + entry: ruff --fix --no-update-check --force-exclude + additional_dependencies: ['ruff==0.0.226'] + files: \.pyi?$ + exclude: ^.*/.*_vendor/|^tests/dags/test_imports.py - repo: https://github.com/asottile/blacken-docs rev: 1.13.0 hooks: @@ -256,33 +285,6 @@ repos: - --exclude-file=.codespellignorelines - repo: local hooks: - - id: update-common-sql-api-stubs - name: Check and update common.sql API stubs - entry: ./scripts/ci/pre_commit/pre_commit_update_common_sql_api_stubs.py - language: python - files: ^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.pyi?$ - additional_dependencies: [ 'rich>=12.4.4', 'mypy==1.2.0', 'black==22.12.0', 'jinja2' ] - pass_filenames: false - require_serial: true - - id: update-black-version - name: Update black versions everywhere - entry: ./scripts/ci/pre_commit/pre_commit_update_black_version.py - language: python - files: ^.pre-commit-config.yaml$ - additional_dependencies: [ 'pyyaml' ] - pass_filenames: false - require_serial: true - - id: ruff - name: ruff - language: python - require_serial: true - pass_filenames: true - # Since ruff makes use of multiple cores we _purposefully_ don't run this in docker so it can use the - # host CPU to it's fullest - entry: ruff --fix --no-update-check --force-exclude - additional_dependencies: [ 'ruff==0.0.226' ] - files: \.pyi?$ - exclude: ^.*/.*_vendor/|^tests/dags/test_imports.py - id: replace-bad-characters name: Replace bad characters entry: ./scripts/ci/pre_commit/pre_commit_replace_bad_characters.py From 661a5d9d01aa1620236d3a95d5434e1f6eeeb97e Mon Sep 17 00:00:00 2001 From: Vincent Beck Date: Fri, 21 Apr 2023 10:26:57 -0600 Subject: [PATCH 6/9] Move pre-commit hook to appropriate location --- .pre-commit-config.yaml | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 1d472e4cfd50a..21175c85a4706 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -891,6 +891,12 @@ repos: entry: ./scripts/ci/pre_commit/pre_commit_www_lint.py additional_dependencies: ['yarn@1.22.19'] pass_filenames: false + - id: check-tests-unittest-testcase + name: Check that unit tests do not inherit from unittest.TestCase + entry: ./scripts/ci/pre_commit/pre_commit_unittest_testcase.py + language: python + pass_filenames: true + files: ^tests/.*\.py$ ## ADD MOST PRE-COMMITS ABOVE THAT LINE # The below pre-commits are those requiring CI image to be built - id: mypy-dev @@ -945,11 +951,4 @@ repos: pass_filenames: false files: ^airflow/migrations/versions/.*\.py$|^docs/apache-airflow/migrations-ref\.rst$ additional_dependencies: ['rich>=12.4.4'] - - id: check-tests-unittest-testcase - name: Check that unit tests do not inherit from unittest.TestCase - entry: ./scripts/ci/pre_commit/pre_commit_unittest_testcase.py - language: python - pass_filenames: true - files: ^tests/.*\.py$ - additional_dependencies: [ 'rich>=12.4.4' ] ## ONLY ADD PRE-COMMITS HERE THAT REQUIRE CI IMAGE From 4847fd31c81df3ed91142c05ff5d994570fc3f0e Mon Sep 17 00:00:00 2001 From: Vincent Beck Date: Fri, 21 Apr 2023 14:48:52 -0600 Subject: [PATCH 7/9] Use ast --- .../pre_commit_unittest_testcase.py | 46 +++++++------------ 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/scripts/ci/pre_commit/pre_commit_unittest_testcase.py b/scripts/ci/pre_commit/pre_commit_unittest_testcase.py index 951681f76c178..85f9966664e20 100755 --- a/scripts/ci/pre_commit/pre_commit_unittest_testcase.py +++ b/scripts/ci/pre_commit/pre_commit_unittest_testcase.py @@ -17,43 +17,31 @@ # under the License. from __future__ import annotations -import re +import ast +import pathlib import sys -from pathlib import Path -from rich.console import Console -if __name__ not in ("__main__", "__mp_main__"): - raise SystemExit( - "This file is intended to be executed as an executable program. You cannot use it as a module." - f"To run this script, run the ./{__file__} command [FILE] ..." - ) +def check_test_file(file: str) -> int: + node = ast.parse(pathlib.Path(file).read_text("utf-8"), file) + found = 0 + classes = [c for c in node.body if isinstance(c, ast.ClassDef)] + for c in classes: + # Some classes are returned as an ast.Attribute, some as an ast.Name object. Not quite sur why + parent_classes = [base.attr for base in c.bases if isinstance(base, ast.Attribute)] + parent_classes.extend([base.id for base in c.bases if isinstance(base, ast.Name)]) -console = Console(color_system="standard", width=200) + if "TestCase" in parent_classes: + found += 1 + print(f"The class {c.name} inherits from TestCase, please use pytest instead") + return found -def _check_file(file: Path) -> list: - content = file.read_text() - return re.findall(r"class[^(]+\(unittest.TestCase\)\:", content) - -def _join_with_newline(list_): - return "\n".join(list_) +def main(*args: str) -> int: + return sum([check_test_file(file) for file in args[1:]]) if __name__ == "__main__": - error_list = [] - for file in sys.argv[1:]: - matches = _check_file(Path(file)) - if matches: - error_list.append((file, matches)) - if error_list: - error_message = "\n".join([f"{f}: \n{_join_with_newline(m)}" for f, m in error_list]) - console.print( - f""" -[red]Found tests inheriting from unittest.TestCase. Please use pytest instead.[/] -{error_message} -""" - ) - sys.exit(1) + sys.exit(main(*sys.argv)) From 7b26da8ef6965cfdc0ac8cac99f6bca759d88133 Mon Sep 17 00:00:00 2001 From: Vincent Beck Date: Fri, 21 Apr 2023 14:54:37 -0600 Subject: [PATCH 8/9] Fix typo --- scripts/ci/pre_commit/pre_commit_unittest_testcase.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ci/pre_commit/pre_commit_unittest_testcase.py b/scripts/ci/pre_commit/pre_commit_unittest_testcase.py index 85f9966664e20..c13c9a99258e2 100755 --- a/scripts/ci/pre_commit/pre_commit_unittest_testcase.py +++ b/scripts/ci/pre_commit/pre_commit_unittest_testcase.py @@ -28,7 +28,7 @@ def check_test_file(file: str) -> int: found = 0 classes = [c for c in node.body if isinstance(c, ast.ClassDef)] for c in classes: - # Some classes are returned as an ast.Attribute, some as an ast.Name object. Not quite sur why + # Some classes are returned as an ast.Attribute, some as an ast.Name object. Not quite sure why parent_classes = [base.attr for base in c.bases if isinstance(base, ast.Attribute)] parent_classes.extend([base.id for base in c.bases if isinstance(base, ast.Name)]) From abf24ca77e8f258bc2839b4a7a1d94934d9791a1 Mon Sep 17 00:00:00 2001 From: Vincent Beck Date: Mon, 1 May 2023 11:21:58 -0400 Subject: [PATCH 9/9] Regenerate images --- images/breeze/output-commands-hash.txt | 6 ++-- ...utput_setup_check-all-params-in-groups.svg | 6 ++-- ...output_setup_regenerate-command-images.svg | 32 ++++++++----------- 3 files changed, 20 insertions(+), 24 deletions(-) diff --git a/images/breeze/output-commands-hash.txt b/images/breeze/output-commands-hash.txt index ef30a84c32281..44538a16b61b4 100644 --- a/images/breeze/output-commands-hash.txt +++ b/images/breeze/output-commands-hash.txt @@ -47,12 +47,12 @@ release-management:start-release:acb384d86e02ff5fde1bf971897be17c release-management:verify-provider-packages:566c60fb1bfdc5ed7c4be590736891b2 release-management:a6d6b27d8705294bd55cd1281b05f4c8 setup:autocomplete:03343478bf1d0cf9c101d454cdb63b68 -setup:check-all-params-in-groups:81afea3f465f3a78ebb23d7e58b0df94 +setup:check-all-params-in-groups:c3aca085350fc09451a6d502be9ee821 setup:config:3ffcd35dd24b486ddf1d08b797e3d017 -setup:regenerate-command-images:710a63ee7e8643e51a297e4c718009e8 +setup:regenerate-command-images:aaf263095a037d2271640513d8c156fe setup:self-upgrade:d02f70c7a230eae3463ceec2056b63fa setup:version:123b462a421884dc2320ffc5e54b2478 -setup:d2d911732098038cd6770dd47a151c0a +setup:26f37743534e14f5aad5300aad920301 shell:bd3e004a92ebcec8feb40fc5cd95872d start-airflow:ee5066f1420a489864b48bc4e5e472da static-checks:806eafbc99a76ebba6178b0b461b3499 diff --git a/images/breeze/output_setup_check-all-params-in-groups.svg b/images/breeze/output_setup_check-all-params-in-groups.svg index efed1c4b51dab..1ed51d446efa8 100644 --- a/images/breeze/output_setup_check-all-params-in-groups.svg +++ b/images/breeze/output_setup_check-all-params-in-groups.svg @@ -157,9 +157,9 @@ │release-management:release-prod-images | release-management:start-rc-process |                         │ │release-management:start-release | release-management:verify-provider-packages | release-management |  │ │setup:autocomplete | setup:check-all-params-in-groups | setup:config | setup:regenerate-command-images │ -│| setup:self-upgrade | setup:version | setup |  | shell | start-airflow | static-checks |  | shell |   │ -│start-airflow | static-checks |  | stop | testing:docker-compose-tests | testing:helm-tests |          │ -│testing:integration-tests | testing:tests | testing)                                                   │ +│| setup:self-upgrade | setup:version | setup | shell | start-airflow | static-checks | stop |          │ +│testing:docker-compose-tests | testing:helm-tests | testing:integration-tests | testing:tests |        │ +│testing)                                                                                               │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭─ Common options ─────────────────────────────────────────────────────────────────────────────────────────────────────╮ │--verbose-vPrint verbose information about performed steps.│ diff --git a/images/breeze/output_setup_regenerate-command-images.svg b/images/breeze/output_setup_regenerate-command-images.svg index 32d910ed0950b..5bb38f1c124b7 100644 --- a/images/breeze/output_setup_regenerate-command-images.svg +++ b/images/breeze/output_setup_regenerate-command-images.svg @@ -1,4 +1,4 @@ - + │release-management:release-prod-images | release-management:start-rc-process |                      │ │release-management:start-release | release-management:verify-provider-packages | release-management │ │| setup:autocomplete | setup:check-all-params-in-groups | setup:config |                            │ -│setup:regenerate-command-images | setup:self-upgrade | setup:version | setup |  | shell |           │ -│start-airflow | static-checks |  | shell | start-airflow | static-checks |  | stop |                │ -│testing:docker-compose-tests | testing:helm-tests | testing:integration-tests | testing:tests |     │ -│testing)                                                                                            │ -│--check-onlyOnly check if some images need to be regenerated. Return 0 if no need or 1 if needed. Cannot be used│ -│together with --command flag or --force.                                                            │ -╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ -╭─ Common options ─────────────────────────────────────────────────────────────────────────────────────────────────────╮ -│--verbose-vPrint verbose information about performed steps.│ -│--dry-run-DIf dry-run is set, commands are only printed, not executed.│ -│--help-hShow this message and exit.│ -╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ +│setup:regenerate-command-images | setup:self-upgrade | setup:version | setup | shell | start-airflow│ +│| static-checks | stop | testing:docker-compose-tests | testing:helm-tests |                        │ +│testing:integration-tests | testing:tests | testing)                                                │ +│--check-onlyOnly check if some images need to be regenerated. Return 0 if no need or 1 if needed. Cannot be used│ +│together with --command flag or --force.                                                            │ +╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ +╭─ Common options ─────────────────────────────────────────────────────────────────────────────────────────────────────╮ +│--verbose-vPrint verbose information about performed steps.│ +│--dry-run-DIf dry-run is set, commands are only printed, not executed.│ +│--help-hShow this message and exit.│ +╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯