Yicong-Huang commented on code in PR #4704: URL: https://github.com/apache/texera/pull/4704#discussion_r3177389418
########## amber/src/main/python/core/runnables/test_heartbeat.py: ########## @@ -0,0 +1,97 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import socket +from threading import Event +from unittest.mock import patch, MagicMock + +import pytest + +from core.runnables.heartbeat import Heartbeat + + +def make_heartbeat(host="localhost", port=12345, interval=0.05, event=None): + return Heartbeat(host, port, interval, event or Event()) + + +class TestHeartbeatInit: + def test_parses_host_and_port_from_grpc_tcp_url(self): + hb = make_heartbeat(host="example.test", port=9090) + assert hb._parsed_server_host == "example.test" + assert hb._parsed_server_port == 9090 + + def test_records_interval_and_stop_event_references(self): + event = Event() + hb = make_heartbeat(interval=2.5, event=event) + assert hb._interval == 2.5 + assert hb._stop_event is event + + def test_captures_original_parent_pid_at_construction_time(self): + with patch("core.runnables.heartbeat.os.getppid", return_value=4242): + hb = make_heartbeat() + assert hb._original_parent_pid == 4242 + + def test_supports_ipv6_host_in_bracketed_form(self): + hb = make_heartbeat(host="[::1]", port=9090) + assert hb._parsed_server_host == "::1" + assert hb._parsed_server_port == 9090 + + +class TestCheckHeartbeat: + def test_returns_true_when_socket_connects(self): + hb = make_heartbeat(host="h", port=1) + fake_sock = MagicMock() + with patch( + "core.runnables.heartbeat.socket.create_connection", + return_value=fake_sock, + ) as mock_connect: + assert hb._check_heartbeat() is True + mock_connect.assert_called_once_with(("h", 1), timeout=1) + fake_sock.close.assert_called_once() + Review Comment: Added `test_returns_false_when_socket_close_raises` in 97e1022caf: it stubs `socket.create_connection` to return a mock whose `close()` raises `OSError`, and asserts `_check_heartbeat()` returns `False`. The bare `except Exception` path is now pinned. ########## amber/src/main/python/core/runnables/test_heartbeat.py: ########## @@ -0,0 +1,97 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import socket +from threading import Event +from unittest.mock import patch, MagicMock + +import pytest + +from core.runnables.heartbeat import Heartbeat + + +def make_heartbeat(host="localhost", port=12345, interval=0.05, event=None): + return Heartbeat(host, port, interval, event or Event()) + + +class TestHeartbeatInit: + def test_parses_host_and_port_from_grpc_tcp_url(self): + hb = make_heartbeat(host="example.test", port=9090) + assert hb._parsed_server_host == "example.test" + assert hb._parsed_server_port == 9090 + + def test_records_interval_and_stop_event_references(self): + event = Event() + hb = make_heartbeat(interval=2.5, event=event) + assert hb._interval == 2.5 + assert hb._stop_event is event + + def test_captures_original_parent_pid_at_construction_time(self): + with patch("core.runnables.heartbeat.os.getppid", return_value=4242): + hb = make_heartbeat() + assert hb._original_parent_pid == 4242 + + def test_supports_ipv6_host_in_bracketed_form(self): + hb = make_heartbeat(host="[::1]", port=9090) + assert hb._parsed_server_host == "::1" + assert hb._parsed_server_port == 9090 + + +class TestCheckHeartbeat: + def test_returns_true_when_socket_connects(self): + hb = make_heartbeat(host="h", port=1) + fake_sock = MagicMock() + with patch( + "core.runnables.heartbeat.socket.create_connection", + return_value=fake_sock, + ) as mock_connect: + assert hb._check_heartbeat() is True + mock_connect.assert_called_once_with(("h", 1), timeout=1) + fake_sock.close.assert_called_once() + + def test_returns_false_when_socket_connection_raises(self): + hb = make_heartbeat() + with patch( + "core.runnables.heartbeat.socket.create_connection", + side_effect=ConnectionRefusedError("nope"), + ): + assert hb._check_heartbeat() is False + + def test_returns_false_when_socket_connection_times_out(self): + hb = make_heartbeat() + with patch( + "core.runnables.heartbeat.socket.create_connection", + side_effect=socket.timeout("slow"), + ): + assert hb._check_heartbeat() is False + + +class TestRunEarlyExit: + def test_returns_immediately_when_stop_event_is_already_set(self): + event = Event() + event.set() + hb = make_heartbeat(interval=10.0, event=event) + # Without an early exit this would block on wait(timeout=10). Review Comment: Replaced in 97e1022caf with a comment that describes the actual mechanism: `Event.wait(timeout=10)` returns `True` because the event is already set, so `while not self._stop_event.wait(...)` short-circuits before the loop body runs. ########## amber/src/main/python/core/runnables/test_heartbeat.py: ########## @@ -0,0 +1,97 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import socket +from threading import Event +from unittest.mock import patch, MagicMock + +import pytest + +from core.runnables.heartbeat import Heartbeat + + +def make_heartbeat(host="localhost", port=12345, interval=0.05, event=None): + return Heartbeat(host, port, interval, event or Event()) + + +class TestHeartbeatInit: + def test_parses_host_and_port_from_grpc_tcp_url(self): + hb = make_heartbeat(host="example.test", port=9090) + assert hb._parsed_server_host == "example.test" + assert hb._parsed_server_port == 9090 + + def test_records_interval_and_stop_event_references(self): + event = Event() + hb = make_heartbeat(interval=2.5, event=event) + assert hb._interval == 2.5 + assert hb._stop_event is event + + def test_captures_original_parent_pid_at_construction_time(self): + with patch("core.runnables.heartbeat.os.getppid", return_value=4242): + hb = make_heartbeat() + assert hb._original_parent_pid == 4242 + + def test_supports_ipv6_host_in_bracketed_form(self): + hb = make_heartbeat(host="[::1]", port=9090) + assert hb._parsed_server_host == "::1" + assert hb._parsed_server_port == 9090 + + +class TestCheckHeartbeat: + def test_returns_true_when_socket_connects(self): + hb = make_heartbeat(host="h", port=1) + fake_sock = MagicMock() + with patch( + "core.runnables.heartbeat.socket.create_connection", + return_value=fake_sock, + ) as mock_connect: + assert hb._check_heartbeat() is True + mock_connect.assert_called_once_with(("h", 1), timeout=1) + fake_sock.close.assert_called_once() + + def test_returns_false_when_socket_connection_raises(self): + hb = make_heartbeat() + with patch( + "core.runnables.heartbeat.socket.create_connection", + side_effect=ConnectionRefusedError("nope"), + ): + assert hb._check_heartbeat() is False + + def test_returns_false_when_socket_connection_times_out(self): + hb = make_heartbeat() + with patch( + "core.runnables.heartbeat.socket.create_connection", + side_effect=socket.timeout("slow"), + ): + assert hb._check_heartbeat() is False + + +class TestRunEarlyExit: + def test_returns_immediately_when_stop_event_is_already_set(self): + event = Event() + event.set() + hb = make_heartbeat(interval=10.0, event=event) + # Without an early exit this would block on wait(timeout=10). + with patch.object(hb, "_check_heartbeat") as mock_check: + hb.run() Review Comment: Added `@pytest.mark.timeout(2)` in 97e1022caf, matching the `test_network_*.py` style. A regression that re-enters the loop or blocks on `wait()` will now fail fast instead of hanging 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
