Skip to content
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

[AIRFLOW-6763] Make systems tests ready for backport tests #7389

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Feb 9, 2020

We will run system test on back-ported operators for 1.10* series of airflow
and for that we need to have support for running system tests using pytest's
markers and reading environment variables passed from HOST machine (to pass
credentials). 

This is the first step to automate system tests execution.


Issue link: AIRFLOW-6763

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

@boring-cyborg boring-cyborg bot added area:dev-tools provider:google Google (including GCP) related issues labels Feb 9, 2020
@potiuk
Copy link
Member Author

potiuk commented Feb 9, 2020

@nuclearpinguin @mik-laj (and @ashb / @kaxil ) - this is a change that makes it very easy to run system tests using pytest's markers in the breeze/CI environment. We will use it to test all the backported 2.0 packages on older airlfow versions. For now for Google/GCP for which we have system tests integrated, but it's very generic and we can apply it for other systems as well as we will have a possibility to run the system tests automatically in the future when we move to github actions.

@potiuk potiuk force-pushed the AIRFLOW-6763-make-system-tests-ready-for-backport-tests branch from d844348 to ac55753 Compare February 9, 2020 21:11
TESTING.rst Outdated Show resolved Hide resolved
TESTING.rst Outdated Show resolved Hide resolved
TESTING.rst Outdated Show resolved Hide resolved
TESTING.rst Outdated Show resolved Hide resolved
TESTING.rst Outdated Show resolved Hide resolved
TESTING.rst Outdated Show resolved Hide resolved
TESTING.rst Outdated Show resolved Hide resolved
@ashb
Copy link
Member

ashb commented Feb 9, 2020

I've skimmed the changes and they look fine, but I don't see how this is connected to testing backported operators?

@ashb
Copy link
Member

ashb commented Feb 9, 2020

(i.e. why the system bit, Vs unit test is needed to check the backport)

@potiuk
Copy link
Member Author

potiuk commented Feb 9, 2020

I've skimmed the changes and they look fine, but I don't see how this is connected to testing backported operators?

I am writing devlist message shortly about it.

@codecov-io
Copy link

codecov-io commented Feb 9, 2020

Codecov Report

Merging #7389 into master will increase coverage by 0.43%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7389      +/-   ##
==========================================
+ Coverage   86.26%   86.69%   +0.43%     
==========================================
  Files         877      877              
  Lines       41221    41242      +21     
==========================================
+ Hits        35559    35755     +196     
+ Misses       5662     5487     -175
Impacted Files Coverage Δ
...viders/docker/example_dags/example_docker_swarm.py 100% <0%> (ø) ⬆️
...ow/providers/docker/example_dags/example_docker.py 100% <0%> (ø) ⬆️
airflow/utils/dag_processing.py 88.12% <0%> (+0.19%) ⬆️
airflow/models/dag.py 91.16% <0%> (+0.25%) ⬆️
airflow/jobs/scheduler_job.py 89.66% <0%> (+0.43%) ⬆️
airflow/jobs/backfill_job.py 92.09% <0%> (+1.12%) ⬆️
airflow/hooks/dbapi_hook.py 91.73% <0%> (+1.65%) ⬆️
airflow/configuration.py 91.43% <0%> (+3.42%) ⬆️
airflow/providers/postgres/hooks/postgres.py 94.36% <0%> (+16.9%) ⬆️
airflow/kubernetes/refresh_config.py 74.5% <0%> (+23.52%) ⬆️
... and 6 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 9cbd7de...9751b69. Read the comment docs.

TESTING.rst Outdated Show resolved Hide resolved
TESTING.rst Outdated Show resolved Hide resolved
@potiuk potiuk force-pushed the AIRFLOW-6763-make-system-tests-ready-for-backport-tests branch from e6faaed to 1630c44 Compare February 9, 2020 23:26
TESTING.rst Outdated Show resolved Hide resolved
@potiuk
Copy link
Member Author

potiuk commented Feb 10, 2020

