[GitHub] [airflow] SamWheating commented on a change in pull request #17347: Handle and log exceptions raised during task callback

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

SamWheating commented on a change in pull request #17347:
URL: https://github.com/apache/airflow/pull/17347#discussion_r681182088



##########
File path: tests/models/test_taskinstance.py
##########
@@ -1743,6 +1743,32 @@ def on_execute_callable(context):
         ti.refresh_from_db()
         assert ti.state == State.SUCCESS
 
+    def test_success_callback_handles_and_logs_exception(self):
+        called = completed = False
+
+        def on_success_callable(context):
+            nonlocal called, completed
+            called = True
+            raise KeyError
+            completed = True
+
+        dag = DAG(
+            'test_success_callback_handles_exception',
+            start_date=DEFAULT_DATE,
+            end_date=DEFAULT_DATE + datetime.timedelta(days=10),
+        )
+        task = DummyOperator(
+            task_id='op', email='test@test.test', on_success_callback=on_success_callable, dag=dag
+        )

Review comment:
       Oh, super neat! However, it looks like TestTaskInstance is a subclass of `unitest.Testcase`, which [doesn't seem to support all uses of pytest fixtures](https://docs.pytest.org/en/6.2.x/unittest.html#pytest-features-in-unittest-testcase-subclasses).  Is there a reason why some test classes are subclasses of unittest.testcase, and others aren't? 
   
   Anyways, I've refactored this test to include all three callback types, but have been having some trouble getting the `dag_maker` fixture to work. Are you able to give me a hand with this?




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