-
Notifications
You must be signed in to change notification settings - Fork 14.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix --ignore
-related clobbering when breeze tests providers
#41967
Fix --ignore
-related clobbering when breeze tests providers
#41967
Conversation
b042b5f
to
c6069b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Very cool ! Thanks for looking so closely at this one :) |
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).NB: the bug 1 tests and code were written in a TDD style - the first commit adds the suspension/exclusion tests that need to succeed, the second commit fixes the production code so the tests go green. The primary-arg test is always green and demonstrates the bug 2 behavior.
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 theextra_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
).