[GitHub] [airflow] alexkruc commented on a diff in pull request #24663: Adding generic SqlToSlackOperator

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

alexkruc commented on code in PR #24663:
URL: https://github.com/apache/airflow/pull/24663#discussion_r906771101


##########
tests/providers/presto/transfers/test_presto_to_slack.py:
##########
@@ -41,37 +41,60 @@ def _construct_operator(**kwargs):
         operator = PrestoToSlackOperator(task_id=TEST_DAG_ID, **kwargs)
         return operator
 
-    @mock.patch('airflow.providers.presto.transfers.presto_to_slack.PrestoHook')
-    @mock.patch('airflow.providers.presto.transfers.presto_to_slack.SlackWebhookHook')
-    def test_hooks_and_rendering(self, mock_slack_hook_class, mock_presto_hook_class):
+    @mock.patch('airflow.providers.slack.transfers.sql_to_slack.SlackWebhookHook')
+    def test_hooks_and_rendering_with_slack_conn(self, mock_slack_hook_class):

Review Comment:
   I left the test as is, but changed the mocking location.
   now, the test is verifying that the message that is getting built using the operator is actually triggered from the generic `sql_to_slack.SlackWebhookHook`.
   All the other tests like rendering different df names and such are in `SqlToSlackOperator` tests.. So I think that the tests are pretty minimal as is, and we can delete them and you won't lose any "important" tests, or you will miss some logic..
   My guideline here was end to end test `PrestoToSlackOperator`, and make sure that the actual call is called from `SqlToSlackOperator`.. if we remove `PrestoToSlackOperator` eventually, I see no issue here, as e2e is not important anymore :)
   
   Or am I mistaken here?



-- 
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