If we do it it has a nice "feature" - we would not have to add "tests" folders at all to be added to PYTHONPATH (we do it in "setup_idea.py" now for IntelliJ and "setup_vscode.py' I think. But if each tests is a standalone "test script" that does not have to even know what "package" it is in - there would not be a need to add "tests" to PYTHONPATH at all.
J, On Mon, Jul 28, 2025 at 3:12 PM Jarek Potiuk <ja...@potiuk.com> wrote: > That is an interesting alternative and I quite like it. > > We tried to use importlib before and we failed (but we did not try "hard") > - Andrey tried it one day and it generated a lot of errors - but we've > changed a LOT since the (including separating of common test classes to > "devel_common". > > It might be worth doing a POC for that - this adds very nice isolation > where all tests are generally fully "standalone". I might attempt to make a > POC on that. > > I would not asy "only fixtures" is the right approach - but I think it > should generally apply to trying to import common code between "tests". > There is a bit of imports that are pretty "valid" in some way and easier to > use than fixtures I think - i.e. "devel-common" classes, but they are not > "tests" classes - they are regular "src" classes in "tests_common" package > in "apache-airflow-devel-common" distribution, so this is pretty good idea > to import them. > > But I do agree that importing one test from other classes is "fishy" and > if importlib might help us with this, then it is an easy way to enforce it. > I will try it first. That's a very good idea :) > > J. > > On Mon, Jul 28, 2025 at 10:24 AM Ash Berlin-Taylor <a...@apache.org> wrote: > >> Did you look at changing the pytest import option instead? >> https://docs.pytest.org/en/stable/explanation/pythonpath.html - and >> specifically the `importlib` option >> >> To quote their documentation: >> >> > importlib: this mode uses more fine control mechanisms provided by >> importlib < >> https://docs.python.org/3/library/importlib.html#module-importlib> to >> import test modules, without changing sys.path < >> https://docs.python.org/3/library/sys.html#sys.path>. >> > >> > Advantages of this mode: >> > >> > pytest will not change sys.path < >> https://docs.python.org/3/library/sys.html#sys.path> at all. >> > >> > Test module names do not need to be unique – pytest will generate a >> unique name automatically based on the rootdir. >> > >> > Disadvantages: >> > >> > Test modules can’t import each other. >> > >> > Testing utility modules in the tests directories (for example a >> tests.helpers module containing test-related functions/classes) are not >> importable. The recommendation in this case it to place testing utility >> modules together with the application/library code, for example >> app.testing.helpers. >> > >> > Important: by “test utility modules”, we mean functions/classes which >> are imported by other tests directly; this does not include fixtures, which >> should be placed in conftest.py files, along with the test modules, and are >> discovered automatically by pytest. >> > >> >> I think with pytest fixtures we almost shouldn’t need to import tests >> directly at all, so if they are a package or not should not be an issue. >> Doing this (or working towards) it means we don’t need to worry about the >> name at all, and we already have the `tests_common` etc as a place to put >> helpers. >> >> That way we don’t need any special rules or naming convention or >> otherwise “duplicate” folder names. >> >> I.e. instead of adding a name to make things unique, we forbid imports of >> helper functions and make them use `pytest` fixtures instead. >> >> Thoughts? >> >> > On 27 Jul 2025, at 17:55, Jens Scheffler <j_scheff...@gmx.de.INVALID> >> wrote: >> > >> > Hi Jarek, >> > >> > I like the general idea of the structure as you propose. >> > >> > I would not "swap" as you described afterwards but propose to use the >> first proposal you made. >> > >> > For docker and k8s tests I think it would be better to keep them >> outside of "airflow-core" as they have a rather integrative/system scope >> and are only indirectly related to core. For the moment I'd keep then and >> once we re-structure the packages still we can align later. >> > >> > Jens >> > >> > On 25.07.25 11:57, Amogh Desai wrote: >> >> Hi Jarek, >> >> >> >>> So maybe it's a good time to improve it and come up with a convention >> that >> >> we can apply also to shared folders? Such change will be very little >> >> disruptive and can be largely automated and applied in a single PR >> >> >> >> Yeah, I think it's a good time and idea to get such a thing done. >> >> >> >> *> from unit.amazon import s3_operator_test* >> >> I never paid close attention to such import patterns and I do not like >> >> them. Its very >> >> confusing and ambiguous, and it's very hard to tell "what package is >> this" >> >> to any of those >> >> imports. >> >> >> >> While writing the Task SDK integration tests I did keep a lot of those >> >> things in mind and if you >> >> see the internal imports I have, they are of the pattern: >> >> *from task_sdk_tests.constants import AIRFLOW_ROOT_PATH *which is far >> less >> >> ambiguous and >> >> confusing. >> >> >> >>> Note that this does not yet tackle the other "special" test types we >> have >> >> -> kubernetes-tests, docker-tests, task-sdk-tests (right now being >> merged >> >> by Amogh), especially the last one seem to be clashing with >> >> `task_sdk_tests`. But I think (and this would be separate discussion) - >> >> this also adds an opportunity (next step) to move those "special" test >> >> types we have. Eventually we could move those tests to be "yet another >> test >> >> type" under "tests": >> >> >> >> Agreed, we can consider moving those to the right places where >> applicable, >> >> like: >> >> *task_sdk_tests/* >> >> >> >> *├── unit/└── integration/ # Moved from task-sdk-tests/* >> >> >> >> We can start small here and build up to fix the issues one package at a >> >> time, and if >> >> we can come up with a good AI prompt, we can get some work multiplexed >> by >> >> the community >> >> too. >> >> >> >> >> >> Thanks & Regards, >> >> Amogh Desai >> >> >> >> >> >> On Thu, Jul 24, 2025 at 8:38 PM Jarek Potiuk <ja...@potiuk.com> wrote: >> >> >> >>> Hello here, >> >>> >> >>> Since we have now started even more isolation and separation of the >> code >> >>> and libraries, I think it might be a good time to maybe improve a bit >> the >> >>> way our tests "package" hierarchy is done - in providers and >> elsewhere. >> >>> >> >>> This question came up when i started to add pre-commit checks after >> our >> >>> "shared code" PR have been merged - in >> >>> https://github.com/apache/airflow/pull/53697 >> >>> >> >>> One problem with having tests in monorepo, is that if all "tests" >> folders >> >>> are added to PYTHONPATH, we cannot really put tests in the "top" >> level of >> >>> the tests folder (that is pretty natural whe you do not have multiple >> of >> >>> "tests" folders. There are two reasons for that: >> >>> >> >>> * it's likely module names or package names will have the same names >> in >> >>> separate distributions >> >>> * quite often - if we add tests in a sub-package of tests directly >> they >> >>> might clash with the same names - for example from stdlib or another >> >>> package we installed. Good examples of it (and something that >> happened in >> >>> the past were `tests/smtp/test_*` and `tests/kubernetes/test_*`. >> Depending >> >>> on how your IDE and env was configured you could have ended up in >> "import >> >>> kubernetes" not behaving as you expected. >> >>> >> >>> So far in providers we have the >> "{unit/system/integration}/<PROVIDER>/" >> >>> convention that solved the issue. But even me as author of it, I must >> admit >> >>> `from unit.amazon` when you want to cross-import between different >> amazon >> >>> or different providers looks rather stupid and baffling. Even >> recently Ash >> >>> commented in one PR "that must be wrong" - and yeah, it does look >> wrong.... >> >>> >> >>> So maybe it's a good time to improve it and come up with a convention >> that >> >>> we can apply also to shared folders? Such change will be very little >> >>> disruptive and can be largely automated and applied in a single PR - >> and >> >>> modern git and IDE integration will follow such changes nicely - so >> that >> >>> you can see history of changes and it resolves most of conflicts >> >>> automatically - so it could be done with very little disruption (only >> >>> currently opened PRs will have to be rebased). >> >>> >> >>> My proposal would be to add a "parent" folder in the "tests" >> directory to >> >>> indicate where the test is coming from. That might sound like a >> >>> duplication, but I think it's a natural consequence of having code in >> >>> monorepo, and a nice way to improve the organisation of tests. Also >> it has >> >>> an immediate notion or where the imports come from. >> >>> >> >>> airflow-core\ >> >>> src\ >> >>> airflow\ >> >>> tests\ >> >>> airflow_core_tests\ >> >>> unit\ >> >>> system\ >> >>> integration\ >> >>> airflow-task-sdk\ >> >>> src\ >> >>> airflow\ >> >>> sdk >> >>> tests\ >> >>> task_sdk_tests\ >> >>> # here all tests are unit so no >> need >> >>> for sub-packages >> >>> >> >>> providers/amazon/tests\ >> >>> provider_tests\ >> >>> unit\ >> >>> amazon >> >>> integration\ >> >>> amazon >> >>> >> >>> >> >>> providers/cncf/kubernetes/tests\ >> >>> provider_tests\ >> >>> unit\ >> >>> cncf\kubernetes\ >> >>> integration\ >> >>> >> cncf\kubernetes\ >> >>> >> >>> >> >>> I also considered swapping "unit" / "provider" >> (provider_tests\amazon\unit) >> >>> - but that would make it a bit more complex when we choose type of >> tests to >> >>> run or examples to have and I think it's somewhat better to have the >> >>> unit/system/integration distinction right after provider_tests because >> >>> essentially those test types are VERY different. >> >>> >> >>> Again - that sounds like a lot of duplication in the path, but I >> think it's >> >>> worth it (and our pre-commits already make sure that there are no >> typos and >> >>> keep it in order and they can be updated to keep this new structure). >> >>> >> >>> The main benefit of it hat when you import (wherever) between >> different >> >>> distributions - the imports will **look** better and be more obvious: >> >>> >> >>> from provider_tests.unit.amazon import some_amazon_test_code >> >>> from task_sdk_tests import apis >> >>> >> >>> Note that this does not yet tackle the other "special" test types we >> have >> >>> -> kubernetes-tests, docker-tests, task-sdk-tests (right now being >> merged >> >>> by Amogh), especially the last one seem to be clashing with >> >>> `task_sdk_tests`. But I think (and this would be separate discussion) >> - >> >>> this also adds an opportunity (next step) to move those "special" test >> >>> types we have. Eventually we could move those tests to be "yet >> another test >> >>> type" under "tests": >> >>> >> >>> airflow_core_tests\ >> >>> unit >> >>> system >> >>> integration >> >>> docker >> >>> kubernetes >> >>> >> >>> task_sdk_tests\ >> >>> unit >> >>> integration >> >>> >> >>> But I would approach it as the next step after we reorganize imports >> for >> >>> the "regular" tests we have. >> >>> >> >>> WDYT? Good idea? Other proposals? >> >>> >> >>> J. >> >>> >> > >> > --------------------------------------------------------------------- >> > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org >> > For additional commands, e-mail: dev-h...@airflow.apache.org >> > >> >>
