diff --git a/hatch.toml b/hatch.toml index d59c3d3..2f12370 100644 --- a/hatch.toml +++ b/hatch.toml @@ -6,8 +6,7 @@ pre-install-commands = [ [envs.default.scripts] sync = "pip install -r requirements-testing.txt" test = "pytest --cov-config pyproject.toml {args:test}" -test-windows = """pytest --cov-config pyproject.toml {args:} \ - test/openjd/adaptor_runtime --cov-fail-under=77""" +test-windows = "pytest --cov-config pyproject.toml {args:test} --cov-fail-under=90" typing = "mypy {args:src test}" style = [ "ruff {args:.}", diff --git a/pyproject.toml b/pyproject.toml index 0d8d722..c12fed9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -150,4 +150,4 @@ is-posix = "sys_platform != 'win32'" [tool.coverage.report] show_missing = true -fail_under = 93 +fail_under = 92 diff --git a/src/openjd/adaptor_runtime/_named_pipe/named_pipe_request_handler.py b/src/openjd/adaptor_runtime/_named_pipe/named_pipe_request_handler.py index 55000ea..59be4c8 100644 --- a/src/openjd/adaptor_runtime/_named_pipe/named_pipe_request_handler.py +++ b/src/openjd/adaptor_runtime/_named_pipe/named_pipe_request_handler.py @@ -104,8 +104,8 @@ def send_response(self, status: HTTPStatus, body: str = ""): body: A string containing the message body to be sent back to the client. """ response = {"status": status, "body": body} - _logger.debug(f"NamedPipe Server: Send Response: {response}") NamedPipeHelper.write_to_pipe(self.pipe_handle, json.dumps(response)) + _logger.debug("NamedPipe Server: Sent Response.") def validate_request_path_and_method(self, request_path: str, request_method) -> bool: """ diff --git a/src/openjd/adaptor_runtime_client/win_client_interface.py b/src/openjd/adaptor_runtime_client/win_client_interface.py index fac5206..e997bbb 100644 --- a/src/openjd/adaptor_runtime_client/win_client_interface.py +++ b/src/openjd/adaptor_runtime_client/win_client_interface.py @@ -21,7 +21,7 @@ def __init__(self, server_path: str) -> None: server_path (str): Used as pipe name in Named Pipe Server. """ super().__init__(server_path) - _signal.signal(_signal.SIGTERM, self.graceful_shutdown) # type: ignore[attr-defined] + _signal.signal(_signal.SIGBREAK, self.graceful_shutdown) # type: ignore[attr-defined] def _send_request( self, diff --git a/test/openjd/adaptor_runtime/conftest.py b/test/openjd/adaptor_runtime/conftest.py index 5756c35..ab41c34 100644 --- a/test/openjd/adaptor_runtime/conftest.py +++ b/test/openjd/adaptor_runtime/conftest.py @@ -1,6 +1,5 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. import platform -import os from typing import Generator from openjd.adaptor_runtime._osname import OSName @@ -13,20 +12,6 @@ import win32netcon -# TODO: Remove this one after Windows Development finish -# https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest.hookspec.pytest_collection_modifyitems -def pytest_collection_modifyitems(items): - if OSName.is_windows(): - # Add the tests' paths that we want to enable in Windows - do_not_skip_paths = [ - os.path.abspath(os.path.dirname(__file__)), - ] - skip_marker = pytest.mark.skip(reason="Skipping tests on Windows") - for item in items: - if not any(not_skip_path in item.fspath.strpath for not_skip_path in do_not_skip_paths): - item.add_marker(skip_marker) - - # List of platforms that can be used to mark tests as specific to that platform # See [tool.pytest.ini_options] -> markers in pyproject.toml _PLATFORMS = set( diff --git a/test/openjd/adaptor_runtime_client/integ/fake_client.py b/test/openjd/adaptor_runtime_client/integ/fake_client.py index 6f49a6e..b5e1b28 100644 --- a/test/openjd/adaptor_runtime_client/integ/fake_client.py +++ b/test/openjd/adaptor_runtime_client/integ/fake_client.py @@ -2,16 +2,17 @@ from __future__ import annotations +from signal import Signals from time import sleep as _sleep from types import FrameType as _FrameType from typing import Any as _Any from typing import Dict as _Dict from typing import Optional as _Optional -from openjd.adaptor_runtime_client import HTTPClientInterface as _HTTPClientInterface +from openjd.adaptor_runtime_client import ClientInterface as _ClientInterface -class FakeClient(_HTTPClientInterface): +class FakeClient(_ClientInterface): shutdown: bool def __init__(self, port: str) -> None: @@ -22,7 +23,7 @@ def close(self, args: _Optional[_Dict[str, _Any]]) -> None: print("closing") def graceful_shutdown(self, signum: int, frame: _Optional[_FrameType]) -> None: - print("Received SIGTERM signal.") + print(f"Received {Signals(signum).name} signal.") self.shutdown = True def run(self): diff --git a/test/openjd/adaptor_runtime_client/integ/test_integration_client_interface.py b/test/openjd/adaptor_runtime_client/integ/test_integration_client_interface.py index 0908816..6af1901 100644 --- a/test/openjd/adaptor_runtime_client/integ/test_integration_client_interface.py +++ b/test/openjd/adaptor_runtime_client/integ/test_integration_client_interface.py @@ -3,17 +3,23 @@ from __future__ import annotations import os as _os +import signal +import sys import subprocess as _subprocess from time import sleep as _sleep +from typing import Dict, Any + +from openjd.adaptor_runtime._osname import OSName class TestIntegrationClientInterface: """ "These are the integration tests for the client interface.""" def test_graceful_shutdown(self) -> None: - client_subprocess = _subprocess.Popen( - [ - "python", + # Create the subprocess + popen_params: Dict[str, Any] = dict( + args=[ + sys.executable, _os.path.join(_os.path.dirname(_os.path.realpath(__file__)), "fake_client.py"), ], stdin=_subprocess.PIPE, @@ -21,15 +27,25 @@ def test_graceful_shutdown(self) -> None: stdout=_subprocess.PIPE, encoding="utf-8", ) + if OSName.is_windows(): + # In Windows, this is required for signal. SIGBREAK will be sent to the entire process group. + # Without this one, current process will also get the SIGBREAK and may react incorrectly. + popen_params.update(creationflags=_subprocess.CREATE_NEW_PROCESS_GROUP) # type: ignore[attr-defined] + client_subprocess = _subprocess.Popen(**popen_params) # To avoid a race condition, giving some extra time for the logging subprocess to start. - _sleep(0.5) - client_subprocess.terminate() + _sleep(0.5 if OSName.is_posix() else 4) + if OSName.is_windows(): + signal_type = signal.CTRL_BREAK_EVENT # type: ignore[attr-defined] + else: + signal_type = signal.SIGTERM + + client_subprocess.send_signal(signal_type) # To avoid a race condition, giving some extra time for the log to be updated after # receiving the signal. - _sleep(0.5) + _sleep(0.5 if OSName.is_posix() else 4) out, _ = client_subprocess.communicate() - assert "Received SIGTERM signal." in out + assert f"Received {'SIGBREAK' if OSName.is_windows() else 'SIGTERM'} signal." in out diff --git a/test/openjd/adaptor_runtime_client/unit/test_client_interface.py b/test/openjd/adaptor_runtime_client/unit/test_client_interface.py index e1ec6de..b9e315c 100644 --- a/test/openjd/adaptor_runtime_client/unit/test_client_interface.py +++ b/test/openjd/adaptor_runtime_client/unit/test_client_interface.py @@ -2,27 +2,35 @@ from http import HTTPStatus import json +from signal import Signals from types import FrameType as _FrameType from typing import ( Any as _Any, Dict as _Dict, List as _List, Optional as _Optional, + Generator, + Tuple, + Dict, ) from unittest import mock +from unittest.mock import patch, MagicMock from urllib.parse import urlencode import pytest from _pytest.capture import CaptureFixture as _CaptureFixture +from openjd.adaptor_runtime._osname import OSName from openjd.adaptor_runtime_client import ( Action as _Action, - HTTPClientInterface as _HTTPClientInterface, + ClientInterface as _ClientInterface, PathMappingRule as _PathMappingRule, ) +win_client_interface = pytest.importorskip("openjd.adaptor_runtime_client.win_client_interface") -class FakeClient(_HTTPClientInterface): + +class FakeClient(_ClientInterface): """Since we need to override the DCC Client (because it's an interface). We are going to use this FakeClient for our testing. """ @@ -35,14 +43,15 @@ def hello_world(self, args: _Optional[_Dict[str, _Any]]) -> None: print(f"args = {args}") def graceful_shutdown(self, signum: int, frame: _Optional[_FrameType]) -> None: - print("Received SIGTERM signal.") + print(f"Received {Signals(signum).name} signal.") # This function needs to be overridden. def close(self, args: _Optional[_Dict[str, _Any]]) -> None: pass -class TestClientInterface: +@pytest.mark.skipif(not OSName.is_posix(), reason="Posix-specific tests") +class TestLinuxClientInterface: @pytest.mark.parametrize( argnames=("original_path", "new_path"), argvalues=[ @@ -334,13 +343,317 @@ def test_request_next_action( assert reason == "mocked_reason" assert str(action) == str(a1) - @mock.patch.object(_HTTPClientInterface, "_perform_action") + +class TestWindowsClientInterface: + @pytest.fixture + def patched_named_pipe_helper( + self, + ) -> Generator[Tuple[MagicMock, MagicMock, MagicMock], None, None]: + with patch.object( + win_client_interface.NamedPipeHelper, "write_to_pipe" + ) as mock_write_to_pipe, patch.object( + win_client_interface.NamedPipeHelper, "establish_named_pipe_connection" + ) as mock_establish_named_pipe_connection, patch.object( + win_client_interface.NamedPipeHelper, "read_from_pipe" + ) as mock_read_from_pipe: + yield mock_write_to_pipe, mock_establish_named_pipe_connection, mock_read_from_pipe + + @pytest.mark.parametrize( + argnames=("original_path", "new_path"), + argvalues=[ + ("original/path", "new/path"), + ("🌚\\🌒\\🌓\\🌔\\🌝\\🌖\\🌗\\🌘\\🌚", "🌝/🌖/🌗/🌘/🌚/🌒/🌓/🌔/🌝"), + ], + ) + def test_map_path( + self, + original_path: str, + new_path: str, + patched_named_pipe_helper: Tuple[MagicMock, MagicMock, MagicMock], + ) -> None: + ( + mock_write_to_pipe, + mock_establish_named_pipe_connection, + mock_read_from_pipe, + ) = patched_named_pipe_helper + # GIVEN + body = json.dumps({"status": 200, "body": json.dumps({"path": new_path})}) + mock_read_from_pipe.return_value = body + dcc_client = FakeClient(socket_path="socket_path") + # WHEN + mapped = dcc_client.map_path(original_path) + + # THEN + assert mapped == new_path + mock_write_to_pipe.assert_has_calls( + [ + mock.call( + mock_establish_named_pipe_connection(), + json.dumps( + { + "method": "GET", + "path": "/path_mapping", + "params": json.dumps({"path": [original_path]}), + } + ), + ), + ] + ) + mock_establish_named_pipe_connection().close.assert_called_once() + + @pytest.mark.parametrize( + argnames=("rules"), + argvalues=[ + ( + [ + { + "source_path_format": "one", + "source_path": "here", + "destination_os": "two", + "destination_path": "there", + } + ] + ), + ], + ) + def test_path_mapping_rules( + self, + rules: _List[_Any], + patched_named_pipe_helper: Tuple[MagicMock, MagicMock, MagicMock], + ) -> None: + ( + mock_write_to_pipe, + mock_establish_named_pipe_connection, + mock_read_from_pipe, + ) = patched_named_pipe_helper + body = json.dumps({"status": 200, "body": json.dumps({"path_mapping_rules": rules})}) + mock_read_from_pipe.return_value = body + + dcc_client = FakeClient(socket_path="socket_path") + + # WHEN + expected = dcc_client.path_mapping_rules() + + # THEN + assert len(expected) == len(rules) + for i in range(0, len(expected)): + assert _PathMappingRule(**rules[i]) == expected[i] + + mock_write_to_pipe.assert_has_calls( + [ + mock.call( + mock_establish_named_pipe_connection(), + json.dumps( + { + "method": "GET", + "path": "/path_mapping_rules", + } + ), + ), + ] + ) + mock_establish_named_pipe_connection().close.assert_called_once() + + def test_path_mapping_rules_throws_nonvalid_json( + self, + patched_named_pipe_helper: Tuple[MagicMock, MagicMock, MagicMock], + ) -> None: + ( + mock_write_to_pipe, + mock_establish_named_pipe_connection, + mock_read_from_pipe, + ) = patched_named_pipe_helper + # GIVEN + + body = json.dumps({"status": 200, "body": "bad json"}) + mock_read_from_pipe.return_value = body + + client = FakeClient(socket_path="socket_path") + + # WHEN + with pytest.raises(RuntimeError) as raised_err: + client.path_mapping_rules() + + # THEN + assert "Expected JSON string from /path_mapping_rules endpoint, but got error: " in str( + raised_err.value + ) + + mock_write_to_pipe.assert_has_calls( + [ + mock.call( + mock_establish_named_pipe_connection(), + json.dumps( + { + "method": "GET", + "path": "/path_mapping_rules", + } + ), + ), + ] + ) + + mock_read_from_pipe.assert_called_once() + mock_establish_named_pipe_connection().close.assert_called_once() + + @pytest.mark.parametrize( + "response_val, expected_error", + [ + ( + {"path_mapping_rules": "this-is-not-a-list"}, + "Expected list for path_mapping_rules, but got: this-is-not-a-list", + ), + ( + {"path_mapping_rules": ["not-a-rule-dict"]}, + "Expected PathMappingRule object, but got: not-a-rule-dict", + ), + ], + ) + def test_path_mapping_rules_throws_errors( + self, + response_val: Dict[str, str], + expected_error: str, + patched_named_pipe_helper: Tuple[MagicMock, MagicMock, MagicMock], + ) -> None: + ( + mock_write_to_pipe, + mock_establish_named_pipe_connection, + mock_read_from_pipe, + ) = patched_named_pipe_helper + # GIVEN + body = json.dumps({"status": 200, "body": json.dumps(response_val)}) + mock_read_from_pipe.return_value = body + client = FakeClient(socket_path="socket_path") + + # WHEN + with pytest.raises(RuntimeError) as raised_err: + client.path_mapping_rules() + + # THEN + assert expected_error in str(raised_err.value) + mock_write_to_pipe.assert_has_calls( + [ + mock.call( + mock_establish_named_pipe_connection(), + json.dumps( + { + "method": "GET", + "path": "/path_mapping_rules", + } + ), + ), + ] + ) + mock_read_from_pipe.assert_called_once() + mock_establish_named_pipe_connection().close.assert_called_once() + + def test_map_path_error( + self, + patched_named_pipe_helper: Tuple[MagicMock, MagicMock, MagicMock], + ) -> None: + ( + mock_write_to_pipe, + mock_establish_named_pipe_connection, + mock_read_from_pipe, + ) = patched_named_pipe_helper + ORIGINAL_PATH = "some/path" + REASON = "Could not process request." + + body = json.dumps({"status": 500, "body": REASON}) + mock_read_from_pipe.return_value = body + + dcc_client = FakeClient(socket_path="socket_path") + + # WHEN + with pytest.raises(RuntimeError) as exc_info: + dcc_client.map_path(ORIGINAL_PATH) + + # THEN + mock_write_to_pipe.assert_has_calls( + [ + mock.call( + mock_establish_named_pipe_connection(), + json.dumps( + { + "method": "GET", + "path": "/path_mapping", + "params": '{"path": ["some/path"]}', + } + ), + ), + ] + ) + mock_read_from_pipe.assert_called_once() + mock_establish_named_pipe_connection().close.assert_called_once() + assert str(exc_info.value) == ( + f"ERROR: Failed to get a mapped path for path '{ORIGINAL_PATH}'. " + f"Server response: Status: 500, Response: '{REASON}'" + ) + + def test_request_next_action( + self, + patched_named_pipe_helper: Tuple[MagicMock, MagicMock, MagicMock], + ) -> None: + ( + mock_write_to_pipe, + mock_establish_named_pipe_connection, + mock_read_from_pipe, + ) = patched_named_pipe_helper + + socket_path = "socket_path" + dcc_client = FakeClient(socket_path) + assert dcc_client.server_path == socket_path + body = json.dumps({"status": 200, "body": ""}) + mock_read_from_pipe.return_value = body + status, reason, action = dcc_client._request_next_action() + + assert action is None + + a1 = _Action("a1") + + body = json.dumps({"status": 200, "body": str(a1)}) + mock_read_from_pipe.return_value = body + + status, reason, action = dcc_client._request_next_action() + + mock_write_to_pipe.assert_has_calls( + [ + mock.call( + mock_establish_named_pipe_connection(), + json.dumps( + { + "method": "GET", + "path": "/action", + } + ), + ), + mock.call( + mock_establish_named_pipe_connection(), + json.dumps( + { + "method": "GET", + "path": "/action", + } + ), + ), + ] + ) + + assert len(mock_establish_named_pipe_connection().close.call_args_list) == 2 + + assert status == 200 + assert reason == "OK" + assert str(action) == str(a1) + + +class TestPerformAction: + @mock.patch.object(_ClientInterface, "_perform_action") def test_poll(self, mocked_perform_action: mock.Mock, capsys: _CaptureFixture) -> None: a1 = _Action("render", {"arg1": "val1"}) a2 = _Action("close") with mock.patch.object( - _HTTPClientInterface, + _ClientInterface, "_request_next_action", side_effect=[ (404, "Not found", a1), diff --git a/test/openjd/test_copyright_header.py b/test/openjd/test_copyright_header.py index faa445b..23b22db 100644 --- a/test/openjd/test_copyright_header.py +++ b/test/openjd/test_copyright_header.py @@ -11,7 +11,7 @@ def _check_file(filename: Path) -> None: - with open(filename) as infile: + with open(filename, "r", encoding="utf-8") as infile: lines_read = 0 for line in infile: if _copyright_header_re.search(line):