Yeah, I wasn’t sure if it would work for us. Let us know how it goes. > On 28 Jul 2025, at 14:12, 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 >>> >> >>
--------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org For additional commands, e-mail: dev-h...@airflow.apache.org
