[AIRFLOW-867] Enable and fix lots of untested unit tests #2078
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2078 +/- ##
==========================================
+ Coverage 66.98% 67.42% +0.44%
==========================================
Files 142 142
Lines 10731 10735 +4
==========================================
+ Hits 7188 7238 +50
+ Misses 3543 3497 -46
Continue to review full report at Codecov.
|
|
Given the scope of this and the fact that it will impact many in-flight PRs that may need to restructure their unit tests, I think it should get attention quickly from a few sets of eyes. At first glance it looks good and the green light from travis is obviously encouraging. @bolkedebruin @r39132 @criccomini please take a look ps @gsakkis nice work uncovering these sleeper tests :) |
|
@gsakkis great stuff. Can you pease rebase and verify landscape? Might ask you to split it up, as this is a massive and we will indeed catch other changes mid-flight. It also becomes easier to track down issues if the changes are smaller. |
gsakkis
force-pushed
the
rebase/tests-refactor
branch
from
37e1ee0
to
ef09d9e
Compare
|
@bolkedebruin the branch has been rebased. Although the (squashed) diff is big, (a) it is mostly due to renames and (b) almost all changes are in tests so the risk of introducing issues in the working code is minimal. Splitting it into smaller chunks may in fact lead to bugs by omission, e.g. fail to bring back all tests, so I would suggest merging it asap. |
|
@gsakkis please make sure you squash the commits and relate the commit message to the Jira issue as per guidelines. I'll merge when rebased/squash and commit message is proper. |
gsakkis
force-pushed
the
rebase/tests-refactor
branch
from
5f839e5
to
46dbdbd
Compare
gsakkis
force-pushed
the
rebase/tests-refactor
branch
from
4029fef
to
b155af8
Compare
|
@bolkedebruin all done. |
|
@aoen can you please have a look? Some signatures of functions have been changed. |
| @@ -1108,7 +1108,6 @@ def are_dependencies_met( | |||
| :param verbose: whether or not to print details on failed dependencies | |||
| :type verbose: boolean | |||
| """ | |||
| dep_context = dep_context or DepContext() | |||
| @@ -1131,7 +1130,6 @@ def get_failed_dep_statuses( | |||
| self, | |||
| dep_context=None, | |||
| session=None): | |||
| dep_context = dep_context or DepContext() | |||
There was a problem hiding this comment.
Same as above (why remove this line?)
| @@ -81,6 +81,10 @@ def get_dep_statuses(self, ti, session, dep_context): | |||
| :param dep_context: the context for which this dependency should be evaluated for | |||
| :type dep_context: DepContext | |||
| """ | |||
| from airflow.ti_deps.dep_context import DepContext | |||
There was a problem hiding this comment.
We should avoid imports at local scope. I think the previous implementation was OK but it seems you have some concerns?
There was a problem hiding this comment.
The motivation is probably clearer in the original commit before the squash. While it's possible to make dep_context optional without a local import, it requires further refactoring and this PR is already big enough so I opted for the less invasive option. But feel free to refactor it or revert the previous signature (and update the dozens previously broken tests that were passing None) if you feel strongly about it.
|
@aoen @bolkedebruin @gsakkis just want to touch base here -- as feared, we already have merge conflicts with new tests. If we want to commit this we will need to move rapidly. |
|
I still find the local import thing and the signature changes pretty hacky personally, but I won't stop others from +1ing. If my questions re the changed signature are answered and a TODO is added to get rid of the local module imports then I guess that's good enough for now since missing test coverage is a pretty scary thing. |
|
I am all for increased/improved testing, but I want to see this PR being broken up. It touches several areas outside testing and this is where I feel uncomfortable. Such a large PR I am just not able to review properly. |
|
@gsakkis Firstly, thanks for taking this on. It's been a while and the project has improved a lot since this time. If you'd like to reopen this PR, we will take an earnest look at it with a plan to merge quickly. I will close this PR for now and the associated JIRA. Pls reopen if you would like to revisit this work. |
Dear Airflow Maintainers,
Please accept this PR that addresses the following issues:
Master branch: Ran 256 tests in 819.435s
This branch: Ran 360 tests in 912.790s
It also fixes a few subtle bugs not captured by the already enabled tests.