-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Fix flaky providers test by proper google deprecations_ignore.yml placing
#46638
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
Conversation
deprecations_ignore.yml
|
I was able to reproduce problem with google deprecation locally but it's strange and root cause is still not clear for me, calling test with providers standard test included triggers warning failure (from breeze shell): But calling only google fixes the test, I believe the config order matters here somehow |
ashb
left a comment
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.
If we're adding them here are they duplicated in the other ignore file? Can we remove them from there?
(Also wow that's quite a lot of warnings to ignore! What warning are we actually ignoring here?)
I think so (at least can confirm it works with local tests). Removed, I've also added a comment in conftest
I am not very familiar with google provider but there are lots of deprecation warnings inside it and very intricate machinery to generate them sometimes. This provider is HUGE. But I've just moved the list of ignores that were already present. I hope the change in EDIT: yeah they were triggered and even with deleted deprecation ignore lines from main file it works now with the whole providers test suit |
deprecations_ignore.ymldeprecations_ignore.yml placing
|
I believe the approach I took in #46608. that cleans-up a lot of common and duplicated provider code after we moved all providers is better. It should fix the deprecations_ignore.yml problem. After removing the "old" way providers were added and basically removing the entire "providers" package we could do the deprecations_ignore differently - namely instead of adding deprecations_ignore purely for the provider in question, we are adding all deprecations_ignore found in all providers, ALWAYS in pytest_plugin. This way: a) we can keep deprecations_ignore in corresponding providers I have to still make the PR green - there are still few things failing there, and also add some more docs and templating/pre-commits to make sure that "integration" and "system" tests in provider have the right init.py files and few more things to remove, but this evening the tests in CI already look very promising - I have most integration tests running and the deprecation warnings from google provider seem to be handled well - via the above mechanism. cc: @ashb
We are ignoring all the warnings that are deprecations that we know about and will remove in the future. Generally our approach is that our tests should be "warning free" and that's our end-goal, but in some cases we will have some intentional deprecations that we want to keep together with tests. This is the reason why we have this mechanism here to intentionally mark some of those deprecations as "expected" Possibly we can figure out even better way of doing it - such as marking certain deprecations as "ignorable" by the tests - which I think is a good idea and since we already have the specific deprecation warnings in Google, we might implement it and make similar approach for all other providers. But that should be one of the next steps I think after we clean-up stuff after provider move. Currently the problem is nearly not present, because as part of cleanup we removed all deprecation warnings (except the Google ones which we agred have already specific deprecation schedule and we are fine with it as discussed and agreed on the devlist), but we will have more of those "expected but ignorable" deprecation warnings as we introduce changes in providers - and maybe it's time to think about such mechanism |
|
Thank you for clarification! The main reason for this PR was to unblock #46612 and fix scheduled CI pipelines. Now it is merged, so there is no reason to do it "hacky" way. I will close this one. Maybe it's a good idea to describe reasons behind deprecation ignores for google somewhere close to the tests. |
Absolutely. PRs are most welcome for that ! |
I believe it should wait for #46608 to be merged and after that, I could explain it better with the new mechanism in place. Is it appropriate to put something like |
|
The PR is merged now - also see the discussion i started on the devlist https://lists.apache.org/thread/xm6th19xw2v1zo8k8kjomsbzz51njxgv -> I think, the description of deprecations_ignore behaviour is better in contributing docs - somewhere in https://github.com/apache/airflow/tree/main/contributing-docs/testing - especially that we have no longer "providers/tests" folder :D |
This MR fixes flaky google provider deprecation warnings by returning ignore-file missed during provider migration. Previously all providers' tests would init
providers/tests/conftest.pybut now each provider tests are placed in it's own directories.This specific ignore file allows us to not depend on what provider directories are present in pytest call.
For example here only specific providers are present and the check failed due to warnings:
And here the whole suit is running and everything is ok:
^ 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.rstor{issue_number}.significant.rst, in newsfragments.