@ashb > (i.e. why the system bit, Vs unit test is needed to check the backport)

The backported packages are already unit-tested. They are copied straight from the master where the unit tests have been run and are green in CI.

There is basically no easy way - unless you want to rewrite manually all the tests - to run unit tests with back-ported operators/hooks/sensors from 2.0 in the context of Airlfow 1.10. I at least could not find any way to do it easily - but if you think you can do it easily, it would be great. I just think it is awfully lot of effort.

Many of the unit tests use mocking for internal structure of packages and it would not work to convert those.

And it would not make a lot of sense because we would just (again) test the same - behaviors of the operators - pretty much all the "operators/hooks/sensors" test are mocking out the communication to the external systems and test internal logic of the operators. We do not need to test it again - we know that this works.

What we do not know if the operators are working with airflow-1.10 framework and whether the framework changes did not spoil anything. So we really need to test integration of the operators from 2.0 to 1.10 and the best we can do here is to run those (already unit-tested) operators in the context of 1.10 talking to the real system and see if they are still working there. We have a lot of such tests for GCP and we can fairly easiy test those 2.0 operators to run on top of airflow 1.10.x. Which is really what we want to know.

@potiuk
Copy link
Member Author

potiuk commented Feb 10, 2020

@ashb -> one more thing. With few more breeze's modifications, It will actually be possible (bear with me) to also run unit tests with the old version of airflow. I plan to make it super easy to remove "sources" of airflow but leave tests and install any old version of Airflow inside the breeze docker. That will - in theory make it possible to run the "unit" and "integration" tests against old version of Airflow. But I am afraid due to this mocking it will not really work in many cases.

@potiuk potiuk force-pushed the AIRFLOW-6763-make-system-tests-ready-for-backport-tests branch from 1630c44 to a51b9aa Compare February 12, 2020 12:42
@potiuk
Copy link
Member Author

potiuk commented Feb 12, 2020

Hello @ashb @mik-laj @nuclearpinguin @kaxil @dimberman -> I updated the implementation now and it works really nicely.

Breeze is now turned into "airflow_1.10." testing playground. It will be super-useful to automatically run system tests (currently only for google/ operators) using the latests tests and "example_dags" from "providers", but using the installed airflow version.

You can run ./breeze --install-airflow-version <VERSION> . If you run it with "current" version it works as previously (current version from sources). If you run it with '1.10.1' -> '1.10.9' it will remove the current sources, will uninstall installed airflow master and will install the released version from PIP. The automated installation works out of the box for all versions: 1.10.1 -> 1.10.9.

There is an easy way to test installation and run system tests of the released "backported" providers package.

If you have binary wheel "providers" package prepared locally or downloaded, you just need to copy the wheel package to "files" dir. You can download it from the latest build of teh

Then inside the container with installed airflow version you should be able to run: pip install /files/<NAME_OF_THE_FILE>.whl to install the "providers" package. And then the example tests should run out of the box for most operators.

The "/opt/airflow/tests" folders and "/providers" folders are mapped into the container (and system test class handles it properly) so that we can still modify the tests locally and run them with airflow 1.10.*.

For example, right after installing the 'providers' package you should be able to run:

pytest tests/providers/google/cloud/operators/test_functions_system.py --systems google.cloud and the system tests

It will work for both - 2.0 and 1.10.* series providing that you have proper GCP credentials in "files/airflow-breeze-config/variables.env"

@potiuk potiuk force-pushed the AIRFLOW-6763-make-system-tests-ready-for-backport-tests branch from a51b9aa to 1213b27 Compare February 12, 2020 13:05
BREEZE.rst Outdated Show resolved Hide resolved
BREEZE.rst Outdated Show resolved Hide resolved
TESTING.rst Outdated Show resolved Hide resolved
TESTING.rst Outdated Show resolved Hide resolved
@potiuk
Copy link
Member Author

potiuk commented Feb 13, 2020

