Add pre-commit hook to ban usage of unittest.TestCase in unit tests - #30775
Conversation
|
|
||
| def _check_file(file: Path) -> list: | ||
| content = file.read_text() | ||
| return re.findall(r"class[^(]+\(unittest.TestCase\)\:", content) |
There was a problem hiding this comment.
NIT: I am fine with using regexp, but as the old Chinese proverb says "you have problem - introduce regexp, now you have two problems".
Why don't we use AST for that ? We have a couple of other pre-commits that use AST parsing of Python files, and while esoteric a bit for people, it's not as difficult as it might seem. We have some examples there that migh help with getting the right parser.
Ths has the nice benefit that it could also get the error message including source and line code (which are present in the AST tree I believe) and that it will always work even if you have long class name which will lead your class definition to be broken across mulitple lines.
There was a problem hiding this comment.
I really dont like using Regex for this as well. I did not know this AST, I'll take a look :) Thanks!
There was a problem hiding this comment.
Updated. I confess the code is WAY cleaner now. However, I am not a pro in AST (as a matter of fact, it is the first time I use it). So feel free to challenge my code
There was a problem hiding this comment.
These files come from the merge from main. They should not be part of this PR
There was a problem hiding this comment.
You need to rebae and regenerate the "help" images- there is a help with list of pre-commits that got updated in the meantime and you have conflict.
|
static checks fails |
| 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 |
There was a problem hiding this comment.
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())"]
| for c in classes: | ||
| # 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)]) |
There was a problem hiding this comment.
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" :)
There was a problem hiding this comment.
cc: @uranusjr -> you have more AST experience so maybe you can have some educational comments here as well.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah. Happy to merge once static checks fixed.
potiuk
left a comment
There was a problem hiding this comment.
Pending static checks/regeneration of the images.
|
All green :) |
|
Side comment: It's funny how new pre-commit check is held back by other pre-commits :) . But for a good reason, and while it might be frustrating at times, the amount of frustration those pre-commit save everyone else, is totally worth it. |
Addresses second item from phase 2 of #29305
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.