Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,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
Expand Down
2 changes: 2 additions & 0 deletions STATIC_CODE_CHECKS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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 | |
Expand Down
1 change: 1 addition & 0 deletions dev/breeze/src/airflow_breeze/pre_commit_ids.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion images/breeze/output-commands-hash.txt
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ setup:version:123b462a421884dc2320ffc5e54b2478
setup:26f37743534e14f5aad5300aad920301
shell:bd3e004a92ebcec8feb40fc5cd95872d
start-airflow:ee5066f1420a489864b48bc4e5e472da
static-checks:543f0c776d0f198e80a0f75058445bb2
static-checks:806eafbc99a76ebba6178b0b461b3499
stop:e5aa686b4e53707ced4039d8414d5cd6
testing:docker-compose-tests:b86c044b24138af0659a05ed6331576c
testing:helm-tests:936cf28fd84ce4ff5113795fdae9624b
Expand Down
28 changes: 14 additions & 14 deletions images/breeze/output_setup.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
52 changes: 26 additions & 26 deletions images/breeze/output_setup_check-all-params-in-groups.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
58 changes: 29 additions & 29 deletions images/breeze/output_setup_regenerate-command-images.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
108 changes: 54 additions & 54 deletions images/breeze/output_static-checks.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
47 changes: 47 additions & 0 deletions scripts/ci/pre_commit/pre_commit_unittest_testcase.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#!/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 ast
import pathlib
import sys


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 sure why

@potiuk potiuk Apr 22, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The Name is when you use class directly imported:

from airflow.providers.common.sql.hooks.sql import DbApiHook 
class DbApiHookInProvider(DbApiHook):

The Attr is when you use "module.name" expression - because then from the Python syntax point of view you are reading an attribute of a Name defined by import.

from airflow import plugins_manager
class FakePlugin(plugins_manager.AirflowPlugin):

Then what you derive from is effectivally an attribute of loaded 'plugins_manager' Name:

["Attribute(value=Name(id='plugins_manager', ctx=Load()), attr='AirflowPlugin', ctx=Load())"]

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)])

@potiuk potiuk Apr 22, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI. In general, this is not a complete solution to find out if your class derives from a TestCase or not. However possibly it is good-enough.

The cases that are not handled (potentially) with this code:

  • TestCase might be another TestCase not necessarily the unit TestCase - it could be "airflow.test_util.TestCase" - in order to find out if this is the real Test Case, we would have to walk the tree likely and find out where it has been imported from

  • The parent class could be transitively deriving from TestCase and this code would not have found it. So Test Case might be your grand-parent and this case is not handled by this code.

HOWEVER.

I think it's good-enough :) :

  • we can assume any 'TestCase` is bad - and keep it as convention (never use TestCase as your base test class name)
  • we can assume that even if we have "grand-parent" Test Case, then the parent of ours is also checked during the pre-commit and it will be the parent that will fail. This would not be a correct assumption if the parent class is outside of the "tests" directory, but let's asume it will never happen

Unless of course you would like to dig deeper and make it "perfect" :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cc: @uranusjr -> you have more AST experience so maybe you can have some educational comments here as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There’s really no reason at all to subclass from anything when you use Pytest anyway, so I think it’s good enough to just assume any TestCase is bad.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah. Happy to merge once static checks fixed.


if "TestCase" in parent_classes:
found += 1
print(f"The class {c.name} inherits from TestCase, please use pytest instead")

return found


def main(*args: str) -> int:
return sum([check_test_file(file) for file in args[1:]])


if __name__ == "__main__":
sys.exit(main(*sys.argv))