Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling in measurement register with discovery service #353

Merged
merged 49 commits into from
Aug 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
9ec5243
Feat: Added global boolean to identify test environment
MounikaBattu17 Aug 8, 2023
eb74dd6
Fix: Setting IsInTestState boolean to true for measurements in tests
MounikaBattu17 Aug 8, 2023
4632765
Fix: Killed Discovery Service which was started by Test environment
MounikaBattu17 Aug 8, 2023
0bc7ea2
Fix: Delete globaltestingstate.py
MounikaBattu17 Aug 9, 2023
58aa0f8
Feat: Added a generator class discovery service process
MounikaBattu17 Aug 9, 2023
7212665
Feat: Added generator fixture for yeilding discovery service process
MounikaBattu17 Aug 9, 2023
73c194d
Fix: Returned _discovery_service_subprocess
MounikaBattu17 Aug 9, 2023
7dbb932
Fix: Updated tests to use generator fixture
MounikaBattu17 Aug 9, 2023
0b5e74a
Merge branch 'main' into users/mounika/tests-fix-resoure-warning
MounikaBattu17 Aug 9, 2023
fb468f2
Fix: Update reurn types in Context manager discovery_service_process.py
MounikaBattu17 Aug 9, 2023
3225c0e
Fix: Import Self from typing_extensions
MounikaBattu17 Aug 9, 2023
9c7dd06
Fix: Removed throw away variable assignments
MounikaBattu17 Aug 10, 2023
d52160d
Test: Remove the context manager call
MounikaBattu17 Aug 10, 2023
044fd2f
Test: Don't call discovery_service_process
MounikaBattu17 Aug 10, 2023
3da8595
Revert: Reverted back test changes
MounikaBattu17 Aug 10, 2023
102a028
Test: Check if stub is None or not
MounikaBattu17 Aug 10, 2023
3e81232
Revert: Verified stub is actually None
MounikaBattu17 Aug 10, 2023
822594a
Test: Don't raise exception
MounikaBattu17 Aug 10, 2023
e839281
Fix: Removed throw away varaible assignments
MounikaBattu17 Aug 10, 2023
4bf3a7f
Revert: Raise Exception for Discovery Service
MounikaBattu17 Aug 10, 2023
300c526
Fix: Skipped tests for non-windows & no measurement registration file
MounikaBattu17 Aug 14, 2023
0063147
Fix: Correct the if condition
MounikaBattu17 Aug 14, 2023
6589f60
Fix: Use Pathlib instead of os
MounikaBattu17 Aug 14, 2023
ce9c6f7
Feat: Dont start measurement on unsuccessful register with discovery …
MounikaBattu17 Aug 16, 2023
4f24947
Feat: Added tests to cover the exception cases
MounikaBattu17 Aug 16, 2023
5841501
Feat: Added expect_discovery_service_stub_error marker in pyproject.toml
MounikaBattu17 Aug 16, 2023
7f80f4a
Merge branch 'main' into users/mounika/improve-error-handling
MounikaBattu17 Aug 16, 2023
6953af6
Fix: Merge errors
MounikaBattu17 Aug 16, 2023
f06592c
Fix: Merge errors
MounikaBattu17 Aug 16, 2023
a8c27ba
Test: Fix Mypy error: return nothing instead of None
MounikaBattu17 Aug 16, 2023
bf8e8d4
Test: Mypy error: Don't specify return type
MounikaBattu17 Aug 16, 2023
31b0ab2
Fix: Updated test case name
MounikaBattu17 Aug 16, 2023
f6e4989
Fix: Update test case name
MounikaBattu17 Aug 16, 2023
99f258c
Refractor: if else in start method in service_manager.py
MounikaBattu17 Aug 16, 2023
c9189d6
Refractor: Used Union to specify different return types
MounikaBattu17 Aug 16, 2023
bae1649
Refractor: Naming changes, if else
MounikaBattu17 Aug 16, 2023
e15eec9
Refractor: assert condition
MounikaBattu17 Aug 16, 2023
ecffed0
Fix: Lint errors
MounikaBattu17 Aug 16, 2023
00a7200
Refractor: Use Optional instead of Union
MounikaBattu17 Aug 16, 2023
2839c8c
Fix: Updated exceptions to be raised from discovery_client.py
MounikaBattu17 Aug 17, 2023
150599c
Fix: Updated caller to catch the expections from called methods
MounikaBattu17 Aug 17, 2023
3fdb8c6
Fix: Test updates on error handling
MounikaBattu17 Aug 17, 2023
854b5a8
Revert: Removed new marker in pyproject.toml
MounikaBattu17 Aug 17, 2023
328ae7f
Fix: Updated raise with no arguments
MounikaBattu17 Aug 21, 2023
3c1ed92
Fix: Don't catch the inner exceptions
MounikaBattu17 Aug 21, 2023
f0decea
Fix: FakeDiscoveryServiceError
MounikaBattu17 Aug 21, 2023
1ab02bf
Fix: Lint errors
MounikaBattu17 Aug 21, 2023
c359015
Fix: Don't catch inner exceptions in stop method
MounikaBattu17 Aug 21, 2023
1185d8d
Fix: Spacing
MounikaBattu17 Aug 21, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions ni_measurementlink_service/_internal/discovery_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,15 @@ def register_measurement_service(
)
else:
_logger.exception("Error in registering with discovery service.")
return False
raise
except FileNotFoundError:
_logger.error(
"Unable to register with discovery service. Possible reason: discovery service not running."
)
return False
raise
except Exception:
_logger.exception("Error in registering with discovery service.")
return False
raise
return True

