[AIRFLOW-5631] Change way of running GCP system tests by turbaszek · Pull Request #6299 · apache/airflow

turbaszek · 2019-10-10T11:43:26Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-5631

Description

Here are some details about my PR, including screenshots of any UI changes:
This PR proposes a new way of running GCP related system tests.
It uses SystemTests base class and authentication is provided by a
context manager thus it's easier to understand what's going on.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

airflow/gcp/hooks/base.py

codecov-io · 2019-10-10T12:24:47Z

Codecov Report

Merging #6299 into master will decrease coverage by 0.29%.
The diff coverage is 94.91%.

@@            Coverage Diff            @@
##           master    #6299     +/-   ##
=========================================
- Coverage   83.96%   83.67%   -0.3%     
=========================================
  Files         627      628      +1     
  Lines       36549    36608     +59     
=========================================
- Hits        30689    30632     -57     
- Misses       5860     5976    +116

Impacted Files	Coverage Δ
airflow/contrib/hooks/gcp_api_base_hook.py	`100% <ø> (ø)`	⬆️
airflow/gcp/utils/credentials_provider.py	`94.91% <94.91%> (ø)`
airflow/kubernetes/volume_mount.py	`44.44% <0%> (-55.56%)`	⬇️
airflow/kubernetes/volume.py	`52.94% <0%> (-47.06%)`	⬇️
airflow/kubernetes/pod_launcher.py	`45.25% <0%> (-46.72%)`	⬇️
airflow/kubernetes/kube_client.py	`33.33% <0%> (-41.67%)`	⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py	`70.14% <0%> (-28.36%)`	⬇️
airflow/contrib/operators/ssh_operator.py	`82.5% <0%> (-1.25%)`	⬇️
airflow/models/taskinstance.py	`93.28% <0%> (-0.51%)`	⬇️
airflow/utils/dag_processing.py	`58.15% <0%> (-0.33%)`	⬇️
... and 2 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 37a342d...4d7f100. Read the comment docs.

mik-laj · 2019-10-14T18:25:37Z

Travis is sad. Can you fix it?

airflow/utils/log/colored_log.py

mik-laj · 2019-10-21T08:20:08Z

tests/gcp/operators/test_cloud_sql_system.py

In other places, the helper is set as a class property. Do you have recommendations about it?

I think the helpers need some more discussion. Many of them performs same thing like create a bucket or delete a bucket, so maybe it's worth to rethink our overall approach. If we are going to migrate to pytest then helpers should be replaced by fixtures.

Ok. We can do it in separate PR.

mik-laj

Small nits.

airflow/gcp/utils/credentials_provider.py

mik-laj · 2019-10-21T15:44:06Z

tests/gcp/operators/test_cloud_sql_system.py

Ok. We can do it in separate PR.

tests/test_utils/gcp_system_helpers.py

mik-laj · 2019-10-21T16:08:07Z

tests/gcp/utils/base_gcp_system_test_case.py

Is this class still used?

No, I should remove it ✅

This commit proposes a new way of running GCP related system tests. It uses SystemTests base class and authentication is provided by a context manager thus it's easier to understand what's going on.

TobKed · 2019-11-04T17:58:07Z

tests/gcp/operators/test_bigquery_system.py


-@unittest.skipIf(TestDagGcpSystem.skip_check(GCP_BIGQUERY_KEY), SKIP_TEST_WARNING)
-class BigQueryExampleDagsSystemTest(TestDagGcpSystem):
+@skip_gcp_system(GCP_BIGQUERY_KEY, require_local_executor=True)


This parameters should not have require_local_executor set to True I think. I have problem with running this test.

Yes, I agree. Can you make a PR with fix because I am on holidays?

Sorry for late response. I will try to fix it but I have limited time due to my holidays :)

mik-laj reviewed Oct 10, 2019

View reviewed changes