[
https://issues.apache.org/jira/browse/AIRFLOW-5720?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16974561#comment-16974561
]
ASF GitHub Bot commented on AIRFLOW-5720:
-----------------------------------------
dstandish commented on pull request #6390: [AIRFLOW-5720] don't call
_get_connections_from_db in cloud sql test
URL: https://github.com/apache/airflow/pull/6390
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
> don't call _get_connections_from_db in TestCloudSqlDatabaseHook
> ---------------------------------------------------------------
>
> Key: AIRFLOW-5720
> URL: https://issues.apache.org/jira/browse/AIRFLOW-5720
> Project: Apache Airflow
> Issue Type: New Feature
> Components: gcp
> Affects Versions: 1.10.5
> Reporter: Daniel Standish
> Assignee: Daniel Standish
> Priority: Major
>
> Issues with this test class:
> *tests are mocking lower-level than they need to*
> Tests were mocking {{airflow.hook.BaseHook.get_connections}}.
> Instead they can mock
> {{airflow.gcp.hooks.cloud_sql.CloudSqlDatabaseHook.get_connection}} which is
> more direct.
> *should not reference private method*
> This is an impediment to refactoring of connections / creds.
> *Tests had complexity that did not add a benefit*
> They all had this bit:
> {code:python}
> self._setup_connections(get_connections, uri)
> gcp_conn_id = 'google_cloud_default'
> hook = CloudSqlDatabaseHook(
>
> default_gcp_project_id=BaseHook.get_connection(gcp_conn_id).extra_dejson.get(
> 'extra__google_cloud_platform__project')
> )
> {code}
> {{_setup_connections}} was like this:
> {code:python}
> @staticmethod
> def _setup_connections(get_connections, uri):
> gcp_connection = mock.MagicMock()
> gcp_connection.extra_dejson = mock.MagicMock()
> gcp_connection.extra_dejson.get.return_value = 'empty_project'
> cloudsql_connection = Connection()
> cloudsql_connection.parse_from_uri(uri)
> cloudsql_connection2 = Connection()
> cloudsql_connection2.parse_from_uri(uri)
> get_connections.side_effect = [[gcp_connection],
> [cloudsql_connection],
> [cloudsql_connection2]]
> {code}
> Issues here are as follows.
> 1. no test ever used the third side effect
> 2. the first side effect does not help us; {{default_gcp_project_id}} is
> irrelevant
> Only one of the three connections in {{_setup_connections}} has any impact on
> the test.
> The call of {{BaseHook.get_connection}} only serves to discard the first
> connection in mock side effect list, {{gcp_connection}}.
> The second connection is the one that matters, and it is returned when
> {{CloudSqlDatabaseHook}} calls `self.get_connection` during init. Since it
> is a mock side effect, it doesn't matter what value is given for conn_id. So
> the {{CloudSqlDatabaseHook}} init param {{default_gcp_project_id}} has no
> consequence. And because it has no consequence, we should not supply a value
> for it because this is misleading.
> We should not have extra code that does not serve a purpose because it makes
> it harder to understand what's actually happening.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
