[GitHub] [superset] dpgaspar commented on a change in pull request #12520: test: hive db engine spec

[GitBox <gi...@apache.org>]

dpgaspar commented on a change in pull request #12520:
URL: https://github.com/apache/superset/pull/12520#discussion_r558343251



##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -247,3 +265,83 @@ def is_readonly(sql: str) -> bool:
     assert is_readonly("EXPLAIN SELECT 1")
     assert is_readonly("SELECT 1")
     assert is_readonly("WITH (SELECT 1) bla SELECT * from bla")
+
+
+def test_upload_to_s3_no_bucket_path():
+    with pytest.raises(
+        Exception,
+        match="No upload bucket specified. You can specify one in the config file.",
+    ):
+        upload_to_s3("filename", "prefix", Table("table"))
+
+
+@mock.patch("boto3.client")
+@mock.patch(
+    "superset.db_engine_specs.hive.config",
+    {**app.config, "CSV_TO_HIVE_UPLOAD_S3_BUCKET": "bucket"},
+)
+def test_upload_to_s3_client_error(client):
+    from botocore.exceptions import ClientError
+
+    client.return_value.upload_file.side_effect = ClientError(
+        {"Error": {}}, "operation_name"
+    )
+
+    with pytest.raises(ClientError):
+        upload_to_s3("filename", "prefix", Table("table"))
+
+
+@mock.patch("boto3.client")
+@mock.patch(
+    "superset.db_engine_specs.hive.config",
+    {**app.config, "CSV_TO_HIVE_UPLOAD_S3_BUCKET": "bucket"},
+)
+def test_upload_to_s3_success(client):
+    client.return_value.upload_file.return_value = True
+
+    location = upload_to_s3("filename", "prefix", Table("table"))
+    assert f"s3a://bucket/prefix/table" == location
+
+
+def test_fetch_data_query_error():
+    from TCLIService import ttypes
+
+    err_msg = "error message"
+    cursor = mock.Mock()
+    cursor.poll.return_value.operationState = ttypes.TOperationState.ERROR_STATE
+    cursor.poll.return_value.errorMessage = err_msg
+    with pytest.raises(Exception, match=f"('Query error', '{err_msg})'"):
+        HiveEngineSpec.fetch_data(cursor)
+
+
+@mock.patch("superset.db_engine_specs.base.BaseEngineSpec.fetch_data")
+def test_fetch_data_programming_error(fetch_data_mock):
+    from pyhive.exc import ProgrammingError
+
+    fetch_data_mock.side_effect = ProgrammingError
+    cursor = mock.Mock()
+    assert HiveEngineSpec.fetch_data(cursor) == []
+
+
+@mock.patch("superset.db_engine_specs.base.BaseEngineSpec.fetch_data")
+def test_fetch_data_success(fetch_data_mock):
+    return_value = ["a", "b"]
+    fetch_data_mock.return_value = return_value
+    cursor = mock.Mock()
+    assert HiveEngineSpec.fetch_data(cursor) == return_value
+
+
+@mock.patch("superset.db_engine_specs.hive.HiveEngineSpec._latest_partition_from_df")
+def test_where_latest_partition(mock_method):

Review comment:
       to increase coverage would be nice to add 2 further tests:
   - `latest_partition` raises an exception
   - when `values` is None and `columns` is None
   

##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -168,6 +169,23 @@ def test_create_table_from_csv_append() -> None:
         )
 
 
+@mock.patch(
+    "superset.db_engine_specs.hive.config",
+    {**app.config, "CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC": lambda *args: True},
+)
+@mock.patch("superset.db_engine_specs.hive.g", spec={})
+@mock.patch("tableschema.Table")
+def test_create_table_from_csv_if_exists_fail(mock_table, mock_g):
+    mock_table.infer.return_value = {}
+    mock_g.user = True
+    mock_database = mock.MagicMock()
+    mock_database.get_df.return_value.empty = False
+    with pytest.raises(SupersetException, match="Table already exists"):
+        HiveEngineSpec.create_table_from_csv(
+            "foo.csv", Table("foobar"), mock_database, {}, {"if_exists": "fail"}

Review comment:
       add one further test with `{"if_exists": "replace"}` would be great

##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -168,6 +169,23 @@ def test_create_table_from_csv_append() -> None:
         )
 
 
+@mock.patch(
+    "superset.db_engine_specs.hive.config",
+    {**app.config, "CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC": lambda *args: True},
+)
+@mock.patch("superset.db_engine_specs.hive.g", spec={})
+@mock.patch("tableschema.Table")
+def test_create_table_from_csv_if_exists_fail(mock_table, mock_g):

Review comment:
       Too increase coverage, add one test with `Table.schema` also to cover: https://github.com/apache/superset/blob/master/superset/db_engine_specs/hive.py#L235




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org