[AIRFLOW-867] Enable and fix lots of untested unit tests

[gsakkis]

Dear Airflow Maintainers,

Please accept this PR that addresses the following issues:

• https://issues.apache.org/jira/browse/AIRFLOW-867

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.

[codecov-io]

Codecov Report

Merging #2078 into master will increase coverage by 0.44%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
airflow/models.py	86.27% <ø> (+0.19%)	✅
airflow/ti_deps/deps/base_ti_dep.py	88.37% <100%> (+4.58%)	✅
airflow/operators/latest_only_operator.py	93.1% <100%> (+65.51%)	✅
airflow/executors/sequential_executor.py	86.36% <ø> (-13.64%)	❌
airflow/utils/state.py	83.33% <ø> (-13.34%)	❌
airflow/task_runner/bash_task_runner.py	93.33% <ø> (-6.67%)	❌
airflow/utils/helpers.py	41.08% <ø> (-2.48%)	❌
airflow/jobs.py	73.17% <ø> (+0.1%)	✅
airflow/ti_deps/deps/trigger_rule_dep.py	95.16% <ø> (+1.61%)	✅
... and 14 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44fcabc...b155af8. Read the comment docs.

[jlowin]

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 :)

[bolkedebruin]

@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]

@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.

[bolkedebruin]

@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]

@bolkedebruin all done.

[bolkedebruin]

@aoen can you please have a look? Some signatures of functions have been changed.

[aoen]

Why remove this line?

[aoen]

Same as above (why remove this line?)

[aoen]

We should avoid imports at local scope. I think the previous implementation was OK but it seems you have some concerns?

[gsakkis]

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.

[jlowin]

@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.

[aoen]

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.

[bolkedebruin]

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.

[r39132]

@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.