Fix pytest plugin

[o-nikolas]

pytest plugins cannot be installed by a non-top-level conftest file. Now that provider tests are moved to a nested directory we need to plumb the plugin in a different way.

The plugin is now provided on the command line for breeze testing commands.

Also the config files which contain exceptions for forbidden_warnings were
set in pytest config in the conftest files. Since the plugin is now loaded at
the top-level and before conftest files are loaded. Move the config into the
python module for the plugin itself. There should be no harm in
configuring all exceptions for each test type. Each discovered test is just checked against the set of exceptions.

---

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

[potiuk]

Nice. I was wondering why we have warnings (yellow tests) back ... I think too much now happens in helm tests simply

[o-nikolas]

Nice. I was wondering why we have warnings (yellow tests) back ... I think too much now happens in helm tests simply

@potiuk Yes this is to fix what was broken by Ash's changes to the directory structure (described in this comment). Every one of our system tests are currently failing for this reason.

I think my fix should solve it, but for some reason all the Helm chart tests are failing as a side-effect and I have ZERO ideas why. I cannot see how they are related...

[o-nikolas]

Nice. I was wondering why we have warnings (yellow tests) back ... I think too much now happens in helm tests simply

@potiuk Yes this is to fix what was broken by Ash's changes to the directory structure (described in this comment). Every one of our system tests are currently failing for this reason.

I think my fix should solve it, but for some reason all the Helm chart tests are failing as a side-effect and I have ZERO ideas why. I cannot see how they are related...

Actually, when I run the tests locally on main it also fails. Which is a relief but also frustrating. How is main not more broken in the CI of all other PRs and Canary builds?

[potiuk]

Actually, when I run the tests locally on main it also fails. Which is a relief but also frustrating. How is main not more broken in the CI of all other PRs and Canary builds?

As discussed in slack - and for posterity. This is another teeting problem after #42505 and later #43173.

It is the result of accidentally triggering database initialization for xdist (parallel) tests - in your PR by plugin initialization, in main runnnig when BACKEND != none.

A solution to that would be to force BACKEND to "none" and _AIRLFOW_SKIP_DB_TESTS to "true" in order to avoid all the DB autouse fixtures. But this is quite tricky as plugins passed with -p wil be initialized before conftest.py that could be used for it, also conftest.py cannot be put at top-level apparently because pytest has problems when several top-level tests folders have conftest.py - it will refuse to load them.

We need to find a better solution - likely use different mechanism of Pytest to do imports - likely https://docs.pytest.org/en/7.1.x/explanation/pythonpath.html or maybe find some other creative ways of loading plugins from common place. Or maybe figure out a better way of sharing common test code.

[potiuk]

cc: @ashb -> maybe you will have some ideas?

[o-nikolas]

@potiuk
I got this one green finally and it's ready for review.

The approach I took was to simply not use the plugin for the helm chart tests. This is because it wasn't being used for them in the past I don't think. It was previously enabled in the tests of providers and core, but not helm, so I've excluded helm. Thoughts?

[potiuk]

The approach I took was to simply not use the plugin for the helm chart tests. This is because it wasn't being used for them in the past I don't think. It was previously enabled in the tests of providers and core, but not helm, so I've excluded helm. Thoughts?

Yes.

[potiuk]

Cool :)

[potiuk]

I think together with the uv fix today, it might be about the last "teething" issue I am aware of with moving providers out of "airlfow" (cc: @ashb @kaxil )

[o-nikolas]

I think together with the uv fix today, it might be about the last "teething" issue I am aware of with moving providers out of "airlfow" (cc: @ashb @kaxil )

Yeah, let's hope!
I wouldn't be surprised if the plugin raised it's head again at least once 😬 It really was proliferated throughout a lot of the test logic we have.

[potiuk]

I wouldn't be surprised if the plugin raised it's head again at least once 😬 It really was proliferated throughout a lot of the test logic we have.

Well. yeah, I might do some cleanups as well when attempting to split providers even further - also I think looking at #43488 might uncover some other dead bodies we keep in the closet.

