o-nikolas commented on PR #43280: URL: https://github.com/apache/airflow/pull/43280#issuecomment-2447764394
Hey folks, clearing up some confusion here: CC @potiuk @ashb @uranusjr 1) >> 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](https://github.com/apache/airflow/pull/42505#issuecomment-2407097770) 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](https://aws-mwaa.github.io/#/open-source/system-tests/dashboard.html) the green runs were from the short time my change was merged). 2) > 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. 3) > 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. 4) > 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
