[GitHub] [airflow] houqp commented on a change in pull request #6553: [AIRFLOW-5902] avoid unnecessary sleep to maintain local task job heart rate

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

houqp commented on a change in pull request #6553: [AIRFLOW-5902] avoid unnecessary sleep to maintain local task job heart rate
URL: https://github.com/apache/airflow/pull/6553#discussion_r346684260
 
 

 ##########
 File path: airflow/jobs/base_job.py
 ##########
 @@ -171,17 +171,14 @@ def heartbeat(self):
             if self.state == State.SHUTDOWN:
                 self.kill()
 
-            is_unit_test = conf.getboolean('core', 'unit_test_mode')
 
 Review comment:
   This if condition was added to speed up the tests, however, there is a much better and more maintainable way to achieve the same effect, which is mock the sleep call from unit tests. By doing that, we get the following benefits:
   
   1. in unit test, we can now check and assert whether sleep has been called with the right argument
   2. avoid unnecessary dictionary lookup and if statement check at runtime
   3. avoid the anti-pattern where unit test logic is mingled with business logic. unit test should be as transparent to the actual code as possible
   
   I noticed we are adopting this pattern in 2 other places as well and I really think we should replace all of them with proper mocking in the test and keep the business code clean.

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


With regards,
Apache Git Services