uranusjr commented on code in PR #75707:
URL: https://github.com/apache/airflow/pull/75707#discussion_r4385342283
##########
task-sdk/tests/task_sdk/coordinators/test_subprocess.py:
##########
@@ -560,10 +561,30 @@ def
test_context_manager_closes_tracked_socket_on_exit(self):
sock.close.assert_called_once()
def test_context_manager_terminates_tracked_popen_on_exit(self):
+ """A failed startup signals the runtime's whole process group, not
just its pid."""
proc = MagicMock(spec=subprocess.Popen)
- with _ResourceTracker(timeout=0.1) as tracker:
+ proc.pid = 6767
+ with (
+ patch("os.getpgid", side_effect=lambda pid: 6767 if pid == 6767
else os.getpid()),
+ patch("os.killpg") as mock_killpg,
+ _ResourceTracker(timeout=0.1) as tracker,
+ ):
Review Comment:
There are two styles in added tests; some fake the supervisor's group as
`os.getpid()`, but some use the real `os.getpgid(0)`. Pick one and extract the
logic into a fixture.
--
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]