[ashb]

pytest plugins cannot be installed by a non-top-level conftest file. Now that provider tests are moved to a nested directory we need to plumb the plugin in a different way.

Yes, but when was this actually a problem?

[potiuk]

Yes, but when was this actually a problem?

The problem was that the plugin did not work - really after the move . It did not catch the "forbidden warnings" in both provider and non-provider tests. That resulted in our tests becoming "yellow" in CI instead of "green" as they used to be before the move - the idea with "forbidden_warnings.py" was that we keep on removing them and go down to 0 eventually but no new warnings should be added.

So this one fixes the setup that no new warnings should be added when tests are running.

[uranusjr]

This breaks running Pytest in a Breeze shell. I’ll try to find a solution for it. In the mean time, add -p tests_common.pytest_plugin manually to your call.

[potiuk]

This breaks running Pytest in a Breeze shell. I’ll try to find a solution for it. In the mean time, add -p tests_common.pytest_plugin manually to your call.

Let me take a look as well

[ashb]

Yeah - precisely -- the plugin not capturing warnings is very different from changing the plugin to be automatically loaded from conftest.

pytest plugins cannot be installed by a non-top-level conftest file

The plugin was only installed in a top level conftest file.

I would like to revert this please if we can't find a way to make it be picked up no matter how people run pytest (breeze, directly, or from within IDEs) -- we shouldn't need to make everyone manually specify.

[potiuk]

Yes. Reverting it now. This something we have to handle differently.

[potiuk]

Yeah - precisely -- the plugin not capturing warnings is very different from changing the plugin to be automatically loaded from conftest.

It's related I think, it's the order in which the plugins are loaded and warning capturing was done. But yes - it should be solved differently.

[ashb]

I'm stepping through this (old/pre merge) version and it is correctly loading the exclusion list from the providers file. Looking at why the warnings filter isn't working though

[potiuk]

I don't know exactlly bt I think that was also realated to the order in which warning captuiring was done.

[o-nikolas]

Hey folks, clearing up some confusion here:

CC @potiuk @ashb @uranusjr

pytest plugins cannot be installed by a non-top-level conftest file. Now that provider tests are moved to a nested directory we need to plumb the plugin in a different way.

Yes, but when was this actually a problem?

@ashb This was a problem as a consequence of the issue you described yourself here
This happens when we run a system test with breeze, which looks something like breeze testing tests providers/tests/system/amazon/aws/example_lambda.py --system amazon. After your changes this leads pytest to load a plugin from a non-toplevel location which is now disallowed by pytest and the command fails. All system test dashboards have been fully red since your merge (example the green runs were from the short time my change was merged).

Yeah - precisely -- the plugin not capturing warnings is very different from changing the plugin to be automatically loaded from conftest.

The changes related to warnings in this PR are only present because moving the plugin to cli flag (due to 1) ) makes a race condition where the input exclusion paths are not yet loaded (since they are configured in conftest files). So they are moved to just be colocated to the code itself. Which is honestly just much simpler.

This breaks running Pytest in a Breeze shell. I’ll try to find a solution for it. In the mean time, add -p tests_common.pytest_plugin manually to your call.

Unfortunately yes, the common way I run those is by using the command from breeze itself for the test type I'm testing with. But I can see how this would be not the same workflow for others. We need to figure out how to get that case covered.

I would like to revert this please if we can't find a way to make it be picked up no matter how people run pytest (breeze, directly, or from within IDEs) -- we shouldn't need to make everyone manually specify.

Fair enough, and agreed, but can we focus on our efforts on finding this new path? Because our system testing infra has been broken for over a week due to this and so far it has not gotten much attention.

[ashb]

If the problem is that breeze testing tests provider/tests/... triggers the conftest error then we should fix that command:

If there are positional args to that then we shouldn't include tests arg. Right now we get breeze testing tests provider/tests/... runs (give or take) pytest tests provider/tests/... when we want/need it to run pytesy provider/tests/...

