Nice to hear that opinion Ash, it's an interesting angle. If someone works on a POC for that, would be interested to know how it spans out.
Thanks & Regards, Amogh Desai On Mon, Jul 28, 2025 at 7:23 PM Ash Berlin-Taylor <a...@apache.org> wrote: > 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 > >
