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:
[email protected]
With regards,
Apache Git Services
