You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/03/12 18:05:08 UTC
[GitHub] [airflow] harishkrao opened a new pull request #22213: Issue 20453 google common cloud fixes part 1
harishkrao opened a new pull request #22213:
URL: https://github.com/apache/airflow/pull/22213
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
related: #20453
The PR is related to the Big Query and PubSub providers' unit tests. Fixed include:
* Changing the tests to assert instead of having only `has_calls`.
* Pre-commit linting
This PR will be followed by at least one more PR to fix the remaining Google cloud provider unit tests mentioned in this issue.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] harishkrao commented on a change in pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
harishkrao commented on a change in pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#discussion_r829479882
##########
File path: tests/providers/google/cloud/hooks/test_dataproc.py
##########
@@ -474,7 +474,7 @@ def test_wait_for_job(self, mock_get_job):
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
]
- mock_get_job.has_calls(calls)
+ mock_get_job.assert_has_calls(calls, any_order=True)
Review comment:
@uranusjr @potiuk Agreed, removed it and re-ran the tests, they succeeded. Pushed the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] harishkrao commented on a change in pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
harishkrao commented on a change in pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#discussion_r830777402
##########
File path: tests/providers/google/cloud/hooks/test_dataproc.py
##########
@@ -474,7 +474,7 @@ def test_wait_for_job(self, mock_get_job):
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
]
- mock_get_job.has_calls(calls)
+ mock_get_job.assert_has_calls(calls, any_order=True)
Review comment:
@uranusjr Removed them from the remaining tests in the PR. Please let me know if they look good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#issuecomment-1077595835
LGTM. @uranusjr ? (BTW. Thanks @harishkrao for taking this one :) . It's unnerving to see tests that well .... test nothing :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#discussion_r830454196
##########
File path: tests/providers/google/cloud/hooks/test_dataproc.py
##########
@@ -474,7 +474,7 @@ def test_wait_for_job(self, mock_get_job):
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
]
- mock_get_job.has_calls(calls)
+ mock_get_job.assert_has_calls(calls, any_order=True)
Review comment:
What about other usages?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#discussion_r825386896
##########
File path: tests/providers/google/cloud/hooks/test_dataproc.py
##########
@@ -474,7 +474,7 @@ def test_wait_for_job(self, mock_get_job):
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
]
- mock_get_job.has_calls(calls)
+ mock_get_job.assert_has_calls(calls, any_order=True)
Review comment:
Why `any_order` here? Can we order the mock calls correctly so this is not needed?
(Same applies for other usages)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#discussion_r827003373
##########
File path: tests/providers/google/cloud/hooks/test_dataproc.py
##########
@@ -474,7 +474,7 @@ def test_wait_for_job(self, mock_get_job):
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
]
- mock_get_job.has_calls(calls)
+ mock_get_job.assert_has_calls(calls, any_order=True)
Review comment:
Also it is strange as the calls seem to be repeated so any order should not be needed ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] harishkrao commented on pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
harishkrao commented on pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#issuecomment-1065948965
@potiuk @mik-laj Opened another PR for the same issue, can you please take a look when you have time? Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] harishkrao commented on a change in pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
harishkrao commented on a change in pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#discussion_r829479882
##########
File path: tests/providers/google/cloud/hooks/test_dataproc.py
##########
@@ -474,7 +474,7 @@ def test_wait_for_job(self, mock_get_job):
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
]
- mock_get_job.has_calls(calls)
+ mock_get_job.assert_has_calls(calls, any_order=True)
Review comment:
@uranusjr @potiuk Agreed, removed it and re-ran the tests, they succeeded. Pushed the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#issuecomment-1078833830
I rebased to rebuild. We had some issues yesterday.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #22213:
URL: https://github.com/apache/airflow/pull/22213
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] harishkrao commented on pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
harishkrao commented on pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#issuecomment-1078614670
Thank you @uranusjr and @potiuk for your reviews and comments :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] harishkrao commented on a change in pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
harishkrao commented on a change in pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#discussion_r830777747
##########
File path: tests/providers/google/cloud/hooks/test_pubsub.py
##########
@@ -371,7 +371,9 @@ def test_publish(self, mock_service):
mock.call(topic=EXPANDED_TOPIC, data=message.get("data", b''), **message.get('attributes', {}))
for message in TEST_MESSAGES
]
- publish_method.has_calls(calls)
+ mock_calls_result = publish_method.mock_calls
+ result_refined = [mock_calls_result[0],mock_calls_result[2],mock_calls_result[4]]
Review comment:
@uranusjr For this change, I had to filter the specific calls from the mock because there are additional calls being made. Filtering allows me to do an assert on the required calls.
##########
File path: tests/providers/google/cloud/transfers/test_sheets_to_gcs.py
##########
@@ -114,9 +114,11 @@ def test_execute(self, mock_upload_data, mock_xcom, mock_sheet_hook, mock_gcs_ho
)
calls = [mock.call(spreadsheet_id=SPREADSHEET_ID, range_=r) for r in RANGES]
- mock_sheet_hook.return_value.get_values.has_calls(calls)
+ mock_sheet_hook.return_value.get_values.assert_has_calls(calls)
calls = [mock.call(mock_gcs_hook, mock_sheet_hook, r, v) for r, v in zip(RANGES, data)]
- mock_upload_data.has_calls(calls)
-
+ mock_upload_data.assert_called()
Review comment:
@uranusjr I had to split the `assert_has_calls` into two tests because the mock_calls and the calls given in the list had differences, such as `GCSHook` vs `GCSHook()`. If you can suggest a better test/tests, I am happy to implemen them.
Two assert tests I wrote are:
1. Count the number of calls made by the mock and compare with the expected calls
2. Was the mock called at least once?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#discussion_r830454196
##########
File path: tests/providers/google/cloud/hooks/test_dataproc.py
##########
@@ -474,7 +474,7 @@ def test_wait_for_job(self, mock_get_job):
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
mock.call(region=GCP_LOCATION, job_id=JOB_ID, project_id=GCP_PROJECT),
]
- mock_get_job.has_calls(calls)
+ mock_get_job.assert_has_calls(calls, any_order=True)
Review comment:
What about other usages? Can those be removed as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #22213: Issue 20453 google common cloud fixes part 1
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #22213:
URL: https://github.com/apache/airflow/pull/22213#issuecomment-1077595900
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org