Have I understood correctly @o-nikolas ?

[o-nikolas]

If the problem is that breeze testing tests provider/tests/... triggers the conftest error then we should fix that command:

This sounds lovely :) I'm not passionate about how the issue is fixed, just that it gets fixed.

If there are positional args to that then we shouldn't include tests arg. Right now we get breeze testing tests provider/tests/... runs (give or take) pytest tests provider/tests/... when we want/need it to run pytesy provider/tests/...

That is certainly a possible approach!

[potiuk]

Yes this is a problem reported today as well on slack https://apache-airflow.slack.com/archives/CCQ7EGB1P/p1730313232385169?thread_ts=1730313232.385169&cid=CCQ7EGB1P and now I see where it is coming from. Yep it's been broken since the provider's move and we should definitely fix it

[potiuk]

And when I recall that (coming back from a meetup now) - I believe this code must have been there before the move only it has been disabled or removed when the providers were moved . I will have to check when I am back.

[potiuk]

I thought it has not been needed BTW and iindownplayed importance of it but I see now that it has been indeed used for system tests so we must bring it back

[potiuk]

@ashb @o-nikolas

I was really scratching my head why this happen and could not figure it out, but I know what happens. I recalled some of the history, I checked out the version before the providers move and it turned out that the "tests" folder has been added there even then.

BUT (And here comes surprise from pytest) the "tests" were never collected when we added any provider's tests because of af a bug introduced in Pytest 8: pytest-dev/pytest#12605 (comment)

The thing that if you run: pytest tests tests/providers/something - the "tests" pytest will NOT be collected, because as of Pytest 8, if there is a parent folder before sub-path, ONLY the subpath tests are collected (!!!!).

It's a very weird pytest behaviour that had already bitten us and I had to workaround it in run_tests.py for parallell non-db tests:

airflow/dev/breeze/src/airflow_breeze/utils/run_tests.py

Line 401 in 51f9e83

# leave only folders, strip --pytest-args that exclude some folders with `-' prefix

So The "tests" folder had been there for quite some time (after we switched to Pytest 8) - but we never realized it, because system test providers were subfolder of the "tests" and when you run pytest tests tests/providers/amazon/sytem/some_test.py - pytest did not collect all the tests from "tests" folder - only used the some_test.py folder.

This is now pytest tests providers/amazon/tests/sytem/some_test.py - and in this case both "tests" and the system test are collected.

Weird as it is, but simply "tests" should have not been at all in this case, but it was never a problem, because all the tests were in "tests/" sub-folder and simply the first "tests" folder was ignored.

So now when I know how we get it, the fix will be easy. PR shortly.

[potiuk]

@ashb @o-nikolas -> #43529 fix here.

[potiuk]

Actually it is not yet final fix - due to the multiple conftest.py at top-level, and moving providers the "All" test types are not exactly working as expected - they only run "core" tests. But this is not an issue for CI at least - becasue there in most cases (except Quarantine tests" we are not using "All" test suite, but individual tests folders (also for Non-DB tests) - so it has no impact on running all tests. I will fix it as a follow up and likely change approach for "All-" test suites

[o-nikolas]

The thing that if you run: pytest tests tests/providers/something - the "tests" pytest will NOT be collected, because as of Pytest 8, if there is a parent folder before sub-path, ONLY the subpath tests are collected (!!!!).

I feel so much better now. I was also really scratching my head trying to figure out how this worked beforehand with passing a provider path, since I knew the tests was also there before. Thanks for solving that one!

[potiuk]

I feel so much better now. I was also really scratching my head trying to figure out how this worked beforehand with passing a provider path, since I knew the tests was also there before. Thanks for solving that one!

Yeah. It's a real surprise from pytest devs to have this behaviour.. but also it made me realise that we have far too many band-aids over band-aids for the test framework of our and now when we extract out provides and task_sdk we need to heavily refactor it as it is far from being easy to reason about - and if it took even me that long to find out where it comes from it means we have to simplify and split it all to 3 separate paths (and more) - see #42632 (comment)