Repository navigation
Separate non-db-tests (and others) to providers/non-providers/task_sdk #42632
Description
Currently non-db-tests execute all test type specified via --test-types in a single process using xdist (-n processors) parallelism in order to make efficient use of parallelisation of the tests. This means that both "core" and "providers" types of tests are run in a single pytest command. For example:
breeze testing non-db-tests \
--parallel-test-types "API Providers[google]"Where PARALLELL_TEST_TYPES are produced by the "selective check" to determine which tests should be run.
However the #42505 splits out providers to a separate directory and we cannot run providers and non-provider tests any longer in the same process, because pytest has a limitation where test module names should not overlap (and tests/conftest.py is different for providers and non-providers, so they cannot be run in the same process as it will result in
_pytest.pathlib.ImportPathMismatchError: ('tests.conftest', '/opt/airflow/tests/conftest.py', PosixPath('/opt/airflow/providers/tests/conftest.py'))
One solution to that (probably best approach) is to introduce a required flag (--providers/--no-providers) for non-db-tests and filter out / core provider test folders when respective flag is used, and run two separate jobs in our CI - one for providers and one for core.
That would require to have to separate jobs in ci.yml. "Non-db Core tests", "Non-db Providers tests" - each of them passing the flag down to run-unit-tests.yml composite workflow.
Activity
gopidesupavan commentedon Oct 1, 2024
@potiuk I am interested in this should I take this on? Might take bit time to workout this, this area new to me , but this will help me to understand more on the airflow ci/cd . :)
Fix testing tests command to allow passing test as extra arg
Fix testing tests command to allow passing test as extra arg
[-]Separate non-db-tests to providers/non-providers[/-][+]Separate non-db-tests (and others) to providers/non-providers/task_sdk[/+]potiuk commentedon Oct 31, 2024
It's going to be a little more complex it seems - we likely wil have to redesign a bit the way how tests are executed in CI with different test "groups" now so i will start a wider discussion on slack channel to discuss how to do it and involve others.
Fix testing tests command to allow passing test as extra arg (#43529)
gopidesupavan commentedon Oct 31, 2024
Absolutely! I just began analyzing this yesterday to see which components can be grouped or not. I noticed some existing options, like skip-provider-tests, and now that we have the task SDK, we may need to support this for executing tests or providing skipping options as well. I completely agree that some redesign is necessary.
potiuk commentedon Oct 31, 2024
Hey Here. I would like to start a discussion and propose some redesign of our CI - to address some of the issues mentioned here - and involve others so that we come up together with a better design and then we get it implemented together.
First let me state the problems and what is wrong currently. And I will write a separte comment on what would be the first proposal to address it.
ui&task_sdkdirectories to.gitignore#43319 all the tests we had were in "tests" folder. Now we have three test folders for unit tests: "tests", "providers/tests", "task_sdk/tests".Also "system" tests are a separate type - but they are under "tests" in "airflow" and "provider' tests.
Those tests cannot be run together in single pytest command - because all of them have a conftest.py file at the top folder and pytest refuses to run them together because it does not know which
conftest.pyis the "top-level" one.Currently we have "non-db-tests" that runs all the "non-db" tests together in a single xdist/parallel fashion and it currenlty runs together "tests" and "providers/tests" together - but this is really thanks to a workaround for it in "providers" (there is an
__init__.pyin "providers" folder that makes the test belong to package "providers.tests" - but this is really a hack because "providers" is where the whole "providers" project is, it's not a python "package".Both "airflow" and "providers" tests are currently run with
breeze testing testscommand (which by default used to run ALL tests ("airflow + providers") and only when you specified particular packages or modules to run as additional arguments woudl run only those tests you specified. This was a bit "hacky" approach how it was done and one of the reasons why running system tests were broken after provider's separation (i temporarily fixed it in Fix testing tests command to allow passing test as extra arg #43529 but this is really a band-aid).Also the same
breeze testing testscommand is used to run "helm" unit tests - and there is another "hack" implemented - we are also using breeze container to run the tests but "helm" tests do not need to be run in breeze container at all - they need a small local venv we already have from k8s wherehelmis installed - and we can run them thereActually
airflowandtask_sdktests technically (eventually when we fully remove all provider references from airflow core and tests) will not have to use breeze container as well. With UV we could technicaly run the tests in a venv created dynamically in CI (with main constraints /uv lockfor reproducibility) rather than using the CI image - because we do not need alll the 700+ deps - they should be "reproducible-enough" without the CI imaage of ours.We alredy have separate "TaskSDK tests" that are running in the same environment as "Non-DB tests" - but only for "task_sdk" and they have separate
breeze testing task-sdk-tests- it looks like a good direction,potiuk commentedon Oct 31, 2024
My proposal (high level) - it needs some detailed thinking and prototyping regarding the parallel execution of tests in CI is to follow the
breeze testing task-sdk-testsand introduce three more testing commands:breeze testing provider-testscommandbreeze testing helm-testscommandbreeze testint system-testscommandThis means that we will loose some of the parallelism benefifts when we run
providertests andairflowtests together (but instead we will increase a number of jobs we have in CI, whch we still should be able to do without exceeding the limits that INFRA has on a number of parallel jobs we can run in CI jobs).We can also gradually move tests to local venvs where they do not need database containers from docker compose and Pytthon / System dependencies that we currently standardise in the CI image.
Also System tests should have a 'dry-run" mode - where we can run them as part of the CI so that we know that they ar e not broken. We are not able to run system tests for all external systems, but at least we should be able to have some "dry-run" system tests that we should be able to run with all PRs.
So high level we have:
NOW
CI image bound tests:
System tests are not currently executed.
And we can do it this way:
Stage 1🟢 COMPLETED by #43979Likely we could attempt to make TaskSDK and Helm tests to run in venv rather than in CI image even now - I think Task SDK (@kaxil @ashb ?) there are very little dependencies there and we could easily make a local venv there instead of the whole CI image as there are no specific system or provider dependencies there.Also spliting out tests to "Airflow" and "Providers" without moving them out of CI image will hel us to address the issues we have now with conftest.py and actual "separation" of the different test groups.CI image bound tests:* Core DB tests* Core Non-DB Tests* Providers DB tests* Providers Non-DB tests* System tests dry-run testsLocal virtualenv tests
* Helm Unit local virtualenv tests* TaskSDK Non-DB testsStage 2
When we remove all the remaining references to providers from Airflow we might move Airflow Non-DB tests to local virtualenv.
CI image bound tests:
Local virtualenv tests:
Stage 3
We switch the providers tests to use task sdk - and at this stage all db tests should be converted to non-db tests.
CI image bound tests:
Local virtualenv tests:
We will still need provider tests to run in CI image and use the image to calculate constraints etc. - for quite some time (maybe for ever) but I see a clear path and stages we could take to simplify the "Airflow" and Task SDK ones.
Stage 4
When we add the capability of testcontainers (see #43514) the db tests might eventually be run in local virtualenv
CI image bound tests:
Local virtualenv tests:
We will still need provider tests to run in CI image and use the image to calculate constraints etc. - for quite some time (maybe for ever) but I see a clear path and stages we could take to simplify the "Airflow" and Task SDK ones.
potiuk commentedon Oct 31, 2024
I made a few refinements to the initial proposal
ashb commentedon Oct 31, 2024
Yes, there should be no deps on a any providers, not even the new standard provider as all of those need to depend on the SDK.
Given the ability of UV to create predictable venvs with the right versions installed, yeah I agree. Also if it's relevant I think every* test in the task SDK will be stateless and could be parallelized - i.e. they are all non-db style tests
potiuk commentedon Oct 31, 2024
Yeah and starting to have it in a freshly created venv without any DB or whatever deps for Task SDK is actually even GOOD because there we will not have even accidental dependencies that we missed.
27 remaining items
Split tests to core/providers/task-sdk/integration/system
Split tests to core/providers/task-sdk/integration/system
Split tests to core/providers/task-sdk/integration/system (#43979)
Split tests to core/providers/task-sdk/integration/system (apache#43979)
Fix testing tests command to allow passing test as extra arg (#43529)
Remove pytest.mark.db_test: airbyte and amazon providers where possible
Remove pytest.mark.db_test: airbyte and amazon providers where possible
Remove pytest.mark.db_test: airbyte and amazon providers where possib…
pytest.mark.db_testfrom providers where possible #52020Fix testing tests command to allow passing test as extra arg (#43529)
Fix testing tests command to allow passing test as extra arg (#43529)
Fix testing tests command to allow passing test as extra arg (#43529)
Fix testing tests command to allow passing test as extra arg (#43529)
Fix testing tests command to allow passing test as extra arg (#43529)
Fix testing tests command to allow passing test as extra arg (#43529)