From 6526c8848d34849ed816e5659b9626990e0b53d5 Mon Sep 17 00:00:00 2001 From: Sanniti Date: Mon, 20 Jul 2020 15:40:10 -0400 Subject: [PATCH 1/6] -added GET & PUT time endpoints, added tests for system, addressed some comments -added router tests, added response links -raise error instead of returning error in regular response --- api/src/opentrons/system/time.py | 78 +++++++++++ api/tests/opentrons/system/test_time.py | 77 +++++++++++ robot-server/robot_server/service/app.py | 4 + .../robot_server/service/system/__init__.py | 0 .../robot_server/service/system/errors.py | 23 ++++ .../robot_server/service/system/models.py | 16 +++ .../robot_server/service/system/router.py | 67 ++++++++++ robot-server/tests/service/system/__init__.py | 0 .../tests/service/system/test_router.py | 121 ++++++++++++++++++ 9 files changed, 386 insertions(+) create mode 100644 api/src/opentrons/system/time.py create mode 100644 api/tests/opentrons/system/test_time.py create mode 100644 robot-server/robot_server/service/system/__init__.py create mode 100644 robot-server/robot_server/service/system/errors.py create mode 100644 robot-server/robot_server/service/system/models.py create mode 100644 robot-server/robot_server/service/system/router.py create mode 100644 robot-server/tests/service/system/__init__.py create mode 100644 robot-server/tests/service/system/test_router.py diff --git a/api/src/opentrons/system/time.py b/api/src/opentrons/system/time.py new file mode 100644 index 00000000000..a7e942f822e --- /dev/null +++ b/api/src/opentrons/system/time.py @@ -0,0 +1,78 @@ +import asyncio +import logging +from typing import Dict, Tuple, Union +from datetime import datetime, timezone +from opentrons.util.helpers import utc_now + +log = logging.getLogger(__name__) + + +def _str_to_dict(res_str) -> Dict[str, Union[str, bool]]: + res_lines = res_str.splitlines() + res_dict = {} + try: + for line in res_lines: + if line: + prop, val = line.split('=') + res_dict[prop] = val if val not in ['yes', 'no'] \ + else val == 'yes' # Convert yes/no to boolean value + except (ValueError, IndexError) as e: + raise Exception("Error converting timedatectl string: {}".format(e)) + return res_dict + + +async def _time_status(loop: asyncio.AbstractEventLoop = None + ) -> Dict[str, Union[str, bool]]: + """ + Get details of robot's date & time, with specifics of RTC (if present) + & status of NTP synchronization. + :return: Dictionary of status params + """ + proc = await asyncio.create_subprocess_shell( + 'timedatectl show', + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + loop=loop or asyncio.get_event_loop() + ) + out, err = await proc.communicate() + return _str_to_dict(out.decode()) + + +async def _set_time(time: str, + loop: asyncio.AbstractEventLoop = None) -> Tuple[str, str]: + proc = await asyncio.create_subprocess_shell( + f'date --utc --set \"{time}\"', + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + loop=loop or asyncio.get_event_loop() + ) + out, err = await proc.communicate() + return out.decode(), err.decode() + + +async def get_system_time(loop: asyncio.AbstractEventLoop = None) -> datetime: + """ + :return: Just the system time as a UTC datetime object + """ + return utc_now() + + +async def set_system_time( + new_time_dt: datetime, + loop: asyncio.AbstractEventLoop = None + ) -> Tuple[datetime, str]: + """ + Set the system time unless system time is already being synchronized using + an RTC or NTPsync. + :return: Tuple specifying current date read and error message, if any. + """ + status = await _time_status(loop) + if status.get('LocalRTC') or status.get('NTPSynchronized'): + # TODO: Update this to handle RTC sync correctly once we introduce RTC + err = 'Cannot set system time; already synchronized with NTP or RTC' + else: + new_time_dt = new_time_dt.astimezone(tz=timezone.utc) + new_time_str = new_time_dt.strftime("%Y-%m-%d %H:%M:%S") + log.info(f'Setting time to {new_time_str} UTC') + out, err = await _set_time(new_time_str) + return utc_now(), err diff --git a/api/tests/opentrons/system/test_time.py b/api/tests/opentrons/system/test_time.py new file mode 100644 index 00000000000..01181ab1f93 --- /dev/null +++ b/api/tests/opentrons/system/test_time.py @@ -0,0 +1,77 @@ +import pytest +from unittest.mock import MagicMock +from datetime import datetime, timezone +from opentrons.system import time + + +@pytest.fixture +def mock_status_str(): + return "Timezone=Etc/UTC\n" \ + "LocalRTC=no\n" \ + "CanNTP=yes\n" \ + "NTP=yes\n" \ + "NTPSynchronized=no\n" \ + "TimeUSec=Fri 2020-08-14 21:44:16 UTC\n" + +@pytest.fixture +def mock_status_dict(): + return {'Timezone': 'Etc/UTC', + 'LocalRTC': False, + 'CanNTP': True, + 'NTP': True, + 'NTPSynchronized': False, + 'TimeUSec': 'Fri 2020-08-14 21:44:16 UTC'} + + +@pytest.fixture +def mock_time(): + # The above time in datetime + return datetime(2020, 8, 14, 21, 44, 16, tzinfo=timezone.utc) + + +def test_str_to_dict(mock_status_str, mock_status_dict): + status_dict = time._str_to_dict(mock_status_str) + assert status_dict == mock_status_dict + + # Test exception raised for unexpected status string + with pytest.raises(Exception, match="Error converting timedatectl.*"): + not_a_status_str = "Something that's not a timedatectl status" + time._str_to_dict(not_a_status_str) + + +async def test_set_time_error_response(mock_status_dict): + + async def async_mock_time_status(*args, **kwargs): + _stat = mock_status_dict + _stat.update(NTPSynchronized=True) + return _stat + + time._time_status = MagicMock(side_effect=async_mock_time_status) + + now, err = await time.set_system_time(datetime.now()) + assert err == 'Cannot set system time; ' \ + 'already synchronized with NTP or RTC' + + +async def test_set_time_response(mock_status_dict, mock_time): + + async def async_mock_time_status(*args, **kwargs): + _stat = mock_status_dict + _stat.update(NTPSynchronized=False) + return _stat + + async def async_mock_set_time(*args, **kwargs): + return "out", "err" + + time._time_status = MagicMock(side_effect=async_mock_time_status) + time._set_time = MagicMock(side_effect=async_mock_set_time) + + # System time gets set successfully + time._set_time.assert_not_called() + await time.set_system_time(mock_time) + time._set_time.assert_called_once() + + # Datetime is converted to special format with UTC timezone for _set_time + await time.set_system_time(datetime.fromisoformat( + "2020-08-14T16:44:16-05:00")) # from EST + time._set_time.assert_called_with("2020-08-14 21:44:16") # to UTC diff --git a/robot-server/robot_server/service/app.py b/robot-server/robot_server/service/app.py index 2069bb9decc..186dc0aed54 100644 --- a/robot-server/robot_server/service/app.py +++ b/robot-server/robot_server/service/app.py @@ -24,6 +24,7 @@ from robot_server.service.session.router import router as session_router from robot_server.service.labware.router import router as labware_router from robot_server.service.protocol.router import router as protocol_router +from robot_server.service.system.router import router as system_router log = logging.getLogger(__name__) @@ -67,6 +68,9 @@ } }) +app.include_router(router=system_router, + tags=["System Control"]) + @app.on_event("startup") async def on_startup(): diff --git a/robot-server/robot_server/service/system/__init__.py b/robot-server/robot_server/service/system/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/robot-server/robot_server/service/system/errors.py b/robot-server/robot_server/service/system/errors.py new file mode 100644 index 00000000000..5d396da5d20 --- /dev/null +++ b/robot-server/robot_server/service/system/errors.py @@ -0,0 +1,23 @@ +from robot_server.service.errors import RobotServerError, CommonErrorDef + + +class SystemException(RobotServerError): + """Base of all system exceptions""" + pass + + +class SystemTimeAlreadySynchronized(SystemException): + """ + Cannot update system time because it is already being synchronized + via NTP or local RTC. + """ + def __init__(self, msg: str): + super().__init__(definition=CommonErrorDef.ACTION_FORBIDDEN, + reason=msg) + + +class SystemSetTimeException(SystemException): + """Server process Failure""" + def __init__(self, msg: str): + super().__init__(definition=CommonErrorDef.INTERNAL_SERVER_ERROR, + error=msg) diff --git a/robot-server/robot_server/service/system/models.py b/robot-server/robot_server/service/system/models.py new file mode 100644 index 00000000000..c09f9f7cca9 --- /dev/null +++ b/robot-server/robot_server/service/system/models.py @@ -0,0 +1,16 @@ +from datetime import datetime +from pydantic import BaseModel + +from robot_server.service.json_api import ResponseModel, ResponseDataModel, \ + RequestDataModel, RequestModel + + +class SystemTimeAttributes(BaseModel): + systemTime: datetime + + +SystemTimeResponseDataModel = ResponseDataModel[SystemTimeAttributes] + +SystemTimeResponse = ResponseModel[SystemTimeResponseDataModel, dict] + +SystemTimeRequest = RequestModel[RequestDataModel[SystemTimeAttributes]] diff --git a/robot-server/robot_server/service/system/router.py b/robot-server/robot_server/service/system/router.py new file mode 100644 index 00000000000..5fb90890d26 --- /dev/null +++ b/robot-server/robot_server/service/system/router.py @@ -0,0 +1,67 @@ +import logging +from datetime import datetime +from fastapi import APIRouter +from opentrons.system import time +from typing import Dict +from robot_server.service.system import models as time_models +from robot_server.service.system import errors +from robot_server.service.json_api import ResourceLink + +router = APIRouter() +log = logging.getLogger(__name__) + +""" +These routes allows the client to read & update robot system time +""" + + +def _create_response(dt: datetime) \ + -> time_models.SystemTimeResponse: + """Create a SystemTimeResponse with system datetime""" + return time_models.SystemTimeResponse( + data=time_models.SystemTimeResponseDataModel.create( + attributes=time_models.SystemTimeAttributes( + systemTime=dt + ), + resource_id="time" + ), + links=_get_valid_time_links(router) + ) + + +def _get_valid_time_links(api_router: APIRouter) -> Dict[str, ResourceLink]: + """ Get valid links for time resource""" + return { + "GET": ResourceLink(href=api_router.url_path_for( + get_time.__name__)), + "PUT": ResourceLink(href=api_router.url_path_for( + set_time.__name__)) + } + + +@router.get("/system/time", + description="Fetch system time & date", + summary="Get robot's time status, which includes- current UTC " + "date & time, local timezone, whether robot time is synced" + " with an NTP server &/or it has an active RTC.", + response_model=time_models.SystemTimeResponse + ) +async def get_time() -> time_models.SystemTimeResponse: + res = await time.get_system_time() + return _create_response(res) + + +@router.put("/system/time", + description="Update system time", + summary="Set robot time", + response_model=time_models.SystemTimeResponse) +async def set_time(new_time: time_models.SystemTimeRequest) \ + -> time_models.SystemTimeResponse: + sys_time, err = await time.set_system_time( + new_time.data.attributes.systemTime) + if err: + if 'already synchronized with NTP or RTC' in err: + raise errors.SystemTimeAlreadySynchronized(err) + else: + raise errors.SystemSetTimeException(err) + return _create_response(sys_time) diff --git a/robot-server/tests/service/system/__init__.py b/robot-server/tests/service/system/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/robot-server/tests/service/system/test_router.py b/robot-server/tests/service/system/test_router.py new file mode 100644 index 00000000000..b9e03292606 --- /dev/null +++ b/robot-server/tests/service/system/test_router.py @@ -0,0 +1,121 @@ +import pytest +from unittest.mock import MagicMock +from datetime import datetime, timezone +from opentrons.system import time + + +@pytest.fixture +def mock_system_time(): + return datetime(2020, 8, 14, 21, 44, 16, tzinfo=timezone.utc) + + +@pytest.fixture +def response_links(): + return { + "GET": { + "href": "/system/time", + "meta": None + }, + "PUT": { + "href": "/system/time", + "meta": None + } + } + + +def test_raise_system_synchronized_error(api_client, mock_system_time): + async def mock_set_system_time(*args, **kwargs): + return mock_system_time, "Cannot set system time; " \ + "already synchronized with NTP or RTC" + + time.set_system_time = MagicMock(side_effect=mock_set_system_time) + + response = api_client.put("/system/time", json={ + "data": { + "id": "time", + "type": "SystemTimeAttributes", + "attributes": {"systemTime": mock_system_time.isoformat()} + } + }) + assert response.json() == {'errors': [{ + 'detail': 'Cannot set system time; already synchronized with NTP ' + 'or RTC', + 'status': '403', + 'title': 'Action Forbidden'}]} + assert response.status_code == 403 + + +def test_raise_system_exception(api_client, mock_system_time): + async def mock_set_system_time(*args, **kwargs): + return mock_system_time, "Something went wrong." + + time.set_system_time = MagicMock(side_effect=mock_set_system_time) + + response = api_client.put("/system/time", json={ + "data": { + "id": "time", + "type": "SystemTimeAttributes", + "attributes": {"systemTime": mock_system_time.isoformat()} + } + }) + assert response.json() == {'errors': [{ + 'detail': 'Something went wrong.', + 'status': '500', + 'title': 'Internal Server Error'}]} + assert response.status_code == 500 + + +def test_get_system_time(api_client, mock_system_time, response_links): + async def mock_get_system_time(*args, **kwargs): + return mock_system_time + time.get_system_time = MagicMock(side_effect=mock_get_system_time) + + response = api_client.get("/system/time") + assert response.json() == { + 'data': { + 'attributes': {'systemTime': mock_system_time.isoformat()}, + 'id': 'time', + 'type': 'SystemTimeAttributes'}, + 'links': response_links, + 'meta': None + } + assert response.status_code == 200 + + +def test_set_with_missing_field(api_client, mock_system_time): + + response = api_client.put("/system/time") + assert response.json() == { + 'errors': [{ + 'detail': 'field required', + 'source': {'pointer': '/body/new_time'}, + 'status': '422', + 'title': 'value_error.missing'}]} + assert response.status_code == 422 + + +def test_set_system_time(api_client, mock_system_time, response_links): + async def mock_set_system_time(*args, **kwargs): + return mock_system_time, "" + + time.set_system_time = MagicMock(side_effect=mock_set_system_time) + + # Correct request + response = api_client.put("/system/time", + json={ + 'data': { + 'attributes': { + 'systemTime': + mock_system_time.isoformat()}, + 'id': 'time', + 'type': 'SystemTimeAttributes'}, + }) + assert response.json() == { + 'data': { + 'attributes': {'systemTime': mock_system_time.isoformat()}, + 'id': 'time', + 'type': 'SystemTimeAttributes'}, + 'links': response_links, + 'meta': None + } + assert response.status_code == 200 From 9b1cc58e1b033dd1f29db15698ddd475beda7023 Mon Sep 17 00:00:00 2001 From: Sanniti Date: Wed, 26 Aug 2020 12:36:57 -0400 Subject: [PATCH 2/6] add docstring & fix linter error --- api/src/opentrons/system/time.py | 4 ++++ api/tests/opentrons/system/test_time.py | 1 + 2 files changed, 5 insertions(+) diff --git a/api/src/opentrons/system/time.py b/api/src/opentrons/system/time.py index a7e942f822e..933c074b178 100644 --- a/api/src/opentrons/system/time.py +++ b/api/src/opentrons/system/time.py @@ -40,6 +40,10 @@ async def _time_status(loop: asyncio.AbstractEventLoop = None async def _set_time(time: str, loop: asyncio.AbstractEventLoop = None) -> Tuple[str, str]: + """ + :return: tuple of output of date --set (usually the new date) + & error, if any + """ proc = await asyncio.create_subprocess_shell( f'date --utc --set \"{time}\"', stdout=asyncio.subprocess.PIPE, diff --git a/api/tests/opentrons/system/test_time.py b/api/tests/opentrons/system/test_time.py index 01181ab1f93..2e8df69b7c3 100644 --- a/api/tests/opentrons/system/test_time.py +++ b/api/tests/opentrons/system/test_time.py @@ -13,6 +13,7 @@ def mock_status_str(): "NTPSynchronized=no\n" \ "TimeUSec=Fri 2020-08-14 21:44:16 UTC\n" + @pytest.fixture def mock_status_dict(): return {'Timezone': 'Etc/UTC', From f13c8d5c0bcb0364280d8ea9bde5ac7c155b50d3 Mon Sep 17 00:00:00 2001 From: Sanniti Date: Mon, 31 Aug 2020 16:04:12 -0400 Subject: [PATCH 3/6] move api/system/time to robot-server. Tests will fail. --- robot-server/robot_server/service/system/router.py | 2 +- robot-server/robot_server/system/__init__.py | 0 .../opentrons => robot-server/robot_server}/system/time.py | 0 robot-server/tests/service/system/test_router.py | 7 ++++++- robot-server/tests/system/__init__.py | 0 .../opentrons => robot-server/tests}/system/test_time.py | 2 +- 6 files changed, 8 insertions(+), 3 deletions(-) create mode 100644 robot-server/robot_server/system/__init__.py rename {api/src/opentrons => robot-server/robot_server}/system/time.py (100%) create mode 100644 robot-server/tests/system/__init__.py rename {api/tests/opentrons => robot-server/tests}/system/test_time.py (98%) diff --git a/robot-server/robot_server/service/system/router.py b/robot-server/robot_server/service/system/router.py index 5fb90890d26..99bc3c180ad 100644 --- a/robot-server/robot_server/service/system/router.py +++ b/robot-server/robot_server/service/system/router.py @@ -1,7 +1,7 @@ import logging from datetime import datetime from fastapi import APIRouter -from opentrons.system import time +from robot_server.system import time from typing import Dict from robot_server.service.system import models as time_models from robot_server.service.system import errors diff --git a/robot-server/robot_server/system/__init__.py b/robot-server/robot_server/system/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/api/src/opentrons/system/time.py b/robot-server/robot_server/system/time.py similarity index 100% rename from api/src/opentrons/system/time.py rename to robot-server/robot_server/system/time.py diff --git a/robot-server/tests/service/system/test_router.py b/robot-server/tests/service/system/test_router.py index b9e03292606..30ee1448131 100644 --- a/robot-server/tests/service/system/test_router.py +++ b/robot-server/tests/service/system/test_router.py @@ -1,7 +1,7 @@ import pytest from unittest.mock import MagicMock from datetime import datetime, timezone -from opentrons.system import time +from robot_server.system import time @pytest.fixture @@ -44,7 +44,10 @@ async def mock_set_system_time(*args, **kwargs): 'title': 'Action Forbidden'}]} assert response.status_code == 403 + # time.set_system_time.reset_mock(return_value=True, side_effect=True) + +@pytest.mark.skip def test_raise_system_exception(api_client, mock_system_time): async def mock_set_system_time(*args, **kwargs): return mock_system_time, "Something went wrong." @@ -82,6 +85,7 @@ async def mock_get_system_time(*args, **kwargs): assert response.status_code == 200 +@pytest.mark.skip def test_set_with_missing_field(api_client, mock_system_time): response = api_client.put("/system/time") @@ -94,6 +98,7 @@ def test_set_with_missing_field(api_client, mock_system_time): assert response.status_code == 422 +@pytest.mark.skip def test_set_system_time(api_client, mock_system_time, response_links): async def mock_set_system_time(*args, **kwargs): return mock_system_time, "" diff --git a/robot-server/tests/system/__init__.py b/robot-server/tests/system/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/api/tests/opentrons/system/test_time.py b/robot-server/tests/system/test_time.py similarity index 98% rename from api/tests/opentrons/system/test_time.py rename to robot-server/tests/system/test_time.py index 2e8df69b7c3..15808aaad9a 100644 --- a/api/tests/opentrons/system/test_time.py +++ b/robot-server/tests/system/test_time.py @@ -1,7 +1,7 @@ import pytest from unittest.mock import MagicMock from datetime import datetime, timezone -from opentrons.system import time +from robot_server.system import time @pytest.fixture From a3ae97700f6d8d6f049ed6a68676eb1bd877fca0 Mon Sep 17 00:00:00 2001 From: Sanniti Date: Wed, 2 Sep 2020 13:06:48 -0400 Subject: [PATCH 4/6] added tavern tests, updated error response, suppressed str_to_dict errors --- .../robot_server/service/system/router.py | 9 +- .../{service => }/system/errors.py | 0 robot-server/robot_server/system/time.py | 39 ++++---- .../system/test_system_time.tavern.yaml | 51 +++++++++++ .../tests/service/system/test_router.py | 88 ++++++++----------- robot-server/tests/system/test_time.py | 38 ++++++-- 6 files changed, 142 insertions(+), 83 deletions(-) rename robot-server/robot_server/{service => }/system/errors.py (100%) create mode 100644 robot-server/tests/integration/system/test_system_time.tavern.yaml diff --git a/robot-server/robot_server/service/system/router.py b/robot-server/robot_server/service/system/router.py index 99bc3c180ad..d6e623acb2b 100644 --- a/robot-server/robot_server/service/system/router.py +++ b/robot-server/robot_server/service/system/router.py @@ -4,7 +4,6 @@ from robot_server.system import time from typing import Dict from robot_server.service.system import models as time_models -from robot_server.service.system import errors from robot_server.service.json_api import ResourceLink router = APIRouter() @@ -57,11 +56,5 @@ async def get_time() -> time_models.SystemTimeResponse: response_model=time_models.SystemTimeResponse) async def set_time(new_time: time_models.SystemTimeRequest) \ -> time_models.SystemTimeResponse: - sys_time, err = await time.set_system_time( - new_time.data.attributes.systemTime) - if err: - if 'already synchronized with NTP or RTC' in err: - raise errors.SystemTimeAlreadySynchronized(err) - else: - raise errors.SystemSetTimeException(err) + sys_time = await time.set_system_time(new_time.data.attributes.systemTime) return _create_response(sys_time) diff --git a/robot-server/robot_server/service/system/errors.py b/robot-server/robot_server/system/errors.py similarity index 100% rename from robot-server/robot_server/service/system/errors.py rename to robot-server/robot_server/system/errors.py diff --git a/robot-server/robot_server/system/time.py b/robot-server/robot_server/system/time.py index 933c074b178..3a9a795cb79 100644 --- a/robot-server/robot_server/system/time.py +++ b/robot-server/robot_server/system/time.py @@ -3,6 +3,7 @@ from typing import Dict, Tuple, Union from datetime import datetime, timezone from opentrons.util.helpers import utc_now +from robot_server.system import errors log = logging.getLogger(__name__) @@ -10,14 +11,15 @@ def _str_to_dict(res_str) -> Dict[str, Union[str, bool]]: res_lines = res_str.splitlines() res_dict = {} - try: - for line in res_lines: - if line: + + for line in res_lines: + if line: + try: prop, val = line.split('=') res_dict[prop] = val if val not in ['yes', 'no'] \ else val == 'yes' # Convert yes/no to boolean value - except (ValueError, IndexError) as e: - raise Exception("Error converting timedatectl string: {}".format(e)) + except (ValueError, IndexError) as e: + log.error("Error converting timedatectl string: {}".format(e)) return res_dict @@ -26,7 +28,7 @@ async def _time_status(loop: asyncio.AbstractEventLoop = None """ Get details of robot's date & time, with specifics of RTC (if present) & status of NTP synchronization. - :return: Dictionary of status params + :return: Dictionary of status params. """ proc = await asyncio.create_subprocess_shell( 'timedatectl show', @@ -42,7 +44,7 @@ async def _set_time(time: str, loop: asyncio.AbstractEventLoop = None) -> Tuple[str, str]: """ :return: tuple of output of date --set (usually the new date) - & error, if any + & error, if any. """ proc = await asyncio.create_subprocess_shell( f'date --utc --set \"{time}\"', @@ -56,27 +58,30 @@ async def _set_time(time: str, async def get_system_time(loop: asyncio.AbstractEventLoop = None) -> datetime: """ - :return: Just the system time as a UTC datetime object + :return: Just the system time as a UTC datetime object. """ return utc_now() -async def set_system_time( - new_time_dt: datetime, - loop: asyncio.AbstractEventLoop = None - ) -> Tuple[datetime, str]: +async def set_system_time(new_time_dt: datetime, + loop: asyncio.AbstractEventLoop = None + ) -> datetime: """ Set the system time unless system time is already being synchronized using an RTC or NTPsync. - :return: Tuple specifying current date read and error message, if any. + Raise error with message, if any. + :return: current date read. """ status = await _time_status(loop) - if status.get('LocalRTC') or status.get('NTPSynchronized'): + if status.get('LocalRTC') is True or status.get('NTPSynchronized') is True: # TODO: Update this to handle RTC sync correctly once we introduce RTC - err = 'Cannot set system time; already synchronized with NTP or RTC' + raise errors.SystemTimeAlreadySynchronized( + 'Cannot set system time; already synchronized with NTP or RTC') else: new_time_dt = new_time_dt.astimezone(tz=timezone.utc) new_time_str = new_time_dt.strftime("%Y-%m-%d %H:%M:%S") log.info(f'Setting time to {new_time_str} UTC') - out, err = await _set_time(new_time_str) - return utc_now(), err + _, err = await _set_time(new_time_str) + if err: + raise errors.SystemSetTimeException(err) + return utc_now() diff --git a/robot-server/tests/integration/system/test_system_time.tavern.yaml b/robot-server/tests/integration/system/test_system_time.tavern.yaml new file mode 100644 index 00000000000..73aec4e4777 --- /dev/null +++ b/robot-server/tests/integration/system/test_system_time.tavern.yaml @@ -0,0 +1,51 @@ +--- +test_name: GET Time +marks: + - usefixtures: + - run_server +stages: + - name: System Time GET request returns time in correct format + request: + url: "{host:s}:{port:d}/system/time" + method: GET + response: + status_code: 200 + json: + data: + attributes: + systemTime: !re_search "^\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+\\+\\d{2}:\\d{2}$" + id: 'time' + type: 'SystemTimeAttributes' + links: + GET: + href: "/system/time" + meta: null + PUT: + href: "/system/time" + meta: null + meta: null + +--- +test_name: PUT Time +marks: + - usefixtures: + - run_server +stages: + - name: System Time PUT request without a time returns a missing field error + request: + url: "{host:s}:{port:d}/system/time" + method: PUT + json: + data: + id: "time" + type: "SystemTimeAttributes" + attributes: {} + response: + status_code: 422 + json: + errors: + - status: "422" + title: "value_error.missing" + detail: "field required" + source: + pointer: "/body/new_time/data/attributes/systemTime" diff --git a/robot-server/tests/service/system/test_router.py b/robot-server/tests/service/system/test_router.py index 30ee1448131..635f0bab368 100644 --- a/robot-server/tests/service/system/test_router.py +++ b/robot-server/tests/service/system/test_router.py @@ -1,7 +1,7 @@ import pytest -from unittest.mock import MagicMock +from unittest.mock import patch from datetime import datetime, timezone -from robot_server.system import time +from robot_server.system import time, errors @pytest.fixture @@ -9,6 +9,12 @@ def mock_system_time(): return datetime(2020, 8, 14, 21, 44, 16, tzinfo=timezone.utc) +@pytest.fixture +def mock_set_system_time(mock_system_time): + with patch.object(time, 'set_system_time') as p: + yield p + + @pytest.fixture def response_links(): return { @@ -23,12 +29,11 @@ def response_links(): } -def test_raise_system_synchronized_error(api_client, mock_system_time): - async def mock_set_system_time(*args, **kwargs): - return mock_system_time, "Cannot set system time; " \ - "already synchronized with NTP or RTC" - - time.set_system_time = MagicMock(side_effect=mock_set_system_time) +def test_raise_system_synchronized_error(api_client, + mock_system_time, + mock_set_system_time): + mock_set_system_time.side_effect = errors.SystemTimeAlreadySynchronized( + 'Cannot set system time; already synchronized with NTP or RTC') response = api_client.put("/system/time", json={ "data": { @@ -44,15 +49,12 @@ async def mock_set_system_time(*args, **kwargs): 'title': 'Action Forbidden'}]} assert response.status_code == 403 - # time.set_system_time.reset_mock(return_value=True, side_effect=True) - - -@pytest.mark.skip -def test_raise_system_exception(api_client, mock_system_time): - async def mock_set_system_time(*args, **kwargs): - return mock_system_time, "Something went wrong." - time.set_system_time = MagicMock(side_effect=mock_set_system_time) +def test_raise_system_exception(api_client, + mock_system_time, + mock_set_system_time): + mock_set_system_time.side_effect = errors.SystemSetTimeException( + 'Something went wrong') response = api_client.put("/system/time", json={ "data": { @@ -62,48 +64,36 @@ async def mock_set_system_time(*args, **kwargs): } }) assert response.json() == {'errors': [{ - 'detail': 'Something went wrong.', + 'detail': 'Something went wrong', 'status': '500', 'title': 'Internal Server Error'}]} assert response.status_code == 500 def test_get_system_time(api_client, mock_system_time, response_links): - async def mock_get_system_time(*args, **kwargs): - return mock_system_time - time.get_system_time = MagicMock(side_effect=mock_get_system_time) - - response = api_client.get("/system/time") - assert response.json() == { - 'data': { - 'attributes': {'systemTime': mock_system_time.isoformat()}, - 'id': 'time', - 'type': 'SystemTimeAttributes'}, - 'links': response_links, - 'meta': None - } - assert response.status_code == 200 - - -@pytest.mark.skip -def test_set_with_missing_field(api_client, mock_system_time): - - response = api_client.put("/system/time") - assert response.json() == { - 'errors': [{ - 'detail': 'field required', - 'source': {'pointer': '/body/new_time'}, - 'status': '422', - 'title': 'value_error.missing'}]} - assert response.status_code == 422 + with patch.object(time, 'get_system_time') as p: + async def mock_get_system_time(*args, **kwargs): + return mock_system_time + p.side_effect = mock_get_system_time + + response = api_client.get("/system/time") + assert response.json() == { + 'data': { + 'attributes': {'systemTime': mock_system_time.isoformat()}, + 'id': 'time', + 'type': 'SystemTimeAttributes'}, + 'links': response_links, + 'meta': None + } + assert response.status_code == 200 -@pytest.mark.skip -def test_set_system_time(api_client, mock_system_time, response_links): - async def mock_set_system_time(*args, **kwargs): - return mock_system_time, "" +def test_set_system_time(api_client, mock_system_time, + mock_set_system_time, response_links): + async def mock_side_effect(*args, **kwargs): + return mock_system_time - time.set_system_time = MagicMock(side_effect=mock_set_system_time) + mock_set_system_time.side_effect = mock_side_effect # Correct request response = api_client.put("/system/time", diff --git a/robot-server/tests/system/test_time.py b/robot-server/tests/system/test_time.py index 15808aaad9a..d72b40946b8 100644 --- a/robot-server/tests/system/test_time.py +++ b/robot-server/tests/system/test_time.py @@ -2,6 +2,7 @@ from unittest.mock import MagicMock from datetime import datetime, timezone from robot_server.system import time +from robot_server.system import errors @pytest.fixture @@ -34,13 +35,16 @@ def test_str_to_dict(mock_status_str, mock_status_dict): status_dict = time._str_to_dict(mock_status_str) assert status_dict == mock_status_dict - # Test exception raised for unexpected status string - with pytest.raises(Exception, match="Error converting timedatectl.*"): - not_a_status_str = "Something that's not a timedatectl status" - time._str_to_dict(not_a_status_str) +@pytest.mark.parametrize( + argnames=["mock_status_err_str"], + argvalues=[[""], ["There is no equal sign"], ["=== Too many ==="]]) +def test_str_to_dict_does_not_raise_error(mock_status_err_str): + res_dict = time._str_to_dict(mock_status_err_str) + assert res_dict == {} -async def test_set_time_error_response(mock_status_dict): + +async def test_set_time_synchronized_error_response(mock_status_dict): async def async_mock_time_status(*args, **kwargs): _stat = mock_status_dict @@ -49,9 +53,25 @@ async def async_mock_time_status(*args, **kwargs): time._time_status = MagicMock(side_effect=async_mock_time_status) - now, err = await time.set_system_time(datetime.now()) - assert err == 'Cannot set system time; ' \ - 'already synchronized with NTP or RTC' + with pytest.raises(errors.SystemTimeAlreadySynchronized): + await time.set_system_time(datetime.now()) + + +async def test_set_time_general_error_response(mock_status_dict): + + async def async_mock_time_status(*args, **kwargs): + _stat = mock_status_dict + _stat.update(NTPSynchronized=False) + return _stat + + async def async_mock_set_time(*args, **kwargs): + return "out", "An error occurred" + + time._time_status = MagicMock(side_effect=async_mock_time_status) + time._set_time = MagicMock(side_effect=async_mock_set_time) + + with pytest.raises(errors.SystemSetTimeException): + await time.set_system_time(datetime.now()) async def test_set_time_response(mock_status_dict, mock_time): @@ -62,7 +82,7 @@ async def async_mock_time_status(*args, **kwargs): return _stat async def async_mock_set_time(*args, **kwargs): - return "out", "err" + return "out", "" time._time_status = MagicMock(side_effect=async_mock_time_status) time._set_time = MagicMock(side_effect=async_mock_set_time) From fbdad1b562eb76a6836dc4bf2e3357887065b06e Mon Sep 17 00:00:00 2001 From: Sanniti Date: Wed, 2 Sep 2020 13:52:15 -0400 Subject: [PATCH 5/6] forgot to remove redundant test --- .../tests/service/system/test_router.py | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/robot-server/tests/service/system/test_router.py b/robot-server/tests/service/system/test_router.py index 635f0bab368..948f3005f18 100644 --- a/robot-server/tests/service/system/test_router.py +++ b/robot-server/tests/service/system/test_router.py @@ -70,24 +70,6 @@ def test_raise_system_exception(api_client, assert response.status_code == 500 -def test_get_system_time(api_client, mock_system_time, response_links): - with patch.object(time, 'get_system_time') as p: - async def mock_get_system_time(*args, **kwargs): - return mock_system_time - p.side_effect = mock_get_system_time - - response = api_client.get("/system/time") - assert response.json() == { - 'data': { - 'attributes': {'systemTime': mock_system_time.isoformat()}, - 'id': 'time', - 'type': 'SystemTimeAttributes'}, - 'links': response_links, - 'meta': None - } - assert response.status_code == 200 - - def test_set_system_time(api_client, mock_system_time, mock_set_system_time, response_links): async def mock_side_effect(*args, **kwargs): From da033b9f4ea9accc2d0eacc0ed1c969a3e35dada Mon Sep 17 00:00:00 2001 From: Sanniti Date: Wed, 2 Sep 2020 16:10:15 -0400 Subject: [PATCH 6/6] better error message --- robot-server/robot_server/system/time.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/robot-server/robot_server/system/time.py b/robot-server/robot_server/system/time.py index 3a9a795cb79..ddd3fd701f4 100644 --- a/robot-server/robot_server/system/time.py +++ b/robot-server/robot_server/system/time.py @@ -19,7 +19,8 @@ def _str_to_dict(res_str) -> Dict[str, Union[str, bool]]: res_dict[prop] = val if val not in ['yes', 'no'] \ else val == 'yes' # Convert yes/no to boolean value except (ValueError, IndexError) as e: - log.error("Error converting timedatectl string: {}".format(e)) + log.error(f'Error converting timedatectl status line {line}:' + f' {e}') return res_dict