potiuk commented on PR #27946: URL: https://github.com/apache/airflow/pull/27946#issuecomment-1328856772
> Is this really a "pre-commit" check or should this just be in the unit tests? > > (We seem to be adding more and more and more checks over time to pre-commit. Is there any rhyme or reason to what goes where? I think for me the rule of thumb I see is what we are testing. * The "unit tests" should test the "real" airflow functionality. (i..e whether things work as expected even if some of the things they use are mocked). * Static checks (i.e. pre-commits) on the other hand check if the "code fullfils certain criteria" (formatting, Type checking, using certain constructs, etc.). The tests above check if: a) the API has not changed accidentally b) whether the code is only using what is intended IMHO, if we need to run AST parsing for Airflow code, this is a clear indication we analyse the code of Airlfow not the functionality of Airflow. So they both seem to fit in the second camp. There are other criteria - such as how fast things are, whether they are feasible to be run in pre-commit - before each commit (if you have pre-commit installed), which also boils down to - whether we can run them super-selectively - only for changed files in some very selected part of the code and be pretty sure about the result - this is what pre-commit gives us. In this case - yes - we are running the tests for "common.sql" in the first case and for all providers in the other case - and it will give good results if we only change few files in providers and run the pre-commit - it should fail the same as we run it for all applicable files. So yeah - this one very firmly goes into pre-commits. Side commend. As discussed in the previous discussion with @vincbeck - we could add "regular" unit tests (or rather system tests following the AIP-47) to add similar checks on "whether it works" level https://github.com/apache/airflow/pull/27854#issuecomment-1325325982 - but this seems to be quite an impossible feat because we would have to run tests with literally every released version of the other providers. Another side - comment. There is another (upcoming) example which will go into unit tests and it would explain where the difference is. For AIP-44 I plan to run ALL APPLICABLE unit tests of our in a "db-less" backend - i.e. with only internal API server running - and making sure our code will not execute any SQLAlchemy/DB operation. - simply to make sure that Airlfow worker, dag file processor, triggerer are truly - DB-less. And in this case this falls precisely in the other camp - where we want to actually "run" the real airflow code and see that it behaves as expected. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
