From cbe6b4a754bd59d34e74a15afcd650bcc0f8699f Mon Sep 17 00:00:00 2001 From: Jericho Tolentino <68654047+jericht@users.noreply.github.com> Date: Wed, 22 May 2024 22:14:38 +0000 Subject: [PATCH] fix windows tests Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com> --- .../_background/backend_runner.py | 6 ++ .../_background/frontend_runner.py | 84 ++++++++++++++----- .../adaptor_runtime/_background/loaders.py | 4 +- .../adaptor_runtime/_utils/_secure_open.py | 6 +- .../integ/background/test_background_mode.py | 19 +++-- .../unit/background/test_frontend_runner.py | 77 +++++++++-------- .../unit/background/test_loaders.py | 6 +- .../adaptor_runtime/unit/test_entrypoint.py | 4 +- 8 files changed, 140 insertions(+), 66 deletions(-) diff --git a/src/openjd/adaptor_runtime/_background/backend_runner.py b/src/openjd/adaptor_runtime/_background/backend_runner.py index aa46f16..5d4e16e 100644 --- a/src/openjd/adaptor_runtime/_background/backend_runner.py +++ b/src/openjd/adaptor_runtime/_background/backend_runner.py @@ -8,6 +8,7 @@ import signal from pathlib import Path from threading import Thread, Event +import traceback from types import FrameType from typing import Callable, List, Optional, Union @@ -126,6 +127,11 @@ def run(self, *, on_connection_file_written: List[Callable[[], None]] | None = N _logger.info("Shutting down server...") shutdown_event.set() raise + except Exception as e: + _logger.critical(f"Unexpected error occurred when writing to connection file: {e}") + _logger.critical(traceback.format_exc()) + _logger.info("Shutting down server") + shutdown_event.set() else: if on_connection_file_written: callbacks = list(on_connection_file_written) diff --git a/src/openjd/adaptor_runtime/_background/frontend_runner.py b/src/openjd/adaptor_runtime/_background/frontend_runner.py index 953c506..b4009a7 100644 --- a/src/openjd/adaptor_runtime/_background/frontend_runner.py +++ b/src/openjd/adaptor_runtime/_background/frontend_runner.py @@ -18,7 +18,7 @@ from threading import Event from types import FrameType from types import ModuleType -from typing import Optional, Dict +from typing import Optional, Callable, Dict from .._osname import OSName from ..process._logging import _ADAPTOR_OUTPUT_LEVEL @@ -169,7 +169,7 @@ def init( # Wait for backend process to create connection file try: - _wait_for_file(str(connection_file_path), timeout_s=5) + _wait_for_connection_file(str(connection_file_path), max_retries=5, interval_s=1) except TimeoutError: _logger.error( "Backend process failed to write connection file in time at: " @@ -414,38 +414,84 @@ def _sigint_handler(self, signum: int, frame: Optional[FrameType]) -> None: self.cancel() -def _wait_for_file(filepath: str, timeout_s: float, interval_s: float = 1) -> None: +def _wait_for_connection_file( + filepath: str, max_retries: int, interval_s: float = 1 +) -> ConnectionSettings: """ - Waits for a file at the specified path to exist and to be openable. + Waits for a connection file at the specified path to exist, be openable, and have connection settings. Args: filepath (str): The file path to check. - timeout_s (float): The max duration to wait before timing out, in seconds. + max_retries (int): The max number of retries before timing out. interval_s (float, optional): The interval between checks, in seconds. Default is 0.01s. Raises: TimeoutError: Raised when the file does not exist after timeout_s seconds. """ + wait_for( + description=f"File '{filepath}' to exist", + predicate=lambda: os.path.exists(filepath), + interval_s=interval_s, + max_retries=max_retries, + ) - def _wait(): - if time.time() - start < timeout_s: - time.sleep(interval_s) - else: - raise TimeoutError(f"Timed out after {timeout_s}s waiting for file at {filepath}") - - start = time.time() - while not os.path.exists(filepath): - _wait() + # Wait before opening to give the backend time to open it first + time.sleep(interval_s) - while True: - # Wait before opening to give the backend time to open it first - _wait() + def file_is_openable() -> bool: try: open(filepath, mode="r").close() - break except IOError: # File is not available yet - pass + return False + else: + return True + + wait_for( + description=f"File '{filepath}' to be openable", + predicate=file_is_openable, + interval_s=interval_s, + max_retries=max_retries, + ) + + def connection_file_loadable() -> bool: + try: + ConnectionSettingsFileLoader(Path(filepath)).load() + except Exception: + return False + else: + return True + + wait_for( + description=f"File '{filepath}' to have valid ConnectionSettings", + predicate=connection_file_loadable, + interval_s=interval_s, + max_retries=max_retries, + ) + + return ConnectionSettingsFileLoader(Path(filepath)).load() + + +def wait_for( + *, + description: str, + predicate: Callable[[], bool], + interval_s: float, + max_retries: int | None = None, +) -> None: + if max_retries is not None: + assert max_retries >= 0, "max_retries must be a non-negative integer" + assert interval_s > 0, "interval_s must be a positive number" + + _logger.info(f"Waiting for {description}") + retry_count = 0 + while not predicate(): + if max_retries is not None and retry_count >= max_retries: + raise TimeoutError(f"Timed out waiting for {description}") + + _logger.info(f"Retrying in {interval_s}s...") + retry_count += 1 + time.sleep(interval_s) class AdaptorFailedException(Exception): diff --git a/src/openjd/adaptor_runtime/_background/loaders.py b/src/openjd/adaptor_runtime/_background/loaders.py index 62892d0..e258a29 100644 --- a/src/openjd/adaptor_runtime/_background/loaders.py +++ b/src/openjd/adaptor_runtime/_background/loaders.py @@ -38,11 +38,11 @@ def load(self) -> ConnectionSettings: with open(self.file_path) as conn_file: loaded_settings = json.load(conn_file) except OSError as e: - errmsg = f"Failed to open connection file: {e}" + errmsg = f"Failed to open connection file '{self.file_path}': {e}" _logger.error(errmsg) raise ConnectionSettingsLoadingError(errmsg) from e except json.JSONDecodeError as e: - errmsg = f"Failed to decode connection file: {e}" + errmsg = f"Failed to decode connection file '{self.file_path}': {e}" _logger.error(errmsg) raise ConnectionSettingsLoadingError(errmsg) from e return DataclassMapper(ConnectionSettings).map(loaded_settings) diff --git a/src/openjd/adaptor_runtime/_utils/_secure_open.py b/src/openjd/adaptor_runtime/_utils/_secure_open.py index a44d246..47269de 100644 --- a/src/openjd/adaptor_runtime/_utils/_secure_open.py +++ b/src/openjd/adaptor_runtime/_utils/_secure_open.py @@ -81,7 +81,7 @@ def get_file_owner_in_windows(filepath: "StrOrBytesPath") -> str: # pragma: is- Returns: str: A string in the format 'DOMAIN\\Username' representing the file's owner. """ - sd = win32security.GetFileSecurity(filepath, win32security.OWNER_SECURITY_INFORMATION) + sd = win32security.GetFileSecurity(str(filepath), win32security.OWNER_SECURITY_INFORMATION) owner_sid = sd.GetSecurityDescriptorOwner() name, domain, _ = win32security.LookupAccountSid(None, owner_sid) return f"{domain}\\{name}" @@ -108,13 +108,13 @@ def set_file_permissions_in_windows(filepath: "StrOrBytesPath") -> None: # prag dacl.AddAccessAllowedAce(win32security.ACL_REVISION, win32con.DELETE, user_sid) # Apply the DACL to the file - sd = win32security.GetFileSecurity(filepath, win32security.DACL_SECURITY_INFORMATION) + sd = win32security.GetFileSecurity(str(filepath), win32security.DACL_SECURITY_INFORMATION) sd.SetSecurityDescriptorDacl( 1, # A flag that indicates the presence of a DACL in the security descriptor. dacl, # An ACL structure that specifies the DACL for the security descriptor. 0, # Don't retrieve the default DACL ) - win32security.SetFileSecurity(filepath, win32security.DACL_SECURITY_INFORMATION, sd) + win32security.SetFileSecurity(str(filepath), win32security.DACL_SECURITY_INFORMATION, sd) def _get_flags_from_mode_str(open_mode: str) -> int: diff --git a/test/openjd/adaptor_runtime/integ/background/test_background_mode.py b/test/openjd/adaptor_runtime/integ/background/test_background_mode.py index 0794f56..974e736 100644 --- a/test/openjd/adaptor_runtime/integ/background/test_background_mode.py +++ b/test/openjd/adaptor_runtime/integ/background/test_background_mode.py @@ -21,7 +21,10 @@ FrontendRunner, HTTPError, ) -from openjd.adaptor_runtime._background.loaders import ConnectionSettingsFileLoader +from openjd.adaptor_runtime._background.loaders import ( + ConnectionSettingsLoadingError, + ConnectionSettingsFileLoader, +) from openjd.adaptor_runtime._osname import OSName mod_path = (Path(__file__).parent.parent).resolve() @@ -77,7 +80,6 @@ def initialized_setup( adaptor_module=sys.modules[AdaptorExample.__module__], connection_file_path=connection_file_path, ) - conn_settings = ConnectionSettingsFileLoader(connection_file_path).load() match = re.search("Started backend process. PID: ([0-9]+)", caplog.text) assert match is not None @@ -96,9 +98,16 @@ def initialized_setup( # Once all handles are closed, the system automatically cleans up the named pipe. if OSName.is_posix(): try: - os.remove(conn_settings.socket) - except FileNotFoundError: - pass # Already deleted + conn_settings = ConnectionSettingsFileLoader(connection_file_path).load() + except ConnectionSettingsLoadingError as e: + print( + f"Failed to load connection settings, socket file cleanup will be skipped: {e}" + ) + else: + try: + os.remove(conn_settings.socket) + except FileNotFoundError: + pass # Already deleted def test_init( self, diff --git a/test/openjd/adaptor_runtime/unit/background/test_frontend_runner.py b/test/openjd/adaptor_runtime/unit/background/test_frontend_runner.py index a673cb1..19e5474 100644 --- a/test/openjd/adaptor_runtime/unit/background/test_frontend_runner.py +++ b/test/openjd/adaptor_runtime/unit/background/test_frontend_runner.py @@ -24,7 +24,7 @@ AdaptorFailedException, FrontendRunner, HTTPError, - _wait_for_file, + _wait_for_connection_file, ) from openjd.adaptor_runtime._background.model import ( AdaptorStatus, @@ -90,8 +90,8 @@ def mock_Popen(self) -> Generator[MagicMock, None, None]: yield m @pytest.fixture(autouse=True) - def mock_wait_for_file(self) -> Generator[MagicMock, None, None]: - with patch.object(frontend_runner, "_wait_for_file") as m: + def mock_wait_for_connection_file(self) -> Generator[MagicMock, None, None]: + with patch.object(frontend_runner, "_wait_for_connection_file") as m: yield m @pytest.fixture(autouse=True) @@ -125,7 +125,7 @@ def test_initializes_backend_process( self, mock_path_exists: MagicMock, mock_Popen: MagicMock, - mock_wait_for_file: MagicMock, + mock_wait_for_connection_file: MagicMock, mock_heartbeat: MagicMock, mock_sys_executable: MagicMock, mock_sys_argv: MagicMock, @@ -212,7 +212,11 @@ def test_initializes_backend_process( stdout=open_mock.return_value, stderr=open_mock.return_value, ) - mock_wait_for_file.assert_called_once_with(str(connection_file_path), timeout_s=5) + mock_wait_for_connection_file.assert_called_once_with( + str(connection_file_path), + max_retries=5, + interval_s=1, + ) mock_heartbeat.assert_called_once() def test_raises_when_adaptor_module_not_package(self): @@ -251,8 +255,9 @@ def test_raises_when_connection_file_exists( # THEN assert raised_err.match( - "Cannot init a new backend process with an existing connection file at: " - + str(conn_file_path) + re.escape( + f"Cannot init a new backend process with an existing connection file at: {conn_file_path}" + ) ) mock_path_exists.assert_called_once_with() @@ -295,13 +300,13 @@ def test_raises_when_connection_file_wait_times_out( self, mock_path_exists: MagicMock, mock_Popen: MagicMock, - mock_wait_for_file: MagicMock, + mock_wait_for_connection_file: MagicMock, caplog: pytest.LogCaptureFixture, ): # GIVEN caplog.set_level("DEBUG") err = TimeoutError() - mock_wait_for_file.side_effect = err + mock_wait_for_connection_file.side_effect = err mock_path_exists.return_value = False pid = 123 mock_Popen.return_value.pid = pid @@ -329,7 +334,11 @@ def test_raises_when_connection_file_wait_times_out( ) mock_path_exists.assert_called_once_with() mock_Popen.assert_called_once() - mock_wait_for_file.assert_called_once_with(str(conn_file_path), timeout_s=5) + mock_wait_for_connection_file.assert_called_once_with( + str(conn_file_path), + max_retries=5, + interval_s=1, + ) class TestHeartbeat: """ @@ -741,17 +750,24 @@ def mock_read_from_pipe(self, mock_response: MagicMock) -> Generator[MagicMock, mock_read_from_pipe.return_value = mock_response yield mock_read_from_pipe + @pytest.fixture + def connection_settings(self) -> ConnectionSettings: + return ConnectionSettings("\\\\.\\pipe") + + @pytest.fixture + def ruynner(self, connection_settings: ConnectionSettings) -> FrontendRunner: + return FrontendRunner(connection_settings=connection_settings) + def test_sends_request( self, mock_read_from_pipe: MagicMock, mock_response: str, + runner: FrontendRunner, ): # GIVEN method = "GET" path = "/path" - runner = FrontendRunner() - # WHEN with patch.object( frontend_runner.NamedPipeHelper, "write_to_pipe" @@ -772,6 +788,7 @@ def test_raises_when_request_fails( self, mock_read_from_pipe: MagicMock, mock_response: str, + runner: FrontendRunner, caplog: pytest.LogCaptureFixture, ): # GIVEN @@ -781,7 +798,6 @@ def test_raises_when_request_fails( mock_read_from_pipe.side_effect = error_instance method = "GET" path = "/path" - runner = FrontendRunner() # WHEN with patch.object( @@ -804,12 +820,12 @@ def test_raises_when_request_fails( def test_raises_when_error_response_received( self, mock_response: str, + runner: FrontendRunner, caplog: pytest.LogCaptureFixture, ): # GIVEN method = "GET" path = "/path" - runner = FrontendRunner() # WHEN with patch.object( @@ -840,13 +856,13 @@ def test_formats_query_string( self, mock_read_from_pipe, mock_response: str, + runner: FrontendRunner, caplog: pytest.LogCaptureFixture, ): # GIVEN method = "GET" path = "/path" params = {"first param": 1, "second_param": ["one", "two three"]} - runner = FrontendRunner() # WHEN with patch.object( @@ -869,13 +885,13 @@ def test_sends_body( self, mock_read_from_pipe, mock_response: str, + runner: FrontendRunner, caplog: pytest.LogCaptureFixture, ): # GIVEN method = "GET" path = "/path" json_body = {"the": "body"} - runner = FrontendRunner() # WHEN with patch.object( @@ -916,62 +932,57 @@ def test_hook(self, signal_mock: MagicMock, cancel_mock: MagicMock) -> None: cancel_mock.assert_called_once() -class TestWaitForFile: +class TestWaitForConnectionFile: """ - Tests for the _wait_for_file method + Tests for the _wait_for_connection_file method """ + @patch.object(frontend_runner.ConnectionSettingsFileLoader, "load") @patch.object(frontend_runner, "open") - @patch.object(frontend_runner.time, "time") @patch.object(frontend_runner.time, "sleep") @patch.object(frontend_runner.os.path, "exists") def test_waits_for_file( self, mock_exists: MagicMock, mock_sleep: MagicMock, - mock_time: MagicMock, open_mock: MagicMock, + mock_conn_file_loader_load: MagicMock, ): # GIVEN filepath = "/path" - timeout = sys.float_info.max + max_retries = 9999 interval = 0.01 - mock_time.side_effect = [1, 2, 3, 4] mock_exists.side_effect = [False, True] err = IOError() open_mock.side_effect = [err, MagicMock()] + mock_conn_file_loader_load.return_value = ConnectionSettings("/server") # WHEN - _wait_for_file(filepath, timeout, interval) + _wait_for_connection_file(filepath, max_retries, interval) # THEN - assert mock_time.call_count == 4 mock_exists.assert_has_calls([call(filepath)] * 2) mock_sleep.assert_has_calls([call(interval)] * 3) open_mock.assert_has_calls([call(filepath, mode="r")] * 2) - @patch.object(frontend_runner.time, "time") @patch.object(frontend_runner.time, "sleep") @patch.object(frontend_runner.os.path, "exists") - def test_raises_when_timeout_reached( + def test_raises_when_retries_reached( self, mock_exists: MagicMock, mock_sleep: MagicMock, - mock_time: MagicMock, ): # GIVEN filepath = "/path" - timeout = 0 + max_retries = 0 interval = 0.01 - mock_time.side_effect = [1, 2] - mock_exists.side_effect = [False] + mock_exists.return_value = False # WHEN with pytest.raises(TimeoutError) as raised_err: - _wait_for_file(filepath, timeout, interval) + _wait_for_connection_file(filepath, max_retries, interval) # THEN - assert raised_err.match(f"Timed out after {timeout}s waiting for file at {filepath}") - assert mock_time.call_count == 2 + assert raised_err.match(f"Timed out waiting for File '{filepath}' to exist") mock_exists.assert_called_once_with(filepath) mock_sleep.assert_not_called() diff --git a/test/openjd/adaptor_runtime/unit/background/test_loaders.py b/test/openjd/adaptor_runtime/unit/background/test_loaders.py index 734da28..513221b 100644 --- a/test/openjd/adaptor_runtime/unit/background/test_loaders.py +++ b/test/openjd/adaptor_runtime/unit/background/test_loaders.py @@ -65,6 +65,7 @@ def test_raises_when_file_open_fails( self, open_mock: MagicMock, loader: ConnectionSettingsFileLoader, + connection_file_path: pathlib.Path, caplog: pytest.LogCaptureFixture, ): # GIVEN @@ -76,11 +77,12 @@ def test_raises_when_file_open_fails( loader.load() # THEN - assert "Failed to open connection file: " in caplog.text + assert f"Failed to open connection file '{connection_file_path}': " in caplog.text def test_raises_when_json_decode_fails( self, loader: ConnectionSettingsFileLoader, + connection_file_path: pathlib.Path, caplog: pytest.LogCaptureFixture, ): # GIVEN @@ -92,7 +94,7 @@ def test_raises_when_json_decode_fails( loader.load() # THEN - assert "Failed to decode connection file: " in caplog.text + assert f"Failed to decode connection file '{connection_file_path}': " in caplog.text class TestConnectionSettingsEnvLoader: diff --git a/test/openjd/adaptor_runtime/unit/test_entrypoint.py b/test/openjd/adaptor_runtime/unit/test_entrypoint.py index 81bd717..80de3dc 100644 --- a/test/openjd/adaptor_runtime/unit/test_entrypoint.py +++ b/test/openjd/adaptor_runtime/unit/test_entrypoint.py @@ -496,7 +496,7 @@ def test_runs_background_serve( ) mock_init.assert_called_once_with( mock_adaptor_runner.return_value, - connection_file_path=conn_file, + connection_file_path=conn_file.resolve(), log_buffer=mock_log_buffer.return_value, ) mock_run.assert_called_once() @@ -609,7 +609,7 @@ def test_runs_background_start( mock_magic_init.assert_called_once_with() mock_init.assert_called_once_with( adaptor_module=mock_adaptor_module, - connection_file_path=conn_file, + connection_file_path=conn_file.resolve(), init_data={}, path_mapping_data={}, reentry_exe=reentry_exe,