def unregister_service(self) -> bool:
Expand Down Expand Up @@ -172,15 +172,15 @@ def unregister_service(self) -> bool:
)
else:
_logger.exception("Error in unregistering with discovery service.")
return False
raise
except FileNotFoundError:
_logger.error(
"Unable to unregister with discovery service. Possible reason: discovery service not running."
)
return False
raise
except Exception:
_logger.exception("Error in unregistering with discovery service.")
return False
raise
return True

def resolve_service(self, provided_interface: str, service_class: str = "") -> ServiceLocation:
Expand Down
36 changes: 32 additions & 4 deletions tests/integration/test_service_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@
measurement_service_pb2_grpc,
)
from tests.utilities import loopback_measurement
from tests.utilities.fake_discovery_service import FakeDiscoveryServiceStub
from tests.utilities.fake_discovery_service import (
FakeDiscoveryServiceError,
FakeDiscoveryServiceStub,
FakeDiscoveryServiceStubError,
)


def test___grpc_service___start_service___service_hosted(grpc_service: GrpcService):
Expand Down Expand Up @@ -43,6 +47,20 @@ def test___grpc_service_without_discovery_service___start_service___service_host
_validate_if_service_running_by_making_rpc(port_number)


@pytest.mark.parametrize("expect_discovery_service_error_stub", [True])
def test___grpc_service___start_service_error_registering_measurement___raises_error(
grpc_service: GrpcService,
):
with pytest.raises(FakeDiscoveryServiceError):
grpc_service.start(
loopback_measurement.measurement_service.measurement_info,
loopback_measurement.measurement_service.service_info,
loopback_measurement.measurement_service.configuration_parameter_list,
loopback_measurement.measurement_service.output_parameter_list,
loopback_measurement.measurement_service.measure_function,
)


def test___grpc_service_started___stop_service___service_stopped(grpc_service: GrpcService):
port_number = grpc_service.start(
loopback_measurement.measurement_service.measurement_info,
Expand Down Expand Up @@ -71,9 +89,19 @@ def discovery_client(discovery_service_stub: FakeDiscoveryServiceStub) -> Discov


@pytest.fixture
def discovery_service_stub() -> FakeDiscoveryServiceStub:
"""Create a FakeDiscoveryServiceStub."""
return FakeDiscoveryServiceStub()
def discovery_service_stub(expect_discovery_service_error_stub: bool) -> FakeDiscoveryServiceStub:
"""Create a valid/error stub based on expect_discovery_service_error_stub value."""
return (
FakeDiscoveryServiceStubError()
if expect_discovery_service_error_stub
else FakeDiscoveryServiceStub()
)


@pytest.fixture
def expect_discovery_service_error_stub() -> bool:
"""Boolean to choose between FakeDiscoveryServiceStub and FakeDiscoveryServiceStubError."""
return False


def _validate_if_service_running_by_making_rpc(port_number):
Expand Down
11 changes: 5 additions & 6 deletions tests/unit/test_discovery_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ def test___start_discovery_service___key_file_exist_after_poll___service_start_s
)


def test___discovery_service_exe_unavailable___register_service___registration_failure(
def test___discovery_service_exe_unavailable___register_service___raises_file_not_found_error(
mocker: MockerFixture,
temp_discovery_key_file_path: pathlib.Path,
temp_registration_json_file_path: pathlib.Path,
Expand All @@ -181,13 +181,12 @@ def test___discovery_service_exe_unavailable___register_service___registration_f
return_value=temp_registration_json_file_path,
)
mocker.patch("subprocess.Popen", side_effect=FileNotFoundError)

discovery_client = DiscoveryClient()
registration_success_flag = discovery_client.register_measurement_service(
_TEST_SERVICE_PORT, _TEST_SERVICE_INFO, _TEST_MEASUREMENT_INFO
)

assert not registration_success_flag
with pytest.raises(FileNotFoundError):
discovery_client.register_measurement_service(
_TEST_SERVICE_PORT, _TEST_SERVICE_INFO, _TEST_MEASUREMENT_INFO
)


@pytest.fixture(scope="module")
Expand Down
15 changes: 15 additions & 0 deletions tests/unit/test_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from typing import List, Type

import pytest
from pytest_mock import MockerFixture

from ni_measurementlink_service.measurement.info import DataType, TypeSpecialization
from ni_measurementlink_service.measurement.service import MeasurementService
Expand Down Expand Up @@ -350,6 +351,20 @@ def test___measurement_service___add_configuration_with_invalid_enum_type___rais
)


def test___measurement_service___host_service_with_grpc_service_not_started___raises_error(
measurement_service: MeasurementService,
mocker: MockerFixture,
):
measurement_service.register_measurement(_fake_measurement_function)
mocker.patch(
"ni_measurementlink_service._internal.service_manager.GrpcService.start",
side_effect=Exception,
)

with pytest.raises(Exception):
measurement_service.host_service()


@pytest.fixture
def measurement_service(test_assets_directory: pathlib.Path) -> MeasurementService:
"""Create a MeasurementService."""
Expand Down
10 changes: 8 additions & 2 deletions tests/utilities/fake_discovery_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,14 @@ class FakeDiscoveryServiceStubError(FakeDiscoveryServiceStub):

def RegisterService(self, request): # noqa: N802 - function name should be lowercase
"""Fake gRPC registration call to discovery service."""
raise Exception("TestException")
raise FakeDiscoveryServiceError("TestException")

def UnregisterService(self, request): # noqa: N802 - function name should be lowercase
"""Fake gRPC un-registration call to discovery service."""
raise Exception("TestException")
raise FakeDiscoveryServiceError("TestException")


class FakeDiscoveryServiceError(Exception):
"""Fake discovery service error to mimic the exceptions thrown from discovery service."""

pass