rjgoyln commented on code in PR #72817:
URL: https://github.com/apache/airflow/pull/72817#discussion_r3970643517
##########
providers/celery/tests/unit/celery/cli/test_celery_command.py:
##########
@@ -73,23 +74,38 @@ def setup_class(cls):
importlib.reload(cli_parser)
cls.parser = cli_parser.get_parser()
- @mock.patch("airflow.providers.celery.cli.celery_command.setup_locations")
- @mock.patch("airflow.providers.celery.cli.celery_command.psutil.Process")
- def test_if_right_pid_is_read(self, mock_process, mock_setup_locations,
tmp_path):
+ @pytest.mark.parametrize(
+ ("process_side_effect", "terminate_side_effect",
"expected_terminate_calls"),
+ [
+ pytest.param(None, None, 1, id="worker-running"),
+ pytest.param(psutil.NoSuchProcess(123), None, 0,
id="stale-pid-file"),
+ pytest.param(None, psutil.NoSuchProcess(123), 1,
id="worker-exits-before-terminate"),
+ ],
+ )
+ @mock.patch("airflow.providers.celery.cli.celery_command.setup_locations",
autospec=True)
+ @mock.patch("airflow.providers.celery.cli.celery_command.psutil.Process",
autospec=True)
+ def test_if_right_pid_is_read(
Review Comment:
The parametrization has outgrown the name — this now covers three process
states rather than whether the right PID is read.
```suggestion
def test_stop_worker_handles_missing_process(
```
##########
providers/celery/src/airflow/providers/celery/cli/celery_command.py:
##########
@@ -383,8 +383,13 @@ def stop_worker(args):
# Send SIGTERM
if pid:
- worker_process = psutil.Process(pid)
- worker_process.terminate()
+ try:
+ worker_process = psutil.Process(pid)
Review Comment:
Out of scope for this PR, but the sibling case is still open: if the OS has
recycled the stale PID, psutil.Process(pid) succeeds and terminate() may signal
an unrelated process. `check_if_pidfile_process_is_running` has the same gap,
so nothing here regresses — just flagging that comparing
`worker_process.name()`/`cmdline()` before signalling is the natural follow-up.
##########
providers/celery/tests/unit/celery/cli/test_celery_command.py:
##########
@@ -73,23 +74,38 @@ def setup_class(cls):
importlib.reload(cli_parser)
cls.parser = cli_parser.get_parser()
- @mock.patch("airflow.providers.celery.cli.celery_command.setup_locations")
- @mock.patch("airflow.providers.celery.cli.celery_command.psutil.Process")
- def test_if_right_pid_is_read(self, mock_process, mock_setup_locations,
tmp_path):
+ @pytest.mark.parametrize(
+ ("process_side_effect", "terminate_side_effect",
"expected_terminate_calls"),
+ [
+ pytest.param(None, None, 1, id="worker-running"),
+ pytest.param(psutil.NoSuchProcess(123), None, 0,
id="stale-pid-file"),
+ pytest.param(None, psutil.NoSuchProcess(123), 1,
id="worker-exits-before-terminate"),
+ ],
+ )
+ @mock.patch("airflow.providers.celery.cli.celery_command.setup_locations",
autospec=True)
+ @mock.patch("airflow.providers.celery.cli.celery_command.psutil.Process",
autospec=True)
+ def test_if_right_pid_is_read(
+ self,
+ mock_process,
+ mock_setup_locations,
+ process_side_effect,
+ terminate_side_effect,
+ expected_terminate_calls,
+ tmp_path,
+ ):
args = self.parser.parse_args(["celery", "stop"])
- pid = "123"
+ pid = 123
path = tmp_path / "testfile"
- # Create pid file
- path.write_text(pid)
- # Setup mock
+ path.write_text(str(pid))
mock_setup_locations.return_value = (os.fspath(path), None, None, None)
+ mock_process.side_effect = process_side_effect
+ mock_process.return_value.terminate.side_effect = terminate_side_effect
- # Calling stop_worker should delete the temporary pid file
celery_command.stop_worker(args)
- # Check if works as expected
+
assert not path.exists()
- mock_process.assert_called_once_with(int(pid))
- mock_process.return_value.terminate.assert_called_once_with()
+ mock_process.assert_called_once_with(pid)
+ assert mock_process.return_value.terminate.call_count ==
expected_terminate_calls
Review Comment:
This drops the old `assert_called_once_with()`, which also pinned that
`terminate` is called with no arguments; `call_count` alone would still pass if
it were called with something. Comparing the call list keeps that check and
stays under the 110-char limit.
```suggestion
assert mock_process.return_value.terminate.call_args_list ==
[mock.call()] * expected_terminate_calls
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
