[
https://issues.apache.org/jira/browse/AIRFLOW-5720?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Daniel Standish updated AIRFLOW-5720:
-------------------------------------
Description:
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. this line:
{{default_gcp_project_id=BaseHook.get_connection(gcp_conn_id).extra_dejson.get(}}
All this line serves to accomplish is to discard the first connection,
{{gcp_connection}}. This is the first invocation of
{{BaseHook.get_connection}}.
The second invocation is the one that matters, namely when
{{CloudSqlDatabaseHook}} calls `self.get_connection`.
This is when {{cloudsql_connection}} is returned. But since it is a mock side
effect, it doesn't matter what value you give for conn_id. So the 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.
was:
Test should not reference "private" methods. This impedes refactoring.
Also some other issues with these tests.
More detail TBD
> 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. this line:
> {{default_gcp_project_id=BaseHook.get_connection(gcp_conn_id).extra_dejson.get(}}
>
> All this line serves to accomplish is to discard the first connection,
> {{gcp_connection}}. This is the first invocation of
> {{BaseHook.get_connection}}.
> The second invocation is the one that matters, namely when
> {{CloudSqlDatabaseHook}} calls `self.get_connection`.
> This is when {{cloudsql_connection}} is returned. But since it is a mock
> side effect, it doesn't matter what value you give for conn_id. So the 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
