You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "topherinternational (via GitHub)" <gi...@apache.org> on 2024/09/02 23:28:23 UTC
[PR] Fix `--ignore`-related clobbering when breeze tests providers [airflow]
topherinternational opened a new pull request, #41967:
URL: https://github.com/apache/airflow/pull/41967
During #41555 we found two subtle bugs in how Breeze prepares pytest arguments when testing providers. This change fixes one of those bugs, and adds a test for another one which should be fixed in a future PR as it probably requires a refactor.
Both bugs stem from Breeze's attempt to force-skip a provider test if it is supposed to be ignored (#40037) - the code looks for pytest suite args that are also ignored, e.g. "`tests/providers/myprovider`" and "`--ignore=tests/providers/myprovider`". When these are found, the suite arg is removed from the pytest args (normal pytest behavior prioritizes a suite arg over an ignore switch, and runs the suite tests anyway).
### Bug 1:
a. If a provider is marked for removal in compat checks (in the `BASE_PROVIDERS_COMPATIBILITY_CHECKS` global), Breeze adds an ignore switch to the `extra_pytest_args` of the form `--ignore=tests/providers/<provider-id>`
b. If a provider is marked as excluded on Python 3.8 (or suspended), Breeze adds 3 ignore switches for unit, system and integration tests; these switches do _not_ use the `=` to connect key and value and so each ignore is two separate arguments, e.g. `--ignore tests/providers/<provider-id>`
-> When the force-skip code sees the `--ignore=...` switch from (a), it finds the "matching" `tests/providers/<provider-id>` argument and thinks it is the test suite argument, when it is really the value for the ignore switch from (b). This leaves a dangling `--ignore` switch key without a value, causing pytest to fail the command.
The change looks like this (omitting other irrelevant arguments):
`tests/providers --ignore tests/providers/myprovider --ignore tests/system/providers/myprovider --ignore tests/integration/providers/myprovider --ignore=tests/providers/myprovider`
becomes
`tests/providers --ignore --ignore tests/system/providers/myprovider --ignore tests/integration/providers/myprovider --ignore=tests/providers/myprovider`
The result is an invalid pytest command and a test failure.
#### Fix
The simplest and safest fix is to make the exclusion switches (b above) conform to `key=value` switch format. This way, the ignore statement cannot be mistakenly read as a suite arg and removed from the command.
This fix is done in this PR.
### Bug 2:
Bug 2 comes from a combination of the existing force-skip code and behavior introduced by the fix to bug 1.
In lowest-direct-dependency tests, providers are tested individually, with each provider getting its own pytest command testing its specific tests directory. As mentioned above in (b), any excluded provider results in 3 ignore switches being added to the pytest command.
In this scenario, the force-skip code causes the suite arg itself to be removed from the pytest command:
`tests/providers/myprovider --ignore=tests/providers/myprovider --ignore=tests/system/providers/myprovider --ignore tests/integration/providers/myprovider`
becomes
`tests/providers/myprovider --ignore=tests/providers/myprovider --ignore=tests/system/providers/myprovider --ignore tests/integration/providers/myprovider`
This is a valid pytest command, which tests every test pytest can find. The intended provider is excluded, as expected, but we also expected no other tests to be run instead of running the rest of the world.
#### Fix
This fix is marked as a todo in the test code. Probably it requires refactoring the `_run_test` function so that it can skip running a test entirely; it seems like a skip is the best option for a situation where a specific test suite is expected to be run but also expected to be ignored. (Another nice part of the refactor would be splitting out the argument-generating code into functions, so args could be tested without calling `_run_test`).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `--ignore`-related clobbering when breeze tests providers [airflow]
Posted by "topherinternational (via GitHub)" <gi...@apache.org>.
topherinternational commented on code in PR #41967:
URL: https://github.com/apache/airflow/pull/41967#discussion_r1741306494
##########
dev/breeze/src/airflow_breeze/utils/run_tests.py:
##########
@@ -288,7 +277,7 @@ def convert_test_type_to_pytest_args(
return find_all_other_tests()
test_dirs = TEST_TYPE_MAP_TO_PYTEST_ARGS.get(test_type)
if test_dirs:
- return test_dirs
+ return test_dirs.copy()
Review Comment:
Added this copy bc the rerunning of tests was modifying this list.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `--ignore`-related clobbering when breeze tests providers [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #41967:
URL: https://github.com/apache/airflow/pull/41967#issuecomment-2325635202
Very cool ! Thanks for looking so closely at this one :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `--ignore`-related clobbering when breeze tests providers [airflow]
Posted by "topherinternational (via GitHub)" <gi...@apache.org>.
topherinternational commented on PR #41967:
URL: https://github.com/apache/airflow/pull/41967#issuecomment-2325619594
> Nice! Thanks for being so dilligent. Are you going to do the refactor for `#2` or would you like for me to take a stab on it :) ?
I can do it, already sketched out a couple of the tests.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `--ignore`-related clobbering when breeze tests providers [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #41967:
URL: https://github.com/apache/airflow/pull/41967#issuecomment-2325634839
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix `--ignore`-related clobbering when breeze tests providers [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #41967:
URL: https://github.com/apache/airflow/pull/41967
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org