You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/08/12 14:46:49 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
ephraimbuddy opened a new pull request #17581:
URL: https://github.com/apache/airflow/pull/17581
This PR removes the use of multiprocessing in TestLocalTaskJob and improves
the test to be more reliable
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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
[GitHub] [airflow] uranusjr commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-900156495
Note: The MSSQL job was red since it’s killed midway, so I think it’s safe to assume all tests are passing right now.
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r691161381
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
So since we’ve established `return_code` is called one more time after it exits, would `mock_return_code.side_effect = [None, 0, 0]` work, or can `return_code` be called more than one extra time?
--
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
[GitHub] [airflow] ephraimbuddy closed pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #17581:
URL: https://github.com/apache/airflow/pull/17581
--
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
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r690353515
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
I have found the culprit, here's a PR https://github.com/apache/airflow/pull/17654
--
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
[GitHub] [airflow] ephraimbuddy closed pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #17581:
URL: https://github.com/apache/airflow/pull/17581
--
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
[GitHub] [airflow] potiuk commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-901155023
🤞
--
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
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r691172919
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
It works with any extra value, I used None and added some explanation. Take a look
--
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
[GitHub] [airflow] ashb commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r687800528
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -273,42 +274,41 @@ def test_heartbeat_failed_fast(self):
delta = (time2 - time1).total_seconds()
assert abs(delta - job.heartrate) < 0.5
- def test_mark_success_no_kill(self):
+ @patch.object(StandardTaskRunner, 'return_code')
+ def test_mark_success_no_kill(self, mock_return_code, caplog, dag_maker):
"""
Test that ensures that mark_success in the UI doesn't cause
the task to fail, and that the task exits
"""
- dag = self.dagbag.dags.get('test_mark_success')
- task = dag.get_task('task1')
-
session = settings.Session()
- dag.clear()
- dag.create_dagrun(
- run_id="test",
- state=State.RUNNING,
- execution_date=DEFAULT_DATE,
- start_date=DEFAULT_DATE,
- session=session,
- )
- ti = TaskInstance(task=task, execution_date=DEFAULT_DATE)
+ def task_function(ti):
+ assert ti.state == State.RUNNING
+ ti.state = State.SUCCESS
+ session.merge(ti)
+ session.commit()
Review comment:
We probably need a comment here to say what we are "simulating".
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -273,42 +274,41 @@ def test_heartbeat_failed_fast(self):
delta = (time2 - time1).total_seconds()
assert abs(delta - job.heartrate) < 0.5
- def test_mark_success_no_kill(self):
+ @patch.object(StandardTaskRunner, 'return_code')
+ def test_mark_success_no_kill(self, mock_return_code, caplog, dag_maker):
"""
Test that ensures that mark_success in the UI doesn't cause
the task to fail, and that the task exits
"""
- dag = self.dagbag.dags.get('test_mark_success')
- task = dag.get_task('task1')
-
session = settings.Session()
- dag.clear()
- dag.create_dagrun(
- run_id="test",
- state=State.RUNNING,
- execution_date=DEFAULT_DATE,
- start_date=DEFAULT_DATE,
- session=session,
- )
- ti = TaskInstance(task=task, execution_date=DEFAULT_DATE)
+ def task_function(ti):
+ assert ti.state == State.RUNNING
+ ti.state = State.SUCCESS
+ session.merge(ti)
+ session.commit()
+
+ with dag_maker('test_mark_success'):
+ task1 = PythonOperator(task_id="task1", python_callable=task_function)
+ dag_maker.create_dagrun()
+
+ ti = TaskInstance(task=task1, execution_date=DEFAULT_DATE)
ti.refresh_from_db()
job1 = LocalTaskJob(task_instance=ti, ignore_ti_state=True)
- settings.engine.dispose()
- process = multiprocessing.Process(target=job1.run)
- process.start()
- for _ in range(0, 50):
- if ti.state == State.RUNNING:
- break
- time.sleep(0.1)
- ti.refresh_from_db()
- assert State.RUNNING == ti.state
- ti.state = State.SUCCESS
- session.merge(ti)
- session.commit()
- process.join(timeout=10)
+
+ # The return code when we mark success in the UI is None
+ def dummy_return_code(*args, **kwargs):
+ return None
+
+ mock_return_code.side_effect = dummy_return_code
Review comment:
I think you can do this
```suggestion
mock_return_code.return_value = None
```
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -587,23 +584,24 @@ def task_function(ti):
ti = TaskInstance(task=task, execution_date=DEFAULT_DATE)
ti.refresh_from_db()
job1 = LocalTaskJob(task_instance=ti, ignore_ti_state=True, executor=SequentialExecutor())
+ job1.task_runner = StandardTaskRunner(job1)
+ job1.task_runner.start()
settings.engine.dispose()
- process = multiprocessing.Process(target=job1.run)
- process.start()
- time.sleep(0.3)
- process.join(timeout=10)
+ caplog.set_level(logging.INFO)
+ job1.run() # os.kill will make this run very short
Review comment:
We should probably still put a timeout here just to be defensive
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r690348897
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
> From the perspective of the original test, heartbeat should stop beating once the return code is 0 but now, it doesn't stop which is the cause of several issues we have around this.
Ohhhh… hmm. Not really sure what best to do 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
[GitHub] [airflow] uranusjr commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-900159470
There are several
```python
# This should not happen.
time.sleep(10)
```
usages in the tests. I wonder if we should add an `assert False, "This should not happen"` at their end to make the test fail explicitly.
--
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
[GitHub] [airflow] ephraimbuddy merged pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged pull request #17581:
URL: https://github.com/apache/airflow/pull/17581
--
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
[GitHub] [airflow] ashb commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-898956844
@ephraimbuddy Where are we with this PR? Is it better/fixed do you think, or does it still have problems?
--
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
[GitHub] [airflow] ephraimbuddy commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-897738711
Hmm, got a failure locally now after several runs....
--
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
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r691080761
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
Turns out this is not really heart beating more but return_code called extra time
--
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
[GitHub] [airflow] potiuk commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-901155023
🤞
--
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
[GitHub] [airflow] ephraimbuddy merged pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged pull request #17581:
URL: https://github.com/apache/airflow/pull/17581
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r691161381
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
So since we’ve established `return_code` is called one more time after it exits, would `mock_return_code.side_effect = [None, 0, 0]` work, or can `return_code` be called more than one extra time?
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
Yeah because the extra call just goes unused.
--
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
[GitHub] [airflow] ephraimbuddy commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-899631359
This is ready for review now. I marked the two flaky tests as quarantined. Others have not failed for many runs now.
@ashb
--
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
[GitHub] [airflow] potiuk commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-898913185
> Looking at your change now, I have next to _no_ idea why we were ever using multiprocessing in the first place
cannot agree more :)
--
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
[GitHub] [airflow] ephraimbuddy commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-898962043
> @ephraimbuddy Where are we with this PR? Is it better/fixed do you think, or does it still have problems?
It still has problems. Some tests still fail in CI
--
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
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r691162763
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
I will check now
--
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
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r691080761
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
Turns out this is not really heart beating more but return_code called extra time
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
I will check now
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
It works with any extra value, I used None and added some explanation. Take a look
--
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
[GitHub] [airflow] ephraimbuddy commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-900167012
> There are several
>
> ```python
> # This should not happen.
> time.sleep(10)
> ```
>
> usages in the tests. I wonder if we should add an `assert False, "This should not happen"` at their end to make the test fail explicitly.
Here, we want to sleep a bit so that the heartbeat would detect the state change. If it fails to detect the state change within the time, then the code beneath would run, which would mean the test has failed because `task_terminated_externally.value` will then be 0. Unless we want to remove `task_terminated_externally.value` from the tests, we can just sleep 10s and assert False. What do you think?
--
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
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r690233500
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
There's a problem here which I'm trying to understand. From the perspective of the original test, heartbeat should stop beating once the return code is `0` but now, it doesn't stop which is the cause of several issues we have around this.
If we return `[None, 0]`, the heartbeat will still beat for the third time. That was why I added `while return_codes`.
--
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
[GitHub] [airflow] ephraimbuddy closed pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #17581:
URL: https://github.com/apache/airflow/pull/17581
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r691184181
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
Yeah because the extra call just goes unused.
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r690217140
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
```suggestion
mock_return_code.side_effect = [None, 0]
```
[Documentation on this](https://docs.python.org/3/library/unittest.mock.html#quick-guide)
--
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
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#discussion_r690237274
##########
File path: tests/jobs/test_local_task_job.py
##########
@@ -372,16 +361,17 @@ def test_localtaskjob_maintain_heart_rate(self):
# loop in _execute()
return_codes = [None, 0]
- def multi_return_code():
- return return_codes.pop(0)
+ def multi_return_code(*args, **kwargs):
+ while return_codes:
+ return return_codes.pop(0)
time_start = time.time()
- with patch.object(StandardTaskRunner, 'start', return_value=None) as mock_start:
- with patch.object(StandardTaskRunner, 'return_code') as mock_ret_code:
- mock_ret_code.side_effect = multi_return_code
- job1.run()
- assert mock_start.call_count == 1
- assert mock_ret_code.call_count == 2
+
+ mock_return_code.side_effect = multi_return_code
Review comment:
For this, I'm experimenting `not heartbeating` whenever return_code is zero and if it works I will correct this test. Should I leave it as quarantined? @ashb @uranusjr
--
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
[GitHub] [airflow] ephraimbuddy commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-899363882
These two tests:
```
test_process_sigterm_works_with_retries
test_process_sigterm_calls_on_failure_callback
```
are still flaky. Others have succeeded for about 3 runs now
--
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
[GitHub] [airflow] uranusjr commented on pull request #17581: Remove the use of multiprocessing in TestLocalTaskJob and Improve Tests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17581:
URL: https://github.com/apache/airflow/pull/17581#issuecomment-900172063
I think an assert would be better than the `task_terminated_externally` check. It’s better to not depend on too much logic to trigger a test failure.
--
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