Looks like this was discussed, decided, and done over the weekend, but just wanted to say thanks for that. The redundancy in he import paths has been bugging me LOL
I'm glad you landed on the unit/integration/system naming convention, I like that but I'm curious what would go in the integration path. Before I started with Airflow, I had been taught that integration test are what we call system tests. I've come to use the two terms more or less interchangeably. What I am familiar without outside of Airflow is unit tests are within a module and integration tests are between modules. Under that definition, most (all?) of what we currently call system tests are integration tests. Maybe it's an "all squares are rectangles but not all rectangles are squares" type definition and I'm missing some distinction between the terms. I guess some of what we have in the existing unit tests would be more accurately integ tests, so maybe that's what you have in mind. Anyway, thanks for another major cleanup task! - ferruzzi ________________________________ From: Jarek Potiuk <ja...@potiuk.com> Sent: Sunday, February 16, 2025 3:48 AM To: dev@airflow.apache.org Subject: RE: [EXT] [DISCUSS] Tests structure for providers CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. AVERTISSEMENT: Ce courrier électronique provient d’un expéditeur externe. Ne cliquez sur aucun lien et n’ouvrez aucune pièce jointe si vous ne pouvez pas confirmer l’identité de l’expéditeur et si vous n’êtes pas certain que le contenu ne présente aucun risque. And yes Ash -> ruff rule it is to ban "providers" imports https://github.com/apache/airflow/pull/46801 (and it found two places where they were still used) On Sun, Feb 16, 2025 at 12:35 PM Jarek Potiuk <ja...@potiuk.com> wrote: > And merged! that was fast. Thanks Bugra, Ash, Jens ! > > On Sun, Feb 16, 2025 at 11:40 AM Jarek Potiuk <ja...@potiuk.com> wrote: > >> https://github.com/apache/airflow/pull/46800 -> there you go :) . Happy >> reviewing of 1570+ files changed. >> >> BTW. It's way easier to review it with `git diff` than with Github UI :) >> >> On Sun, Feb 16, 2025 at 11:32 AM Jarek Potiuk <ja...@potiuk.com> wrote: >> >>> Naaa. that one is super easy to implement and automate and it's better >>> to have one quick "surgical" cut to rename all of those :), I am about to >>> send PR with all of those changes in a moment. >>> >>> On Sun, Feb 16, 2025 at 11:26 AM Buğra Öztürk <ozturkbugr...@gmail.com> >>> wrote: >>> >>>> Amazing!:) Maybe we can divide and conquer this on structure changes and >>>> pre-commit rather than leaving you with all :) >>>> >>>> On Sun, 16 Feb 2025, 10:45 Jarek Potiuk, <ja...@potiuk.com> wrote: >>>> >>>> > Yeah "unit" seems to be the way to go :) . I will make another giant >>>> PR >>>> > (but this time it should be even easier to review) to convert to it :) >>>> > >>>> > On Sun, Feb 16, 2025 at 10:41 AM Buğra Öztürk < >>>> ozturkbugr...@gmail.com> >>>> > wrote: >>>> > >>>> > > Thanks for all the effort on this! It looks great! >>>> > > - The approach looks great! >>>> > > - Heavy plus one on this. The pre-commit should be one of the ground >>>> > > pillars of any standardisation. Having an extra file structure check >>>> > could >>>> > > be included too on top of `from` to eliminate new providers to >>>> follow the >>>> > > consistent structure. >>>> > > - I like the unit too over provider_tests. It is a great idea to >>>> > eliminate >>>> > > ambiguity. The other parts seem pretty pretty good. >>>> > > >>>> > > Along with the latest improvements, these will be great additions! >>>> > > >>>> > > >>>> > > On Sun, Feb 16, 2025 at 4:15 AM Rahul Vats <rah.sharm...@gmail.com> >>>> > wrote: >>>> > > >>>> > > > Thanks for the detailed explanation and for driving this effort—it >>>> > looks >>>> > > > like a solid improvement. >>>> > > > >>>> > > > 1. >>>> > > > >>>> > > > I love the approach & new structure. I can't think of a better >>>> > > approach. >>>> > > > 2. >>>> > > > >>>> > > > I agree that adding a pre-commit check to enforce consistent >>>> imports >>>> > > is >>>> > > > a good idea. It will help maintain the new structure and >>>> prevent >>>> > > > regressions. >>>> > > > 3. >>>> > > > >>>> > > > For the naming, I’d prefer using unit instead of >>>> provider_tests to >>>> > > keep >>>> > > > it more intuitive and consistent with system and integration. >>>> This >>>> > > makes >>>> > > > the structure self-explanatory and easier for new contributors. >>>> > > > >>>> > > > >>>> > > > Regards, >>>> > > > Rahul Vats >>>> > > > >>>> > > > On Sun, 16 Feb 2025 at 03:50, Jarek Potiuk <ja...@potiuk.com> >>>> wrote: >>>> > > > >>>> > > > > > 2) no real opinion, seems useful. Let’s see if we can do it >>>> with a >>>> > > ruff >>>> > > > > rule first? It _might_ be possible. (It is possible to extend >>>> the >>>> > ruff >>>> > > > > rules on a per subproject basis, check out >>>> task_sdk/pyproject.toml) >>>> > > > > >>>> > > > > I am not sure if we need ruff rule for it, as we have not yet >>>> enabled >>>> > > any >>>> > > > > `ruff` airflow rules in airflow development (it is targeted so >>>> far >>>> > for >>>> > > > > users writing DAGs} >>>> > > > > >>>> > > > > I think it's more "airflow internal" rule not something that >>>> can be >>>> > > used >>>> > > > > for general audience (which is pretty-much what ruff rules are >>>> about) >>>> > > > > >>>> > > > > Unfortunately - the current way ruff works does not allow local >>>> > > > > "plugin" functionality to add new rules that would only be >>>> applicable >>>> > > to >>>> > > > > the current project you are in. In the absence of that feature, >>>> > having >>>> > > a >>>> > > > > python pre-commit seem the next-best thing. >>>> > > > > >>>> > > > > J. >>>> > > > > >>>> > > > > >>>> > > > > On Sat, Feb 15, 2025 at 10:50 PM Ash Berlin-Taylor < >>>> a...@apache.org> >>>> > > > wrote: >>>> > > > > >>>> > > > > > 100%. It’s on my list to tackle very soon, it can’t get put >>>> off >>>> > much >>>> > > > > > longer. >>>> > > > > > >>>> > > > > > I have some thoughts (around back compat mostly) but they >>>> aren’t >>>> > > > > collected >>>> > > > > > yet, and I won’t derail this thread. >>>> > > > > > >>>> > > > > > > On 15 Feb 2025, at 21:43, Jarek Potiuk <ja...@potiuk.com> >>>> wrote: >>>> > > > > > > >>>> > > > > > > cc: @ashb @uranusjr @kaxil -> something for consideration >>>> in our >>>> > > > > > > discussions on how to repackage airlfow soon. I keep on >>>> > explaining >>>> > > > why >>>> > > > > > > running code in airflow.__init__.py is a bad idea and >>>> advocating >>>> > > for >>>> > > > > > > removal of it and replace it with explicit initialization, >>>> yet >>>> > that >>>> > > > > topic >>>> > > > > > > have not been discussed yet, but I will plan to start a >>>> > discussion >>>> > > > > about >>>> > > > > > it >>>> > > > > > > soon once we approach the packaging subject. I am not sure >>>> what's >>>> > > > your >>>> > > > > > > thinking is about this - I know you spent consirderable >>>> amount of >>>> > > > time >>>> > > > > on >>>> > > > > > > doing all the "lazy initalization" dance all over the >>>> places, >>>> > and I >>>> > > > > think >>>> > > > > > > it adds a lot of complexity to our code and only partially >>>> solves >>>> > > the >>>> > > > > > > cicular imports problem. But I know @ashb has very strong >>>> feeling >>>> > > > about >>>> > > > > > > being able to do "from airlfow import Dag" - which more or >>>> less >>>> > > > > requires >>>> > > > > > > all this complexity. I just don't think it's worth it. >>>> > > > > > >>>> > > > > > >>>> > > > > >>>> > > > >>>> > > >>>> > > >>>> > > -- >>>> > > Bugra Ozturk >>>> > > >>>> > >>>> >>>