Hey @ashb @mik-laj @nuclearpinguin -> all green for the system tests ready for backporting. BTW, I upladed the docs and added "How to write system tests" doc...

@potiuk
Copy link
Member Author

potiuk commented Feb 14, 2020

Ping.

@ashb
Copy link
Member

ashb commented Feb 14, 2020

Does this fall under AIP-4? That is still draft. Should we vote on that AIP?

@potiuk
Copy link
Member Author

potiuk commented Feb 14, 2020

Sure. I can start a discussion and vote on it - but I think we could merge that one independently (and show it as an example) - for now it's limited only to GCP but it can show others how it can be done in an easy way.

I believe AIP-4 is is much more about running the System Tests in automated way rather than separating them out (and it requires Github Actions move to be completed and some serious discussion on how to store credentials). We've already added some separation (skipIfs, _system.py in GCP) - now we have a generic mechanism but it does not mean we introduce it in "global way" yet.

I think it's best if we merge this one first and then I could start a discussion on how to make the AIP-4 automation working. Actually i wanted to start another discussion right after about involving the community to add system tests to all other provider packages - so that we can automate it with AIP-4 as well. So the discussion might be quite big - having a working, generic implementation in GCP that is generic enough to be reused and to show how it works manually is a great way for people to take part in the discussion.

So for me this is more of an enabler for AIP-4 discussion to happen.

WDYT @ashb ?

@potiuk potiuk force-pushed the AIRFLOW-6763-make-system-tests-ready-for-backport-tests branch 4 times, most recently from f577fd3 to 9751b69 Compare February 18, 2020 00:28
@turbaszek
Copy link
Member

It's a big change but it looks good to me from what I see :)

@dimberman dimberman self-requested a review February 18, 2020 19:32
@potiuk potiuk force-pushed the AIRFLOW-6763-make-system-tests-ready-for-backport-tests branch from 9751b69 to 8409e95 Compare February 18, 2020 20:05
@potiuk
Copy link
Member Author

potiuk commented Feb 18, 2020

Applied changes from @ashb comments in #7439

We will run system test on back-ported operators for 1.10* series of airflow
and for that we need to have support for running system tests using pytest's
markers and reading environment variables passed from HOST machine (to pass
credentials). 

This is the first step to automate system tests execution.
@potiuk potiuk force-pushed the AIRFLOW-6763-make-system-tests-ready-for-backport-tests branch from feae6b3 to 542d257 Compare February 20, 2020 22:04
@potiuk
Copy link
Member Author

potiuk commented Feb 20, 2020

rebased and hopefully all good now.

@potiuk
Copy link
Member Author

potiuk commented Feb 20, 2020

also applied the comments from @efedotova !

@potiuk potiuk merged commit 848fbab into apache:master Feb 21, 2020
potiuk added a commit that referenced this pull request Feb 25, 2020
We will run system test on back-ported operators for 1.10* series of airflow
and for that we need to have support for running system tests using pytest's
markers and reading environment variables passed from HOST machine (to pass
credentials). 

This is the first step to automate system tests execution.

(cherry picked from commit 848fbab)
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
We will run system test on back-ported operators for 1.10* series of airflow
and for that we need to have support for running system tests using pytest's
markers and reading environment variables passed from HOST machine (to pass
credentials). 

This is the first step to automate system tests execution.
kaxil pushed a commit that referenced this pull request Mar 19, 2020
We will run system test on back-ported operators for 1.10* series of airflow
and for that we need to have support for running system tests using pytest's
markers and reading environment variables passed from HOST machine (to pass
credentials). 

This is the first step to automate system tests execution.

(cherry picked from commit 848fbab)
kaxil pushed a commit to astronomer/airflow that referenced this pull request Mar 19, 2020
We will run system test on back-ported operators for 1.10* series of airflow
and for that we need to have support for running system tests using pytest's
markers and reading environment variables passed from HOST machine (to pass
credentials). 

This is the first step to automate system tests execution.

(cherry picked from commit 848fbab)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants