From 28b344020b0e81c560d22755b0515d4ece086760 Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Wed, 19 Jun 2024 15:45:40 +0000 Subject: [PATCH 1/9] Initial refactor of SystemRepo unit tests #90 --- test/unit/repositories/conftest.py | 12 - test/unit/repositories/test_system.py | 1318 +++++++++++++------------ 2 files changed, 663 insertions(+), 667 deletions(-) diff --git a/test/unit/repositories/conftest.py b/test/unit/repositories/conftest.py index 5e00a90f..8a582f3e 100644 --- a/test/unit/repositories/conftest.py +++ b/test/unit/repositories/conftest.py @@ -16,7 +16,6 @@ from inventory_management_system_api.repositories.catalogue_item import CatalogueItemRepo from inventory_management_system_api.repositories.item import ItemRepo from inventory_management_system_api.repositories.manufacturer import ManufacturerRepo -from inventory_management_system_api.repositories.system import SystemRepo from inventory_management_system_api.repositories.unit import UnitRepo from inventory_management_system_api.repositories.usage_status import UsageStatusRepo @@ -80,17 +79,6 @@ def fixture_manufacturer_repository(database_mock: Mock) -> ManufacturerRepo: return ManufacturerRepo(database_mock) -@pytest.fixture(name="system_repository") -def fixture_system_repository(database_mock: Mock) -> SystemRepo: - """ - Fixture to create a `SystemRepo` instance with a mocked Database dependency. - - :param database_mock: Mocked MongoDB database instance. - :return: `SystemRepo` instance with the mocked dependency. - """ - return SystemRepo(database_mock) - - @pytest.fixture(name="unit_repository") def fixture_unit_repository(database_mock: Mock) -> UnitRepo: """ diff --git a/test/unit/repositories/test_system.py b/test/unit/repositories/test_system.py index d9bdeac3..74bbeb51 100644 --- a/test/unit/repositories/test_system.py +++ b/test/unit/repositories/test_system.py @@ -2,15 +2,15 @@ Unit tests for the `SystemRepo` repository """ -from test.unit.repositories.mock_models import MOCK_CREATED_MODIFIED_TIME +from test.unit.repositories.conftest import RepositoryTestHelpers from test.unit.repositories.test_utils import ( MOCK_BREADCRUMBS_QUERY_RESULT_LESS_THAN_MAX_LENGTH, MOCK_MOVE_QUERY_RESULT_INVALID, MOCK_MOVE_QUERY_RESULT_VALID, ) - +from test.unit.services.test_item import ITEM_INFO from typing import Optional -from unittest.mock import MagicMock, call, patch +from unittest.mock import MagicMock, Mock, call, patch import pytest from bson import ObjectId @@ -24,7 +24,7 @@ MissingRecordError, ) from inventory_management_system_api.models.system import SystemIn, SystemOut - +from inventory_management_system_api.repositories.system import SystemRepo SYSTEM_A_INFO = { "parent_id": None, @@ -47,756 +47,764 @@ } -def _test_list(test_helpers, database_mock, system_repository, parent_id: Optional[str]): - """ - Utility method that tests getting Systems - - Verifies that the `list` method properly handles the retrieval of systems with the given filters - """ - # pylint: disable=duplicate-code - system_a = SystemOut(id=str(ObjectId()), **SYSTEM_A_INFO, **MOCK_CREATED_MODIFIED_TIME) - system_b = SystemOut(id=str(ObjectId()), **SYSTEM_B_INFO, **MOCK_CREATED_MODIFIED_TIME) - session = MagicMock() - # pylint: enable=duplicate-code - - # Mock `find` to return a list of System documents - test_helpers.mock_find( - database_mock.systems, - [ - {"_id": CustomObjectId(system_a.id), **SYSTEM_A_INFO, **MOCK_CREATED_MODIFIED_TIME}, - {"_id": CustomObjectId(system_b.id), **SYSTEM_B_INFO, **MOCK_CREATED_MODIFIED_TIME}, - ], - ) - - retrieved_systems = system_repository.list(parent_id, session=session) - - expected_filters = {} - if parent_id: - expected_filters["parent_id"] = None if parent_id == "null" else ObjectId(parent_id) - - database_mock.systems.find.assert_called_once_with(expected_filters, session=session) - assert retrieved_systems == [system_a, system_b] - - -def test_create(test_helpers, database_mock, system_repository): - """ - Test creating a System - - Verify that the `create` method properly handles the System to be created, checks that there is not - a duplicate System, and creates the System - """ - # pylint: disable=duplicate-code - system_in = SystemIn( - **{ - **SYSTEM_A_INFO, - "parent_id": None, - } - ) - system_info = system_in.model_dump() - system_out = SystemOut(id=str(ObjectId()), **system_info) - session = MagicMock() - # pylint: enable=duplicate-code - - # Mock `find_one` to return no duplicate system found in parent system - test_helpers.mock_find_one(database_mock.systems, None) - # Mock `insert_one` to return an object for the inserted system document - test_helpers.mock_insert_one(database_mock.systems, CustomObjectId(system_out.id)) - # Mock `find_one` to return the inserted system document - test_helpers.mock_find_one( - database_mock.systems, - {**system_info, "_id": CustomObjectId(system_out.id)}, - ) - - created_system = system_repository.create(system_in, session=session) - - database_mock.systems.insert_one.assert_called_once_with(system_info, session=session) - assert created_system == system_out - - -def test_create_with_parent_id(test_helpers, database_mock, system_repository): - """ - Test creating a System with a parent ID - - Verify that the `create` method properly handles the creation of a System with a parent ID - """ - # pylint: disable=duplicate-code - system_in = SystemIn( - **{ - **SYSTEM_A_INFO, - "parent_id": str(ObjectId()), - } - ) - system_info = system_in.model_dump() - system_out = SystemOut(id=str(ObjectId()), **system_info) - session = MagicMock() - - # Mock `find_one` to return the parent system document - test_helpers.mock_find_one( - database_mock.systems, - {**system_info, "_id": CustomObjectId(system_out.parent_id), "parent_id": None}, - ) - # pylint: enable=duplicate-code - # Mock `find_one` to return no duplicate system found in parent system - test_helpers.mock_find_one(database_mock.systems, None) - # Mock `insert_one` to return an object for the inserted system document - test_helpers.mock_insert_one(database_mock.systems, CustomObjectId(system_out.id)) - # Mock `find_one` to return the inserted system document - test_helpers.mock_find_one( - database_mock.systems, - {**system_info, "_id": CustomObjectId(system_out.id)}, - ) - - created_system = system_repository.create(system_in, session=session) - - database_mock.systems.insert_one.assert_called_once_with( - {**system_info, "parent_id": CustomObjectId(system_out.parent_id)}, session=session - ) - database_mock.systems.find_one.assert_has_calls( - [ - call({"_id": CustomObjectId(system_out.parent_id)}, session=session), +class SystemRepoDSL: + """Base class for SystemRepo unit tests""" + + test_helpers: RepositoryTestHelpers + mock_database: Mock + mock_utils: Mock + system_repository: SystemRepo + systems_collection: Mock + items_collection: Mock + + mock_session = MagicMock() + + @pytest.fixture(autouse=True) + def setup(self, test_helpers, database_mock): + """Setup fixtures""" + + self.test_helpers = test_helpers + self.mock_database = database_mock + self.system_repository = SystemRepo(database_mock) + self.systems_collection = database_mock.systems + self.items_collection = database_mock.items + + self.mock_session = MagicMock() + + with patch("inventory_management_system_api.repositories.system.utils") as mock_utils: + self.mock_utils = mock_utils + yield + + +class CreateDSL(SystemRepoDSL): + """Base class for create tests""" + + _system_in: SystemIn + _expected_system_out: SystemOut + _created_system: SystemOut + _create_exception: pytest.ExceptionInfo + + def mock_create( + self, + system_data: dict, + parent_system_data: Optional[dict] = None, + duplicate_system_data: Optional[dict] = None, + ): + """Mocks database methods appropriately to test the 'create' repo method + + :param system_data: Dictionary containing the basic system information (excluding id or creation and modified + times as these will be automatically handled) + :param parent_system_data: Either None or a dictionary containing the basic parent system data + :param duplicate_system_data: Either None or a dictionary containing basic system data for a duplicate system + """ + inserted_system_id = CustomObjectId(str(ObjectId())) + + # Pass through system_in first as need creation and modified times + self._system_in = SystemIn(**system_data) + + self._expected_system_out = SystemOut(**self._system_in.model_dump(), id=inserted_system_id) + + # When a parent_id is given, need to mock the find_one for it too + if system_data["parent_id"]: + # If parent_system_data is given as none, then it is intentionally supposed to be, otherwise + # pass through SystemIn first to ensure it has creation and modified times + self.test_helpers.mock_find_one( + self.systems_collection, + ( + {**SystemIn(**parent_system_data).model_dump(), "_id": system_data["parent_id"]} + if parent_system_data + else None + ), + ) + self.test_helpers.mock_find_one( + self.systems_collection, + {**SystemIn(**duplicate_system_data).model_dump(), "_id": ObjectId()} if duplicate_system_data else None, + ) + self.test_helpers.mock_insert_one(self.systems_collection, inserted_system_id) + self.test_helpers.mock_find_one( + self.systems_collection, {**self._system_in.model_dump(), "_id": inserted_system_id} + ) + + def call_create(self): + """Calls the SystemRepo `create` method with the appropriate data from a prior call to `mock_create`""" + + self._created_system = self.system_repository.create(self._system_in, session=self.mock_session) + + def call_create_expecting_error(self, error_type: type): + """Calls the SystemRepo `create` method with the appropriate data from a prior call to `mock_create` + while expecting an error to be raised""" + + with pytest.raises(error_type) as exc: + self.system_repository.create(self._system_in) + self._create_exception = exc + + def check_create_success(self): + """Checks that a prior call to `call_create` worked as expected""" + + system_in_data = self._system_in.model_dump() + + # Obtain a list of expected find_one calls + expected_find_one_calls = [] + # This is the check for parent existence + if self._system_in.parent_id: + expected_find_one_calls.append(call({"_id": self._system_in.parent_id}, session=self.mock_session)) + # Also need checks for duplicate and the final newly inserted system get + expected_find_one_calls.append( call( { - "parent_id": CustomObjectId(system_out.parent_id), - "code": system_out.code, + "parent_id": self._system_in.parent_id, + "code": self._system_in.code, "_id": {"$ne": None}, }, - session=session, - ), - call({"_id": CustomObjectId(system_out.id)}, session=session), - ] - ) - assert created_system == system_out - - -def test_create_with_non_existent_parent_id(test_helpers, database_mock, system_repository): - """ - Test creating a System with a non-existent parent ID - - Verify that the `create` method properly handles a System with a non-existent parent ID - and does not create it - """ - # pylint: disable=duplicate-code - system_in = SystemIn( - **{ - **SYSTEM_A_INFO, - "parent_id": str(ObjectId()), - } - ) - system_info = system_in.model_dump() - system_out = SystemOut(id=str(ObjectId()), **system_info) - # pylint: enable=duplicate-code - - # Mock `find_one` to not return a parent system document - test_helpers.mock_find_one(database_mock.systems, None) - - with pytest.raises(MissingRecordError) as exc: - system_repository.create(system_in) - - database_mock.systems.insert_one.assert_not_called() - assert str(exc.value) == f"No parent System found with ID: {system_out.parent_id}" - - -def test_create_with_duplicate_name_within_parent(test_helpers, database_mock, system_repository): - """ - Test creating a System with a duplicate name within the parent System - - Verify that the `create` method properly handles a System with a duplicate name - and does not create it - """ - # pylint: disable=duplicate-code - system_in = SystemIn( - **{ - **SYSTEM_A_INFO, - "parent_id": str(ObjectId()), - } - ) - system_info = system_in.model_dump() - system_out = SystemOut(id=str(ObjectId()), **system_info) - # pylint: enable=duplicate-code - - # Mock `find_one` to return the parent system document - test_helpers.mock_find_one( - database_mock.systems, - {**system_info, "_id": CustomObjectId(system_out.parent_id), "parent_id": str(ObjectId())}, - ) - # Mock `find_one` to return duplicate system found in parent system - test_helpers.mock_find_one( - database_mock.systems, - { - **system_info, - "_id": ObjectId(), - }, - ) - - with pytest.raises(DuplicateRecordError) as exc: - system_repository.create(system_in) - - database_mock.systems.insert_one.assert_not_called() - assert str(exc.value) == "Duplicate System found within the parent System" - - -def test_get(test_helpers, database_mock, system_repository): - """ - Test getting a System - - Verify that the `get` method properly handles the retrieval of a System by ID - """ - system_out = SystemOut(id=str(ObjectId()), **SYSTEM_A_INFO, **MOCK_CREATED_MODIFIED_TIME) - session = MagicMock() - - # Mock `find_one` to return a system - test_helpers.mock_find_one( - database_mock.systems, - {**SYSTEM_A_INFO, **MOCK_CREATED_MODIFIED_TIME, "_id": CustomObjectId(system_out.id)}, - ) - - retrieved_system = system_repository.get(system_out.id, session=session) - - database_mock.systems.find_one.assert_called_with({"_id": CustomObjectId(system_out.id)}, session=session) - assert retrieved_system == system_out - - -def test_get_with_invalid_id(database_mock, system_repository): - """ - Test getting a System with an invalid ID - - Verify that the `get` method properly handles the retrieval of a System with an invalid ID - """ + session=self.mock_session, + ) + ) + expected_find_one_calls.append( + call( + {"_id": CustomObjectId(self._expected_system_out.id)}, + session=self.mock_session, + ) + ) + self.systems_collection.find_one.assert_has_calls(expected_find_one_calls) - with pytest.raises(InvalidObjectIdError) as exc: - system_repository.get("invalid") - database_mock.systems.find_one.assert_not_called() - assert str(exc.value) == "Invalid ObjectId value 'invalid'" + self.systems_collection.insert_one.assert_called_once_with(system_in_data, session=self.mock_session) + assert self._created_system == self._expected_system_out + def check_create_failed_with_exception(self, message: str): + """Checks that a prior call to `call_create_expecting_error` worked as expected, raising an exception + with the correct message""" -def test_get_with_non_existent_id(test_helpers, database_mock, system_repository): - """ - Test getting a System with a non-existent ID - - Verify that the `get` method properly handles the retrieval of a System with a non-existent ID - """ - system_id = str(ObjectId()) - session = MagicMock() - - # Mock `find_one` to not return a system document - test_helpers.mock_find_one(database_mock.systems, None) - - retrieved_system = system_repository.get(system_id, session=session) + self.systems_collection.insert_one.assert_not_called() - database_mock.systems.find_one.assert_called_with({"_id": CustomObjectId(system_id)}, session=session) - assert retrieved_system is None + assert str(self._create_exception.value) == message -@patch("inventory_management_system_api.repositories.system.utils") -def test_get_breadcrumbs(mock_utils, database_mock, system_repository): - """ - Test getting breadcrumbs for a specific system +class TestCreate(CreateDSL): + """Tests for creating a System""" - Verify that the 'get_breadcrumbs' method properly handles the retrieval of breadcrumbs for a system - """ - system_id = str(ObjectId()) - mock_aggregation_pipeline = MagicMock() - mock_breadcrumbs = MagicMock() - session = MagicMock() + def test_create(self): + """Test creating a System""" - mock_utils.create_breadcrumbs_aggregation_pipeline.return_value = mock_aggregation_pipeline - mock_utils.compute_breadcrumbs.return_value = mock_breadcrumbs - database_mock.systems.aggregate.return_value = MOCK_BREADCRUMBS_QUERY_RESULT_LESS_THAN_MAX_LENGTH + self.mock_create(SYSTEM_A_INFO) + self.call_create() + self.check_create_success() - retrieved_breadcrumbs = system_repository.get_breadcrumbs(system_id, session=session) + def test_create_with_parent_id(self): + """Test creating a System with a valid parent_id""" - database_mock.systems.aggregate.assert_called_once_with(mock_aggregation_pipeline, session=session) - mock_utils.create_breadcrumbs_aggregation_pipeline.assert_called_once_with( - entity_id=system_id, collection_name="systems" - ) - mock_utils.compute_breadcrumbs.assert_called_once_with( - list(MOCK_BREADCRUMBS_QUERY_RESULT_LESS_THAN_MAX_LENGTH), - entity_id=system_id, - collection_name="systems", - ) - assert retrieved_breadcrumbs == mock_breadcrumbs + self.mock_create({**SYSTEM_A_INFO, "parent_id": str(ObjectId())}, parent_system_data=SYSTEM_B_INFO) + self.call_create() + self.check_create_success() + def test_create_with_non_existent_parent_id(self): + """Test creating a System with a non-existent parent_id""" -def test_list(test_helpers, database_mock, system_repository): - """ - Test getting Systems + parent_id = str(ObjectId()) - Verify that the `list` method properly handles the retrieval of systems without filters - """ - _test_list(test_helpers, database_mock, system_repository, None) + self.mock_create({**SYSTEM_A_INFO, "parent_id": parent_id}, parent_system_data=None) + self.call_create_expecting_error(MissingRecordError) + self.check_create_failed_with_exception(f"No parent System found with ID: {parent_id}") + def test_create_with_duplicate_name_within_parent(self): + """Test creating a System with a duplicate system being found in the same parent system""" -def test_list_with_parent_id_filter(test_helpers, database_mock, system_repository): - """ - Test getting Systems based on the provided parent_id filter + self.mock_create( + {**SYSTEM_A_INFO, "parent_id": str(ObjectId())}, + parent_system_data=SYSTEM_B_INFO, + duplicate_system_data=SYSTEM_B_INFO, + ) + self.call_create_expecting_error(DuplicateRecordError) + self.check_create_failed_with_exception("Duplicate System found within the parent System") - Verify that the `list` method properly handles the retrieval of systems based on the provided parent - parent_id filter - """ - _test_list(test_helpers, database_mock, system_repository, str(ObjectId())) +class GetDSL(SystemRepoDSL): + """Base class for get tests""" -def test_list_with_null_parent_id_filter(test_helpers, database_mock, system_repository): - """ - Test getting Systems when the provided parent_id filter is "null" + _obtained_system_id: str + _expected_system_out: SystemOut + _obtained_system: Optional[SystemOut] + _get_exception: pytest.ExceptionInfo - Verify that the `list` method properly handles the retrieval of systems based on the provided - parent_id filter - """ - _test_list(test_helpers, database_mock, system_repository, "null") + def mock_get(self, system_id: str, system_data: Optional[dict]): + """Mocks database methods appropriately to test the 'get' repo method + :param system_id: ID of the system that will be obtained + :param system_data: Dictionary containing the basic system information (excluding id or creation and modified + times as these will be automatically handled) + """ -def test_list_with_parent_id_filter_no_matching_results(test_helpers, database_mock, system_repository): - """ - Test getting Systems based on the provided parent_id filter when there are no matching results - in the database + self._expected_system_out = ( + SystemOut(**SystemIn(**system_data).model_dump(), id=CustomObjectId(system_id)) if system_data else None + ) - Verify that the `list` method properly handles the retrieval of systems based on the provided - parent_id filter when there are no matching results in the database - """ - session = MagicMock() + self.test_helpers.mock_find_one( + self.systems_collection, self._expected_system_out.model_dump() if self._expected_system_out else None + ) - # Mock `find` to return a list of System documents - test_helpers.mock_find(database_mock.systems, []) + def call_get(self, system_id: str): + """Calls the SystemRepo `get` method with the appropriate data from a prior call to `mock_get`""" - parent_id = ObjectId() - retrieved_systems = system_repository.list(str(parent_id), session=session) + self._obtained_system_id = system_id + self._obtained_system = self.system_repository.get(system_id, session=self.mock_session) - database_mock.systems.find.assert_called_once_with({"parent_id": parent_id}, session=session) - assert retrieved_systems == [] + def call_get_expecting_error(self, system_id: str, error_type: type): + """Calls the SystemRepo `get` method with the appropriate data from a prior call to `mock_get` + while expecting an error to be raised""" + with pytest.raises(error_type) as exc: + self.system_repository.get(system_id) + self._get_exception = exc -# pylint:disable=W0613 -def test_list_with_invalid_parent_id_filter(test_helpers, database_mock, system_repository): - """ - Test getting Systems when given an invalid parent_id to filter on + def check_get_success(self): + """Checks that a prior call to `get_system` worked as expected""" - Verify that the `list` method properly handles the retrieval of systems when given an invalid parent_id - filter - """ - with pytest.raises(InvalidObjectIdError) as exc: - system_repository.list("invalid") - database_mock.systems.find.assert_not_called() - assert str(exc.value) == "Invalid ObjectId value 'invalid'" + self.systems_collection.find_one.assert_called_once_with( + {"_id": CustomObjectId(self._obtained_system_id)}, session=self.mock_session + ) + assert self._obtained_system == self._expected_system_out + def check_get_failed_with_exception(self, message: str): + """Checks that a prior call to `call_create_expecting_error` worked as expected, raising an exception + with the correct message""" -@patch("inventory_management_system_api.repositories.system.utils") -def test_update(utils_mock, test_helpers, database_mock, system_repository): - """ - Test updating a System + self.systems_collection.find_one.assert_not_called() - Verify that the `update` method properly handles the update of a System - """ - system = SystemOut(id=str(ObjectId()), **SYSTEM_A_INFO, **MOCK_CREATED_MODIFIED_TIME) - session = MagicMock() + assert str(self._get_exception.value) == message - # Mock `find_one` to return the stored System document - test_helpers.mock_find_one( - database_mock.systems, - system.model_dump(), - ) - # Mock `update_one` to return an object for the updated System document - test_helpers.mock_update_one(database_mock.systems) +class TestGet(GetDSL): + """Tests for getting a System""" - # Mock `find_one` to return the updated System document - system_in = SystemIn(**SYSTEM_A_INFO, **MOCK_CREATED_MODIFIED_TIME) - test_helpers.mock_find_one(database_mock.systems, {**system_in.model_dump(), "_id": CustomObjectId(system.id)}) + def test_get(self): + """Test getting a System""" - updated_system = system_repository.update(system.id, system_in, session=session) + system_id = str(ObjectId()) - utils_mock.create_breadcrumbs_aggregation_pipeline.assert_not_called() - utils_mock.is_valid_move_result.assert_not_called() + self.mock_get(system_id, SYSTEM_A_INFO) + self.call_get(system_id) + self.check_get_success() - database_mock.systems.update_one.assert_called_once_with( - { - "_id": CustomObjectId(system.id), - }, - { - "$set": { - **system_in.model_dump(), - }, - }, - session=session, - ) - database_mock.systems.find_one.assert_has_calls( - [ - call({"_id": CustomObjectId(system.id)}, session=session), - call({"_id": CustomObjectId(system.id)}, session=session), - ] - ) - assert updated_system == SystemOut(id=system.id, **system_in.model_dump()) - - -@patch("inventory_management_system_api.repositories.system.utils") -def test_update_parent_id(utils_mock, test_helpers, database_mock, system_repository): - """ - Test updating a System's parent_id - - Verify that the `update` method properly handles the update of a System when the parent id changes - """ - parent_system_id = str(ObjectId()) - system = SystemOut( - id=str(ObjectId()), **{**SYSTEM_A_INFO, "parent_id": parent_system_id, **MOCK_CREATED_MODIFIED_TIME} - ) - session = MagicMock() - new_parent_id = str(ObjectId()) - - # Mock `find_one` to return a parent System document - test_helpers.mock_find_one( - database_mock.systems, - { - **SYSTEM_B_INFO, - **MOCK_CREATED_MODIFIED_TIME, - "_id": CustomObjectId(new_parent_id), - }, - ) - - # Mock `find_one` to return the stored System document - test_helpers.mock_find_one( - database_mock.systems, - system.model_dump(), - ) - # Mock `find_one` to return no duplicate systems found - test_helpers.mock_find_one(database_mock.systems, None) - # Mock `update_one` to return an object for the updated System document - test_helpers.mock_update_one(database_mock.systems) - # Mock `find_one` to return the updated System document - system_in = SystemIn(**{**SYSTEM_A_INFO, "parent_id": new_parent_id, **MOCK_CREATED_MODIFIED_TIME}) - test_helpers.mock_find_one( - database_mock.systems, - {**system_in.model_dump(), "_id": CustomObjectId(system.id)}, - ) - - # Mock utils so not moving to a child of itself - mock_aggregation_pipeline = MagicMock() - utils_mock.create_move_check_aggregation_pipeline.return_value = mock_aggregation_pipeline - utils_mock.is_valid_move_result.return_value = True - database_mock.systems.aggregate.return_value = MOCK_MOVE_QUERY_RESULT_VALID - - updated_system = system_repository.update(system.id, system_in, session=session) - - utils_mock.create_move_check_aggregation_pipeline.assert_called_once_with( - entity_id=system.id, destination_id=new_parent_id, collection_name="systems" - ) - database_mock.systems.aggregate.assert_called_once_with(mock_aggregation_pipeline, session=session) - utils_mock.is_valid_move_result.assert_called_once() - - database_mock.systems.update_one.assert_called_once_with( - { - "_id": CustomObjectId(system.id), - }, - { - "$set": { - **system_in.model_dump(), - }, - }, - session=session, - ) - database_mock.systems.find_one.assert_has_calls( - [ - call({"_id": CustomObjectId(new_parent_id)}, session=session), - call({"_id": CustomObjectId(system.id)}, session=session), - call( - { - "parent_id": CustomObjectId(new_parent_id), - "code": system.code, - "_id": {"$ne": CustomObjectId(system.id)}, - }, - session=session, - ), - call({"_id": CustomObjectId(system.id)}, session=session), + def test_get_with_non_existent_id(self): + """Test getting a System with a non-existent ID""" + + system_id = str(ObjectId()) + + self.mock_get(system_id, None) + self.call_get(system_id) + self.check_get_success() + + def test_get_with_invalid_id(self): + """Test getting a System with an invalid ID""" + + system_id = "invalid-id" + + self.call_get_expecting_error(system_id, InvalidObjectIdError) + self.check_get_failed_with_exception("Invalid ObjectId value 'invalid-id'") + + +class GetBreadcrumbsDSL(SystemRepoDSL): + """Base class for breadcrumbs tests""" + + _breadcrumbs_query_result: dict + _mock_aggregation_pipeline = MagicMock() + _expected_breadcrumbs: MagicMock + _obtained_system_id: str + _obtained_breadcrumbs: MagicMock + + def mock_breadcrumbs(self, breadcrumbs_query_result: dict): + """Mocks database methods appropriately to test the 'get_breadcrumbs' repo method + + :param breadcrumbs_query_result: Dictionary containing the breadcrumbs query result from the aggregation + pipeline + """ + self._breadcrumbs_query_result = breadcrumbs_query_result + self._mock_aggregation_pipeline = MagicMock() + self._expected_breadcrumbs = MagicMock() + + self.mock_utils.create_breadcrumbs_aggregation_pipeline.return_value = self._mock_aggregation_pipeline + self.systems_collection.aggregate.return_value = breadcrumbs_query_result + self.mock_utils.compute_breadcrumbs.return_value = self._expected_breadcrumbs + + def call_get_breadcrumbs(self, system_id: str): + """Calls the SystemRepo `get_breadcrumbs` method""" + + self._obtained_system_id = system_id + self._obtained_breadcrumbs = self.system_repository.get_breadcrumbs(system_id, session=self.mock_session) + + def check_get_breadcrumbs_success(self): + """Checks that a prior call to `call_get_breadcrumbs` worked as expected""" + + self.mock_utils.create_breadcrumbs_aggregation_pipeline.assert_called_once_with( + entity_id=self._obtained_system_id, collection_name="systems" + ) + self.systems_collection.aggregate.assert_called_once_with( + self._mock_aggregation_pipeline, session=self.mock_session + ) + self.mock_utils.compute_breadcrumbs.assert_called_once_with( + list(self._breadcrumbs_query_result), entity_id=self._obtained_system_id, collection_name="systems" + ) + + assert self._obtained_breadcrumbs == self._expected_breadcrumbs + + +class TestGetBreadcrumbs(GetBreadcrumbsDSL): + """Tests for getting the breadcrumbs of a System""" + + def test_get_system_breadcrumbs(self): + """Test getting a System's breadcrumbs""" + + self.mock_breadcrumbs(MOCK_BREADCRUMBS_QUERY_RESULT_LESS_THAN_MAX_LENGTH) + self.call_get_breadcrumbs(str(ObjectId())) + self.check_get_breadcrumbs_success() + + +class ListDSL(SystemRepoDSL): + """Base class for list tests""" + + _expected_systems_out: list[SystemOut] + _parent_id_filter: str + _obtained_systems_out: list[SystemOut] + + def mock_list(self, systems_data: list[dict]): + """Mocks database methods appropriately to test the 'list' repo method + + :param systems_data: List of system data to use within the mocked data""" + + self._expected_systems_out = [ + SystemOut(**SystemIn(**system_data).model_dump(), id=ObjectId()) for system_data in systems_data ] - ) - assert updated_system == SystemOut(id=system.id, **{**system_in.model_dump(), "parent_id": new_parent_id}) - - -@patch("inventory_management_system_api.repositories.system.utils") -def test_update_parent_id_moving_to_child(utils_mock, test_helpers, database_mock, system_repository): - """ - Test updating a System's parent_id when moving to a child of itself - - Verify that the `update` method properly handles the update of a System when the new parent_id - is a child of itself - """ - parent_system_id = str(ObjectId()) - system = SystemOut( - id=str(ObjectId()), **{**SYSTEM_A_INFO, "parent_id": parent_system_id, **MOCK_CREATED_MODIFIED_TIME} - ) - new_parent_id = str(ObjectId()) - - # Mock `find_one` to return a parent System document - test_helpers.mock_find_one( - database_mock.systems, - { - **SYSTEM_B_INFO, - **MOCK_CREATED_MODIFIED_TIME, - "_id": CustomObjectId(new_parent_id), - }, - ) - - # Mock `find_one` to return the stored System document - test_helpers.mock_find_one( - database_mock.systems, - system.model_dump(), - ) - # Mock `find_one` to return no duplicates found - test_helpers.mock_find_one(database_mock.systems, None) - # Mock `update_one` to return an object for the updated System document - test_helpers.mock_update_one(database_mock.systems) - # Mock `find_one` to return the updated System document - system_in = SystemIn(**{**SYSTEM_A_INFO, "parent_id": new_parent_id, **MOCK_CREATED_MODIFIED_TIME}) - test_helpers.mock_find_one( - database_mock.systems, - {**system_in.model_dump(), "_id": CustomObjectId(system.id)}, - ) - - # Mock utils so moving to a child of itself - mock_aggregation_pipeline = MagicMock() - utils_mock.create_move_check_aggregation_pipeline.return_value = mock_aggregation_pipeline - utils_mock.is_valid_move_result.return_value = False - database_mock.systems.aggregate.return_value = MOCK_MOVE_QUERY_RESULT_INVALID - - system_in = SystemIn(**{**SYSTEM_A_INFO, "parent_id": new_parent_id}) - - with pytest.raises(InvalidActionError) as exc: - system_repository.update(system.id, system_in) - assert str(exc.value) == "Cannot move a system to one of its own children" - - utils_mock.create_move_check_aggregation_pipeline.assert_called_once_with( - entity_id=system.id, destination_id=new_parent_id, collection_name="systems" - ) - database_mock.systems.aggregate.assert_called_once_with(mock_aggregation_pipeline, session=None) - utils_mock.is_valid_move_result.assert_called_once() - - database_mock.systems.update_one.assert_not_called() - database_mock.systems.find_one.assert_has_calls( - [ - call({"_id": CustomObjectId(new_parent_id)}, session=None), - call({"_id": CustomObjectId(system.id)}, session=None), + + self.test_helpers.mock_find( + self.systems_collection, [system_out.model_dump() for system_out in self._expected_systems_out] + ) + + def call_list(self, parent_id: Optional[str]): + """Calls the SystemRepo `list` method""" + + self._parent_id_filter = parent_id + + self._obtained_systems_out = self.system_repository.list(parent_id=parent_id, session=self.mock_session) + + def check_list_success(self): + """Checks that a prior call to 'call_list` worked as expected""" + + self.mock_utils.list_query.assert_called_once_with(self._parent_id_filter, "systems") + self.systems_collection.find.assert_called_once_with( + self.mock_utils.list_query.return_value, session=self.mock_session + ) + + assert self._obtained_systems_out == self._expected_systems_out + + +class TestList(ListDSL): + """Tests for listing System's""" + + def test_list(self): + """Test listing all Systems""" + + self.mock_list([SYSTEM_A_INFO, SYSTEM_B_INFO]) + self.call_list(parent_id=None) + self.check_list_success() + + def test_list_with_parent_id_filter(self): + """Test listing all Systems with a given parent_id""" + + self.mock_list([SYSTEM_A_INFO, SYSTEM_B_INFO]) + self.call_list(parent_id=str(ObjectId())) + self.check_list_success() + + def test_list_with_null_parent_id_filter(self): + """Test listing all Systems with a 'null' parent_id""" + + self.mock_list([SYSTEM_A_INFO, SYSTEM_B_INFO]) + self.call_list(parent_id="null") + self.check_list_success() + + def test_list_with_parent_id_with_no_results(self): + """Test listing all Systems with a parent_id filter returning no results""" + + self.mock_list([]) + self.call_list(parent_id=str(ObjectId())) + self.check_list_success() + + +class UpdateDSL(SystemRepoDSL): + """Base class for update tests""" + + # pylint:disable=too-many-instance-attributes + _system_in: SystemIn + _stored_system: SystemOut + _expected_system_out: SystemOut + _updated_system_id: str + _updated_system: SystemOut + _moving_system: bool + _update_exception: pytest.ExceptionInfo + + def set_update_data(self, new_system_data: dict): + """Assigns the update data to use during a call to `update_system` + + :param new_system_data: New system data to supply to the SystemRepo `update` method""" + self._system_in = SystemIn(**new_system_data) + + # pylint:disable=too-many-arguments + def mock_update( + self, + system_id: str, + new_system_data: dict, + stored_system_data: Optional[dict], + new_parent_system_data: Optional[dict] = None, + duplicate_system_data: Optional[dict] = None, + valid_move_result: bool = True, + ): + """Mocks database methods appropriately to test the 'update' repo method + + :param system_id: ID of the system that will be obtained + :param new_system_data: Dictionary containing the new basic system information (excluding id or creation and + modified times as these will be automatically handled) + :param stored_system_data: Dictionary containing the existing stored basic system information + :param new_parent_system_data: Either None or a dictionary containing the basic parent system data + :param duplicate_system_data: Either None or a dictionary containing basic system data for a duplicate system + :param valid_move_result: Whether to mock in a valid or invalid move result i.e. when True will simulating + moving the system one of its own children + """ + self.set_update_data(new_system_data) + + # When a parent_id is given, need to mock the find_one for it too + if new_system_data["parent_id"]: + # If new_parent_system_data is given as none, then it is intentionally supposed to be, otherwise + # pass through SystemIn first to ensure it has creation and modified times + self.test_helpers.mock_find_one( + self.systems_collection, + ( + {**SystemIn(**new_parent_system_data).model_dump(), "_id": new_system_data["parent_id"]} + if new_parent_system_data + else None + ), + ) + + # Stored system + self._stored_system = ( + SystemOut(**SystemIn(**stored_system_data).model_dump(), id=CustomObjectId(system_id)) + if stored_system_data + else None + ) + self.test_helpers.mock_find_one( + self.systems_collection, self._stored_system.model_dump() if self._stored_system else None + ) + + # Duplicate check + self._moving_system = stored_system_data and (new_system_data["parent_id"] != stored_system_data["parent_id"]) + if (stored_system_data and (self._system_in.name != self._stored_system.name)) or self._moving_system: + self.test_helpers.mock_find_one( + self.systems_collection, + ( + {**SystemIn(**duplicate_system_data).model_dump(), "_id": ObjectId()} + if duplicate_system_data + else None + ), + ) + + # Final system after update + self._expected_system_out = SystemOut(**self._system_in.model_dump(), id=CustomObjectId(system_id)) + self.test_helpers.mock_find_one(self.systems_collection, self._expected_system_out.model_dump()) + + if self._moving_system: + mock_aggregation_pipeline = MagicMock() + self.mock_utils.create_move_check_aggregation_pipeline.return_value = mock_aggregation_pipeline + if valid_move_result: + self.mock_utils.is_valid_move_result.return_value = True + self.systems_collection.aggregate.return_value = MOCK_MOVE_QUERY_RESULT_VALID + else: + self.mock_utils.is_valid_move_result.return_value = False + self.systems_collection.aggregate.return_value = MOCK_MOVE_QUERY_RESULT_INVALID + + def call_update(self, system_id: str): + """Calls the SystemRepo `update` method with the appropriate data from a prior call to `mock_update` (or + `set_update_data`)""" + + self._updated_system_id = system_id + self._updated_system = self.system_repository.update(system_id, self._system_in, session=self.mock_session) + + def call_update_expecting_error(self, system_id: str, error_type: type): + """Calls the SystemRepo `update` method with the appropriate data from a prior call to `mock_update` + while expecting an error to be raised""" + + with pytest.raises(error_type) as exc: + self.system_repository.update(system_id, self._system_in) + self._update_exception = exc + + def check_update_success(self): + """Checks that a prior call to `call_update` worked as expected""" + + # Obtain a list of expected find_one calls + expected_find_one_calls = [] + + # Parent existence check + if self._system_in.parent_id: + expected_find_one_calls.append(call({"_id": self._system_in.parent_id}, session=self.mock_session)) + + # Stored system + expected_find_one_calls.append( call( - { - "parent_id": CustomObjectId(new_parent_id), - "code": system.code, - "_id": {"$ne": CustomObjectId(system.id)}, + {"_id": CustomObjectId(self._expected_system_out.id)}, + session=self.mock_session, + ) + ) + + # Duplicate check (which only runs if moving or changing the name) + if (self._system_in.name != self._stored_system.name) or self._moving_system: + expected_find_one_calls.append( + call( + { + "parent_id": self._system_in.parent_id, + "code": self._system_in.code, + "_id": {"$ne": CustomObjectId(self._updated_system_id)}, + }, + session=self.mock_session, + ) + ) + self.systems_collection.find_one.assert_has_calls(expected_find_one_calls) + + if self._moving_system: + self.mock_utils.create_move_check_aggregation_pipeline.assert_called_once_with( + entity_id=self._updated_system_id, + destination_id=str(self._system_in.parent_id), + collection_name="systems", + ) + self.systems_collection.aggregate.assert_called_once_with( + self.mock_utils.create_move_check_aggregation_pipeline.return_value, session=self.mock_session + ) + + self.systems_collection.update_one.assert_called_once_with( + { + "_id": CustomObjectId(self._updated_system_id), + }, + { + "$set": { + **self._system_in.model_dump(), }, - session=None, - ), - ] - ) + }, + session=self.mock_session, + ) + assert self._updated_system == self._expected_system_out -def test_update_with_invalid_id(database_mock, system_repository): - """ - Test updating a System with an invalid ID + def check_update_failed_with_exception(self, message: str): + """Checks that a prior call to `call_update_expecting_error` worked as expected, raising an exception + with the correct message""" - Verify that the `update` method properly handles the update of a System with an invalid ID - """ - system_id = "invalid" + self.systems_collection.update_one.assert_not_called() - with pytest.raises(InvalidObjectIdError) as exc: - system_repository.update(system_id, MagicMock()) - assert str(exc.value) == f"Invalid ObjectId value '{system_id}'" + assert str(self._update_exception.value) == message - database_mock.systems.update_one.assert_not_called() - database_mock.systems.find_one.assert_not_called() +class TestUpdate(UpdateDSL): + """Tests for updating a System""" -def test_update_with_non_existent_parent_id(test_helpers, database_mock, system_repository): - """ - Test updating a System with a non-existent parent ID + def test_update(self): + """Test updating a System""" - Verify that the `update` method properly handles the update of a System with a non-existent parent ID - """ - system = SystemIn(**{**SYSTEM_A_INFO, "parent_id": str(ObjectId())}) - system_id = str(ObjectId()) + system_id = str(ObjectId()) - # Mock `find_one` to not return a parent System document - test_helpers.mock_find_one(database_mock.systems, None) + self.mock_update(system_id, SYSTEM_A_INFO, SYSTEM_B_INFO) + self.call_update(system_id) + self.check_update_success() - with pytest.raises(MissingRecordError) as exc: - system_repository.update(system_id, system) - assert str(exc.value) == f"No parent System found with ID: {system.parent_id}" + def test_update_with_invalid_id(self): + """Test updating a System with an invalid id""" - database_mock.systems.update_one.assert_not_called() - database_mock.systems.find_one.assert_called_once_with({"_id": system.parent_id}, session=None) + system_id = "invalid-id" + self.set_update_data(SYSTEM_A_INFO) + self.call_update_expecting_error(system_id, InvalidObjectIdError) + self.check_update_failed_with_exception("Invalid ObjectId value 'invalid-id'") -def test_update_duplicate_name_within_parent(test_helpers, database_mock, system_repository): - """ - Test updating a System with a duplicate name within the parent System + def test_update_no_changes(self): + """Test updating a System to have exactly the same contents""" - Verify that the `update` method properly handles the update of a System with a duplicate name in the - parent System - """ - system = SystemIn(**SYSTEM_A_INFO, **MOCK_CREATED_MODIFIED_TIME) - system_id = str(ObjectId()) + system_id = str(ObjectId()) - # Mock `find_one` to return a parent System document - test_helpers.mock_find_one( - database_mock.systems, {**SYSTEM_B_INFO, "_id": CustomObjectId(system_id), **MOCK_CREATED_MODIFIED_TIME} - ) - # Mock `find_one` to return duplicate system found in parent system - test_helpers.mock_find_one( - database_mock.systems, - { - **SYSTEM_B_INFO, - **MOCK_CREATED_MODIFIED_TIME, - "_id": ObjectId(), - }, - ) + self.mock_update(system_id, SYSTEM_A_INFO, SYSTEM_A_INFO) + self.call_update(system_id) + self.check_update_success() - with pytest.raises(DuplicateRecordError) as exc: - system_repository.update(system_id, system) - assert str(exc.value) == "Duplicate System found within the parent System" + def test_update_parent_id(self): + """Test updating a System's parent_id to move it""" - database_mock.systems.update_one.assert_not_called() + system_id = str(ObjectId()) + self.mock_update( + system_id=system_id, + new_system_data={**SYSTEM_A_INFO, "parent_id": str(ObjectId())}, + stored_system_data=SYSTEM_A_INFO, + new_parent_system_data=SYSTEM_B_INFO, + duplicate_system_data=None, + valid_move_result=True, + ) + self.call_update(system_id) + self.check_update_success() -@patch("inventory_management_system_api.repositories.system.utils") -def test_update_change_capitalisation_of_name(utils_mock, test_helpers, database_mock, system_repository): - """ - Test updating a System when the code is the same and the capitalisation of the name has changed. + def test_update_parent_id_to_child_of_self(self): + """Test updating a System's parent_id to a child of it self (should prevent this)""" + system_id = str(ObjectId()) - Verify that the `update` method properly handles the update of a System - """ - system = SystemOut(id=str(ObjectId()), **SYSTEM_A_INFO, **MOCK_CREATED_MODIFIED_TIME) - session = MagicMock() + self.mock_update( + system_id=system_id, + new_system_data={**SYSTEM_A_INFO, "parent_id": str(ObjectId())}, + stored_system_data=SYSTEM_A_INFO, + new_parent_system_data=SYSTEM_B_INFO, + duplicate_system_data=None, + valid_move_result=False, + ) + self.call_update_expecting_error(system_id, InvalidActionError) + self.check_update_failed_with_exception("Cannot move a system to one of its own children") - # Mock `find_one` to return the stored System document - test_helpers.mock_find_one( - database_mock.systems, - system.model_dump(), - ) + def test_update_with_non_existent_parent_id(self): + """Test updating a System's parent_id to a non-existent System""" - # Mock `find_one` to return None as a duplicate was not found - test_helpers.mock_find_one(database_mock.systems, None) + system_id = str(ObjectId()) + new_parent_id = str(ObjectId()) - # Mock `update_one` to return an object for the updated System document - test_helpers.mock_update_one(database_mock.systems) + self.mock_update( + system_id, {**SYSTEM_A_INFO, "parent_id": new_parent_id}, SYSTEM_A_INFO, new_parent_system_data=None + ) + self.call_update_expecting_error(system_id, MissingRecordError) + self.check_update_failed_with_exception(f"No parent System found with ID: {new_parent_id}") - # Mock `find_one` to return the updated System document - system_in = SystemIn(**{**SYSTEM_A_INFO, "name": "TeSt NaMe A"}, **MOCK_CREATED_MODIFIED_TIME) - test_helpers.mock_find_one(database_mock.systems, {**system_in.model_dump(), "_id": CustomObjectId(system.id)}) + def test_update_name_to_duplicate_within_parent(self): + """Test updating a System's name to one that is a duplicate within the same parent System""" - updated_system = system_repository.update(system.id, system_in, session=session) + system_id = str(ObjectId()) + new_name = "New Duplicate Name" - utils_mock.create_breadcrumbs_aggregation_pipeline.assert_not_called() - utils_mock.is_valid_move_result.assert_not_called() + self.mock_update( + system_id, {**SYSTEM_A_INFO, "name": new_name}, SYSTEM_A_INFO, duplicate_system_data=SYSTEM_A_INFO + ) + self.call_update_expecting_error(system_id, DuplicateRecordError) + self.check_update_failed_with_exception("Duplicate System found within the parent System") - database_mock.systems.update_one.assert_called_once_with( - { - "_id": CustomObjectId(system.id), - }, - { - "$set": { - **system_in.model_dump(), - }, - }, - session=session, - ) - database_mock.systems.find_one.assert_has_calls( - [ - call({"_id": CustomObjectId(system.id)}, session=session), - ] - ) - assert updated_system == SystemOut(id=system.id, **system_in.model_dump()) + def test_update_parent_id_with_duplicate_within_parent(self): + """Test updating a System's parent-id to one contains a System with a duplicate name within the same parent + System""" + + system_id = str(ObjectId()) + new_parent_id = str(ObjectId()) + self.mock_update( + system_id, + {**SYSTEM_A_INFO, "parent_id": new_parent_id}, + SYSTEM_A_INFO, + new_parent_system_data=SYSTEM_B_INFO, + duplicate_system_data=SYSTEM_A_INFO, + ) + self.call_update_expecting_error(system_id, DuplicateRecordError) + self.check_update_failed_with_exception("Duplicate System found within the parent System") -def test_delete(test_helpers, database_mock, system_repository): - """ - Test deleting a System - Verify that the `delete` method properly handles the deletion of a System by its ID - """ - system_id = str(ObjectId()) - session = MagicMock() +class DeleteDSL(SystemRepoDSL): + """Base class for delete tests""" - # Mock `delete_one` to return that one document has been deleted - test_helpers.mock_delete_one(database_mock.systems, 1) + _delete_system_id: str + _delete_exception: pytest.ExceptionInfo - # Mock `find_one` to return no systems - test_helpers.mock_find_one(database_mock.systems, None) - # Mock `find_one` to return no items - test_helpers.mock_find_one(database_mock.items, None) + def mock_delete( + self, deleted_count: int, child_system_data: Optional[dict] = None, child_item_data: Optional[dict] = None + ): + """Mocks database methods appropriately to test the 'delete' repo method - system_repository.delete(system_id, session=session) + :param deleted_count: Number of documents deleted successfully + :param child_system_data: Dictionary containing a child system's basic system information (or None) + :param child_item_data: Dictionary containing a child items's basic system information (or None) + """ - database_mock.systems.delete_one.assert_called_once_with({"_id": CustomObjectId(system_id)}, session=session) + self.test_helpers.mock_find_one(self.systems_collection, child_system_data) + self.test_helpers.mock_find_one(self.items_collection, child_item_data) + self.test_helpers.mock_delete_one(self.systems_collection, deleted_count) + def call_delete(self, system_id: str): + """Calls the SystemRepo `delete` method""" -def test_delete_with_child_systems(test_helpers, database_mock, system_repository): - """ - Test deleting a System with child Systems + self._delete_system_id = system_id + self.system_repository.delete(system_id, session=self.mock_session) - Verify that the `delete` method properly handles the deletion of a System with child Systems - """ - system_id = str(ObjectId()) + def call_delete_expecting_error(self, system_id: str, error_type: type): + """Calls the SystemRepo `delete` method while expecting an error to be raised""" - # Mock `find_one` to return a system - test_helpers.mock_find_one(database_mock.systems, MagicMock()) - # Mock `find_one` to return no items - test_helpers.mock_find_one(database_mock.items, None) + self._delete_system_id = system_id + with pytest.raises(error_type) as exc: + self.system_repository.delete(system_id) + self._delete_exception = exc + + def check_delete_success(self): + """Checks that a prior call to `call_delete` worked as expected""" - with pytest.raises(ChildElementsExistError) as exc: - system_repository.delete(system_id) + self.systems_collection.find_one.assert_called_once_with( + {"parent_id": CustomObjectId(self._delete_system_id)}, session=self.mock_session + ) + self.items_collection.find_one.assert_called_once_with( + {"system_id": CustomObjectId(self._delete_system_id)}, session=self.mock_session + ) + self.systems_collection.delete_one.assert_called_once_with( + {"_id": CustomObjectId(self._delete_system_id)}, session=self.mock_session + ) - database_mock.systems.delete_one.assert_not_called() - assert str(exc.value) == f"System with ID {system_id} has child elements and cannot be deleted" + def check_delete_failed_with_exception(self, message: str, expecting_delete_one_called: bool = False): + """Checks that a prior call to `call_delete_expecting_error` worked as expected, raising an exception + with the correct message""" + if not expecting_delete_one_called: + self.systems_collection.delete_one.assert_not_called() + else: + self.systems_collection.delete_one.assert_called_once_with( + {"_id": CustomObjectId(self._delete_system_id)}, session=None + ) -def test_delete_with_child_items(test_helpers, database_mock, system_repository): - """ - Test deleting a System with child Items + assert str(self._delete_exception.value) == message - Verify that the `delete` method properly handles the deletion of a System with child Items - """ - system_id = str(ObjectId()) - # Mock `find_one` to return a system - test_helpers.mock_find_one(database_mock.systems, None) - # Mock `find_one` to return an item (child elements found) - test_helpers.mock_find_one(database_mock.items, MagicMock()) +class TestDelete(DeleteDSL): + """Tests for deleting a System""" - with pytest.raises(ChildElementsExistError) as exc: - system_repository.delete(system_id) + def test_delete(self): + """Test deleting a System""" - database_mock.systems.delete_one.assert_not_called() - assert str(exc.value) == f"System with ID {system_id} has child elements and cannot be deleted" + self.mock_delete(deleted_count=1) + self.call_delete(str(ObjectId())) + self.check_delete_success() + def test_delete_with_child_system(self): + """Test deleting a System when it has a child system""" -def test_delete_with_invalid_id(database_mock, system_repository): - """ - Test deleting a System with an invalid ID + system_id = str(ObjectId()) - Verify that the `delete` method properly handles the deletion of a System with an invalid ID - """ + self.mock_delete(deleted_count=1, child_system_data=SYSTEM_A_INFO) + self.call_delete_expecting_error(system_id, ChildElementsExistError) + self.check_delete_failed_with_exception(f"System with ID {system_id} has child elements and cannot be deleted") - with pytest.raises(InvalidObjectIdError) as exc: - system_repository.delete("invalid") + def test_delete_with_child_item(self): + """Test deleting a System when it has a child item""" - database_mock.systems.delete_one.assert_not_called() - assert str(exc.value) == "Invalid ObjectId value 'invalid'" + system_id = str(ObjectId()) + self.mock_delete(deleted_count=1, child_item_data=ITEM_INFO) + self.call_delete_expecting_error(system_id, ChildElementsExistError) + self.check_delete_failed_with_exception(f"System with ID {system_id} has child elements and cannot be deleted") -def test_delete_with_non_existent_id(test_helpers, database_mock, system_repository): - """ - Test deleting a System with a non-existent ID + def test_delete_non_existent_id(self): + """Test deleting a System""" - Verify that the `delete` method properly handles the deletion of a System with a non-existant ID - """ - system_id = str(ObjectId()) + system_id = str(ObjectId()) - # Mock `delete_one` to return that no document has been deleted - test_helpers.mock_delete_one(database_mock.systems, 0) + self.mock_delete(deleted_count=0) + self.call_delete_expecting_error(system_id, MissingRecordError) + self.check_delete_failed_with_exception( + f"No System found with ID: {system_id}", expecting_delete_one_called=True + ) - # Mock `find_one` to return no systems - test_helpers.mock_find_one(database_mock.systems, None) - # Mock `find_one` to return no items - test_helpers.mock_find_one(database_mock.items, None) + def test_delete_invalid_id(self): + """Test deleting a System""" - with pytest.raises(MissingRecordError) as exc: - system_repository.delete(system_id) - assert str(exc.value) == f"No System found with ID: {system_id}" + system_id = "invalid-id" - database_mock.systems.delete_one.assert_called_once_with({"_id": CustomObjectId(system_id)}, session=None) + self.call_delete_expecting_error(system_id, InvalidObjectIdError) + self.check_delete_failed_with_exception("Invalid ObjectId value 'invalid-id'") From c23971380bd5578d727eefbf8e68adee46ad8b0e Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Thu, 20 Jun 2024 12:42:45 +0000 Subject: [PATCH 2/9] Refactor SystemService unit tests #90 --- test/unit/repositories/test_system.py | 4 +- test/unit/services/test_system.py | 530 +++++++++++++++----------- 2 files changed, 315 insertions(+), 219 deletions(-) diff --git a/test/unit/repositories/test_system.py b/test/unit/repositories/test_system.py index 74bbeb51..f7979aed 100644 --- a/test/unit/repositories/test_system.py +++ b/test/unit/repositories/test_system.py @@ -526,8 +526,8 @@ def call_update(self, system_id: str): self._updated_system = self.system_repository.update(system_id, self._system_in, session=self.mock_session) def call_update_expecting_error(self, system_id: str, error_type: type): - """Calls the SystemRepo `update` method with the appropriate data from a prior call to `mock_update` - while expecting an error to be raised""" + """Calls the SystemRepo `update` method with the appropriate data from a prior call to `mock_update` (or + `set_update_data`) while expecting an error to be raised""" with pytest.raises(error_type) as exc: self.system_repository.update(system_id, self._system_in) diff --git a/test/unit/services/test_system.py b/test/unit/services/test_system.py index 8151619a..b943ed41 100644 --- a/test/unit/services/test_system.py +++ b/test/unit/services/test_system.py @@ -2,17 +2,21 @@ Unit tests for the `SystemService` service """ -from datetime import timedelta -from unittest.mock import MagicMock -from test.unit.services.conftest import MODEL_MIXINS_FIXED_DATETIME_NOW +from test.unit.services.conftest import ServiceTestHelpers +from typing import Optional +from unittest.mock import MagicMock, Mock, patch import pytest from bson import ObjectId +from inventory_management_system_api.core.custom_object_id import CustomObjectId from inventory_management_system_api.core.exceptions import MissingRecordError from inventory_management_system_api.models.system import SystemIn, SystemOut from inventory_management_system_api.schemas.system import SystemPatchSchema, SystemPostSchema +from inventory_management_system_api.services import utils +from inventory_management_system_api.services.system import SystemService +# TODO: Move into common place? These are different to repo ones as these dont have code like the repo ones do SYSTEM_A_INFO = { "name": "Test name a", "location": "Test location", @@ -22,264 +26,356 @@ "parent_id": None, } -SYSTEM_A_INFO_FULL = { - **SYSTEM_A_INFO, - "code": "test-name-a", - "created_time": MODEL_MIXINS_FIXED_DATETIME_NOW, - "modified_time": MODEL_MIXINS_FIXED_DATETIME_NOW, -} - SYSTEM_B_INFO = { "name": "Test name b", - "location": "Test location", - "owner": "Test owner", + "location": "Test location 2", + "owner": "Test owner 2", "importance": "high", - "description": "Test description", + "description": "Test description 2", "parent_id": None, } -SYSTEM_B_INFO_FULL = { - **SYSTEM_A_INFO, - "code": "test-name-b", - "created_time": MODEL_MIXINS_FIXED_DATETIME_NOW, - "modified_time": MODEL_MIXINS_FIXED_DATETIME_NOW, -} +class SystemServiceDSL: + """Base class for SystemService unit tests""" + + test_helpers: ServiceTestHelpers + wrapped_utils: Mock + mock_system_repository: Mock + system_service: SystemService -def test_create( - test_helpers, - system_repository_mock, - model_mixins_datetime_now_mock, # pylint: disable=unused-argument - system_service, -): - """ - Test creating a System - - Verify that the `create` method properly handles the System to be created, generates the code, - and calls the repository's create method - """ - system = SystemOut(id=str(ObjectId()), **SYSTEM_A_INFO_FULL) - - # Mock `create` to return the created System - test_helpers.mock_create(system_repository_mock, system) - - created_system = system_service.create(SystemPostSchema(**SYSTEM_A_INFO)) - - system_repository_mock.create.assert_called_with(SystemIn(**SYSTEM_A_INFO_FULL)) - assert created_system == system - - -def test_create_with_parent_id( - test_helpers, - system_repository_mock, - model_mixins_datetime_now_mock, # pylint: disable=unused-argument - system_service, -): - """ - Test creating a System with a parent ID - - Verify that the `create` method properly handles the System to be created when it has a parent ID - """ - system_info = { - **SYSTEM_B_INFO, - "parent_id": str(ObjectId()), - } - full_system_info = { - **system_info, - "code": SYSTEM_B_INFO_FULL["code"], - "created_time": SYSTEM_B_INFO_FULL["created_time"], - "modified_time": SYSTEM_B_INFO_FULL["modified_time"], - } - system = SystemOut(id=str(ObjectId()), **full_system_info) - - # Mock `get` to return the parent system - test_helpers.mock_get( + @pytest.fixture(autouse=True) + def setup( + self, + test_helpers, system_repository_mock, - SystemOut(id=system.parent_id, **SYSTEM_B_INFO_FULL), - ) + system_service, + # Ensures all created and modified times are mocked throughout + model_mixins_datetime_now_mock, # pylint: disable=unused-argument + ): + """Setup fixtures""" - # Mock `create` to return the created System - test_helpers.mock_create(system_repository_mock, system) + self.test_helpers = test_helpers + self.mock_system_repository = system_repository_mock + self.system_service = system_service - created_system = system_service.create(SystemPostSchema(**system_info)) + with patch("inventory_management_system_api.services.system.utils", wraps=utils) as wrapped_utils: + self.wrapped_utils = wrapped_utils + yield - system_repository_mock.create.assert_called_with(SystemIn(**full_system_info)) - assert created_system == system +class CreateDSL(SystemServiceDSL): + """Base class for create tests""" -def test_create_with_whitespace_name( - test_helpers, - system_repository_mock, - model_mixins_datetime_now_mock, # pylint: disable=unused-argument - system_service, -): - """ - Test creating a System with a name containing leading/trailing/consecutive whitespaces + _system_post: SystemPostSchema + _expected_system_in: dict + _expected_system_out: SystemOut + _created_system: SystemOut + _create_exception: pytest.ExceptionInfo - Verify that the `create` method trims the whitespace from the System name and handles - it correctly - """ - system_info = { - **SYSTEM_A_INFO, - "name": " Test name ", - } - full_system_info = { - **system_info, - "code": "test-name", - "created_time": SYSTEM_B_INFO_FULL["created_time"], - "modified_time": SYSTEM_B_INFO_FULL["modified_time"], - } - system = SystemOut(id=str(ObjectId()), **full_system_info) + def mock_create(self, system_data: dict): + """Mocks repo methods appropriately to test the 'create' service method - # Mock `create` to return the created System - test_helpers.mock_create(system_repository_mock, system) + :param system_data: Dictionary containing the basic system information (excluding id, code or creation and + modified times as these will be automatically handled) + """ + self._system_post = SystemPostSchema(**system_data) - created_system = system_service.create(SystemPostSchema(**system_info)) + self._expected_system_in = SystemIn(**system_data, code=utils.generate_code(system_data["name"], "system")) + self._expected_system_out = SystemOut(**self._expected_system_in.model_dump(), id=ObjectId()) - system_repository_mock.create.assert_called_with(SystemIn(**full_system_info)) - assert created_system == system + self.test_helpers.mock_create(self.mock_system_repository, self._expected_system_out) + def call_create(self): + """Calls the SystemService `create` method with the appropriate data from a prior call to `mock_create`""" -def test_get(test_helpers, system_repository_mock, system_service): - """ - Test getting a System + self._created_system = self.system_service.create(self._system_post) - Verify that the `get` method properly handles the retrieval of a System - """ + def check_create_success(self): + """Checks that a prior call to `call_create` worked as expected""" - system_id = str(ObjectId()) - system = MagicMock() + self.wrapped_utils.generate_code.assert_called_once_with(self._expected_system_out.name, "system") + self.mock_system_repository.create.assert_called_once_with(self._expected_system_in) - # Mock `get` to return a System - test_helpers.mock_get(system_repository_mock, system) + assert self._created_system == self._expected_system_out - retrieved_system = system_service.get(system_id) - system_repository_mock.get.assert_called_once_with(system_id) - assert retrieved_system == system +class TestCreate(CreateDSL): + """Tests for creating a System""" + def test_create(self): + """Test creating a System""" -def test_get_with_non_existent_id(test_helpers, system_repository_mock, system_service): - """ - Test getting a System with a non-existent ID + self.mock_create(SYSTEM_A_INFO) + self.call_create() + self.check_create_success() - Verify that the `get` method properly handles the retrieval of a System with a non-existent ID - """ - system_id = str(ObjectId()) + def test_create_with_parent_id(self): + """Test creating a System with a parent ID""" - # Mock `get` to not return a System - test_helpers.mock_get(system_repository_mock, None) + self.mock_create({**SYSTEM_A_INFO, "parent_id": str(ObjectId())}) + self.call_create() + self.check_create_success() - retrieved_system = system_service.get(system_id) - system_repository_mock.get.assert_called_once_with(system_id) - assert retrieved_system is None +class GetDSL(SystemServiceDSL): + """Base class for get tests""" + _obtained_system_id: str + _expected_system: MagicMock + _obtained_system: MagicMock -def test_get_breadcrumbs(test_helpers, system_repository_mock, system_service): - """ - Test getting breadcrumbs for a system + def mock_get(self): + """Mocks repo methods appropriately to test the 'get' service method""" - Verify that the `get_breadcrumbs` method properly handles the retrieval of a System - """ - system_id = str(ObjectId()) - breadcrumbs = MagicMock() + # Simply a return currently, so no need to use actual data + self._expected_system = MagicMock() + self.test_helpers.mock_get(self.mock_system_repository, self._expected_system) - # Mock `get` to return breadcrumbs - test_helpers.mock_get_breadcrumbs(system_repository_mock, breadcrumbs) + def call_get(self, system_id: str): + """Calls the SystemService `get` method""" - retrieved_breadcrumbs = system_service.get_breadcrumbs(system_id) + self._obtained_system_id = system_id + self._obtained_system = self.system_service.get(system_id) - system_repository_mock.get_breadcrumbs.assert_called_once_with(system_id) - assert retrieved_breadcrumbs == breadcrumbs + def check_get_success(self): + """Checks that a prior call to `call_get` worked as expected""" + self.mock_system_repository.get.assert_called_once_with(self._obtained_system_id) -def test_list(system_repository_mock, system_service): - """ - Test listing systems + assert self._obtained_system == self._expected_system - Verify that the `list` method properly calls the repository function with any passed filters - """ - parent_id = MagicMock() +class TestGet(GetDSL): + """Tests for getting a System""" - result = system_service.list(parent_id=parent_id) + def test_get(self): + """Test getting a System""" - system_repository_mock.list.assert_called_once_with(parent_id) - assert result == system_repository_mock.list.return_value + self.mock_get() + self.call_get(str(ObjectId())) + self.check_get_success() -def test_update( - test_helpers, - system_repository_mock, - model_mixins_datetime_now_mock, # pylint: disable=unused-argument - system_service, -): - """ - Test updating a System +class GetBreadcrumbsDSL(SystemServiceDSL): + """Base class for get_breadcrumbs tests""" - Verify that the 'update' method properly handles the update of a System - """ - system = SystemOut( - id=str(ObjectId()), - **{**SYSTEM_A_INFO_FULL, "created_time": SYSTEM_A_INFO_FULL["created_time"] - timedelta(days=5)}, - ) + _expected_breadcrumbs: MagicMock + _obtained_breadcrumbs: MagicMock + _obtained_system_id: str - # Mock `get` to return a System - test_helpers.mock_get( - system_repository_mock, - SystemOut( - id=system.id, - **{ - **SYSTEM_A_INFO_FULL, - "name": "Different name", - "code": "different-name", - "created_time": system.created_time, - "modified_time": system.created_time, - }, - ), - ) - # Mock 'update' to return the updated System - test_helpers.mock_update(system_repository_mock, system) - - updated_system = system_service.update(system.id, SystemPatchSchema(name=system.name)) - - system_repository_mock.update.assert_called_once_with( - system.id, - SystemIn( - **{ - **SYSTEM_A_INFO_FULL, - "created_time": system.created_time, - } - ), - ) - assert updated_system == system - - -def test_update_with_non_existent_id(test_helpers, system_repository_mock, system_service): - """ - Test updating a System with a non-existent ID - - Verify that the 'update' method properly handles the update of a System with a non-existent ID - """ - # Mock `get` to not return a System - test_helpers.mock_get(system_repository_mock, None) - - system_id = str(ObjectId()) - with pytest.raises(MissingRecordError) as exc: - system_service.update(system_id, MagicMock()) - system_repository_mock.update.assert_not_called() - assert str(exc.value) == f"No System found with ID: {system_id}" - - -def test_delete(system_repository_mock, system_service): - """ - Test deleting a System - - Verify that the `delete` method properly handles the deletion of a System by ID - """ - system_id = MagicMock() - - system_service.delete(system_id) - - system_repository_mock.delete.assert_called_once_with(system_id) + def mock_get_breadcrumbs(self): + """Mocks repo methods appropriately to test the 'get_breadcrumbs' service method""" + + # Simply a return currently, so no need to use actual data + self._expected_breadcrumbs = MagicMock() + self.test_helpers.mock_get_breadcrumbs(self.mock_system_repository, self._expected_breadcrumbs) + + def call_get_breadcrumbs(self, system_id: str): + """Calls the SystemService `get` method""" + + self._obtained_system_id = system_id + self._obtained_breadcrumbs = self.system_service.get_breadcrumbs(system_id) + + def check_get_breadcrumbs_success(self): + """Checks that a prior call to `call_get` worked as expected""" + + self.mock_system_repository.get_breadcrumbs.assert_called_once_with(self._obtained_system_id) + + assert self._obtained_breadcrumbs == self._expected_breadcrumbs + + +class TestGetBreadcrumbs(GetBreadcrumbsDSL): + """Tests for getting the breadcrumbs of a System""" + + def test_get_breadcrumbs(self): + """Test getting a System's breadcrumbs""" + + self.mock_get_breadcrumbs() + self.call_get_breadcrumbs(str(ObjectId())) + self.check_get_breadcrumbs_success() + + +class ListDSL(SystemServiceDSL): + """Base class for list tests""" + + _parent_id_filter: str + _expected_systems: MagicMock + _obtained_systems: MagicMock + + def mock_list(self): + """Mocks repo methods appropriately to test the 'list' service method""" + + # Simply a return currently, so no need to use actual data + self._expected_systems = MagicMock() + self.test_helpers.mock_list(self.mock_system_repository, self._expected_systems) + + def call_list(self, parent_id: Optional[str]): + """Calls the SystemService `list` method""" + + self._parent_id_filter = parent_id + self._obtained_systems = self.system_service.list(parent_id) + + def check_list_success(self): + """Checks that a prior call to `call_list` worked as expected""" + + self.mock_system_repository.list.assert_called_once_with(self._parent_id_filter) + + assert self._obtained_systems == self._expected_systems + + +class TestList(ListDSL): + """Tests for getting a System""" + + def test_list(self): + """Test listing Systems""" + + self.mock_list() + self.call_list(str(ObjectId())) + self.check_list_success() + + +class UpdateDSL(SystemServiceDSL): + """Base class for update tests""" + + _stored_system: Optional[SystemOut] + _system_patch: SystemPatchSchema + _expected_system_in: SystemIn + _expected_system_out: MagicMock + _updated_system_id: str + _updated_system: MagicMock + _update_exception: pytest.ExceptionInfo + + def mock_update(self, system_id: str, system_update_data: dict, stored_system_data: Optional[dict]): + """Mocks repository methods appropriately to test the 'update' service method + + :param system_id: ID of the system that will be obtained + :param system_update_data: Dictionary containing the new basic system information (excluding id, code or + creation and modified times as these will be automatically handled) + :param stored_system_data: Dictionary containing the existing stored basic system information + """ + + # Stored system + self._stored_system = ( + SystemOut( + **SystemIn( + **stored_system_data, code=utils.generate_code(stored_system_data["name"], "system") + ).model_dump(), + id=CustomObjectId(system_id), + ) + if stored_system_data + else None + ) + self.test_helpers.mock_get(self.mock_system_repository, self._stored_system) + + # Patch schema + self._system_patch = SystemPatchSchema(**system_update_data) + + # Updated system + # TODO: Is this really necessary - can just use return value and then don't need this helper function + self._expected_system_out = MagicMock() + self.test_helpers.mock_update(self.mock_system_repository, self._expected_system_out) + + # Construct the expected input for the repository + merged_system_data = {**(stored_system_data or {}), **system_update_data} + self._expected_system_in = SystemIn( + **merged_system_data, + code=utils.generate_code(merged_system_data["name"], "system"), + ) + + def call_update(self, system_id: str): + """Calls the SystemService `update` method with the appropriate data from a prior call to `mock_update`""" + + self._updated_system_id = system_id + self._updated_system = self.system_service.update(system_id, self._system_patch) + + def call_update_expecting_error(self, system_id: str, error_type: type): + """Calls the SystemService `update` method with the appropriate data from a prior call to `mock_update` + while expecting an error to be raised""" + + with pytest.raises(error_type) as exc: + self.system_service.update(system_id, self._system_patch) + self._update_exception = exc + + def check_update_success(self): + """Checks that a prior call to `call_update` worked as expected""" + + # Ensure obtained old system + self.mock_system_repository.get.assert_called_once_with(self._updated_system_id) + + # Ensure new code was obtained if patching name + if self._system_patch.name: + self.wrapped_utils.generate_code.assert_called_once_with(self._system_patch.name, "system") + else: + self.wrapped_utils.generate_code.assert_not_called() + + # Ensure updated with expected data + self.mock_system_repository.update.assert_called_once_with(self._updated_system_id, self._expected_system_in) + + assert self._updated_system == self._expected_system_out + + def check_update_failed_with_exception(self, message: str): + """Checks that a prior call to `call_update_expecting_error` worked as expected, raising an exception + with the correct message""" + + self.mock_system_repository.update.assert_not_called() + + assert str(self._update_exception.value) == message + + +class TestUpdate(UpdateDSL): + """Tests for updating a system""" + + def test_update_all_fields(self): + """Test updating all fields of a System""" + + system_id = str(ObjectId()) + + self.mock_update(system_id, SYSTEM_B_INFO, SYSTEM_A_INFO) + self.call_update(system_id) + self.check_update_success() + + def test_update_description_field_only(self): + """Test updating System's description field only (code should not need regenerating as name doesn't change)""" + + system_id = str(ObjectId()) + + self.mock_update(system_id, {"description": "A new description"}, SYSTEM_A_INFO) + self.call_update(system_id) + self.check_update_success() + + def test_update_with_non_existent_id(self): + """Test updating a System with a non-existent ID""" + + system_id = str(ObjectId()) + + self.mock_update(system_id, SYSTEM_B_INFO, None) + self.call_update_expecting_error(system_id, MissingRecordError) + self.check_update_failed_with_exception(f"No System found with ID: {system_id}") + + +class DeleteDSL(SystemServiceDSL): + """Base class for delete tests""" + + _delete_system_id: str + + def call_delete(self, system_id: str): + """Calls the SystemService `delete` method""" + + self._delete_system_id = system_id + self.system_service.delete(system_id) + + def check_delete_success(self): + """Checks that a prior call to `call_delete` worked as expected""" + + self.mock_system_repository.delete.assert_called_once_with(self._delete_system_id) + + +class TestDelete(DeleteDSL): + """Tests for deleting a System""" + + def test_delete(self): + """Test deleting a System""" + + self.call_delete(str(ObjectId())) + self.check_delete_success() From 86289a654f03343c417b64ba67662893d6c6cf9a Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Thu, 20 Jun 2024 14:47:51 +0000 Subject: [PATCH 3/9] Improve parameter & mock data names and use common data file #90 --- test/unit/mock_data.py | 38 ++++++ test/unit/repositories/test_system.py | 179 +++++++++++++------------- test/unit/services/test_system.py | 74 +++++------ 3 files changed, 163 insertions(+), 128 deletions(-) create mode 100644 test/unit/mock_data.py diff --git a/test/unit/mock_data.py b/test/unit/mock_data.py new file mode 100644 index 00000000..268e62db --- /dev/null +++ b/test/unit/mock_data.py @@ -0,0 +1,38 @@ +""" +Mock data for use in unit tests + +Names should ideally be descriptive enough to recognise what they are without looking at the data itself. +Letters may be appended in places to indicate the data is of the same type, but has different specific values +to others. + +_POST_DATA - Is for a `PostSchema` schema +_IN_DATA - Is for an `In` model +""" + +SYSTEM_POST_DATA_NO_PARENT_A = { + "parent_id": None, + "name": "Test name A", + "description": "Test description A", + "location": "Test location A", + "owner": "Test owner A", + "importance": "low", +} + +SYSTEM_POST_DATA_NO_PARENT_B = { + "parent_id": None, + "name": "Test name B", + "description": "Test description B", + "location": "Test location B", + "owner": "Test owner B", + "importance": "low", +} + +SYSTEM_IN_DATA_NO_PARENT_A = { + **SYSTEM_POST_DATA_NO_PARENT_A, + "code": "test-name-a", +} + +SYSTEM_IN_DATA_NO_PARENT_B = { + **SYSTEM_POST_DATA_NO_PARENT_B, + "code": "test-name-b", +} diff --git a/test/unit/repositories/test_system.py b/test/unit/repositories/test_system.py index f7979aed..aee8c124 100644 --- a/test/unit/repositories/test_system.py +++ b/test/unit/repositories/test_system.py @@ -2,6 +2,7 @@ Unit tests for the `SystemRepo` repository """ +from test.unit.mock_data import SYSTEM_IN_DATA_NO_PARENT_A, SYSTEM_IN_DATA_NO_PARENT_B from test.unit.repositories.conftest import RepositoryTestHelpers from test.unit.repositories.test_utils import ( MOCK_BREADCRUMBS_QUERY_RESULT_LESS_THAN_MAX_LENGTH, @@ -26,26 +27,6 @@ from inventory_management_system_api.models.system import SystemIn, SystemOut from inventory_management_system_api.repositories.system import SystemRepo -SYSTEM_A_INFO = { - "parent_id": None, - "name": "Test name a", - "description": "Test description", - "location": "Test location", - "owner": "Test owner", - "importance": "low", - "code": "test-name-a", -} - -SYSTEM_B_INFO = { - "parent_id": None, - "name": "Test name b", - "description": "Test description", - "location": "Test location", - "owner": "Test owner", - "importance": "low", - "code": "test-name-b", -} - class SystemRepoDSL: """Base class for SystemRepo unit tests""" @@ -86,39 +67,44 @@ class CreateDSL(SystemRepoDSL): def mock_create( self, - system_data: dict, - parent_system_data: Optional[dict] = None, - duplicate_system_data: Optional[dict] = None, + system_in_data: dict, + parent_system_in_data: Optional[dict] = None, + duplicate_system_in_data: Optional[dict] = None, ): """Mocks database methods appropriately to test the 'create' repo method - :param system_data: Dictionary containing the basic system information (excluding id or creation and modified - times as these will be automatically handled) - :param parent_system_data: Either None or a dictionary containing the basic parent system data - :param duplicate_system_data: Either None or a dictionary containing basic system data for a duplicate system + :param system_in_data: Dictionary containing the system data as would be required for a SystemIn database + model (i.e. no id or created and modified times required) + :param parent_system_in_data: Either None or a dictionary containing the parent system data as would be + required for a SystemIn database model + :param duplicate_system_in_data: Either None or a dictionary containing system data for a duplicate system """ inserted_system_id = CustomObjectId(str(ObjectId())) # Pass through system_in first as need creation and modified times - self._system_in = SystemIn(**system_data) + self._system_in = SystemIn(**system_in_data) self._expected_system_out = SystemOut(**self._system_in.model_dump(), id=inserted_system_id) # When a parent_id is given, need to mock the find_one for it too - if system_data["parent_id"]: + if system_in_data["parent_id"]: # If parent_system_data is given as none, then it is intentionally supposed to be, otherwise # pass through SystemIn first to ensure it has creation and modified times self.test_helpers.mock_find_one( self.systems_collection, ( - {**SystemIn(**parent_system_data).model_dump(), "_id": system_data["parent_id"]} - if parent_system_data + {**SystemIn(**parent_system_in_data).model_dump(), "_id": system_in_data["parent_id"]} + if parent_system_in_data else None ), ) self.test_helpers.mock_find_one( self.systems_collection, - {**SystemIn(**duplicate_system_data).model_dump(), "_id": ObjectId()} if duplicate_system_data else None, + ( + {**SystemIn(**duplicate_system_in_data).model_dump(), "_id": ObjectId()} + if duplicate_system_in_data + else None + ), ) self.test_helpers.mock_insert_one(self.systems_collection, inserted_system_id) self.test_helpers.mock_find_one( @@ -185,14 +171,17 @@ class TestCreate(CreateDSL): def test_create(self): """Test creating a System""" - self.mock_create(SYSTEM_A_INFO) + self.mock_create(SYSTEM_IN_DATA_NO_PARENT_A) self.call_create() self.check_create_success() def test_create_with_parent_id(self): """Test creating a System with a valid parent_id""" - self.mock_create({**SYSTEM_A_INFO, "parent_id": str(ObjectId())}, parent_system_data=SYSTEM_B_INFO) + self.mock_create( + {**SYSTEM_IN_DATA_NO_PARENT_A, "parent_id": str(ObjectId())}, + parent_system_in_data=SYSTEM_IN_DATA_NO_PARENT_B, + ) self.call_create() self.check_create_success() @@ -201,7 +190,7 @@ def test_create_with_non_existent_parent_id(self): parent_id = str(ObjectId()) - self.mock_create({**SYSTEM_A_INFO, "parent_id": parent_id}, parent_system_data=None) + self.mock_create({**SYSTEM_IN_DATA_NO_PARENT_A, "parent_id": parent_id}, parent_system_in_data=None) self.call_create_expecting_error(MissingRecordError) self.check_create_failed_with_exception(f"No parent System found with ID: {parent_id}") @@ -209,9 +198,9 @@ def test_create_with_duplicate_name_within_parent(self): """Test creating a System with a duplicate system being found in the same parent system""" self.mock_create( - {**SYSTEM_A_INFO, "parent_id": str(ObjectId())}, - parent_system_data=SYSTEM_B_INFO, - duplicate_system_data=SYSTEM_B_INFO, + {**SYSTEM_IN_DATA_NO_PARENT_A, "parent_id": str(ObjectId())}, + parent_system_in_data=SYSTEM_IN_DATA_NO_PARENT_B, + duplicate_system_in_data=SYSTEM_IN_DATA_NO_PARENT_B, ) self.call_create_expecting_error(DuplicateRecordError) self.check_create_failed_with_exception("Duplicate System found within the parent System") @@ -225,16 +214,18 @@ class GetDSL(SystemRepoDSL): _obtained_system: Optional[SystemOut] _get_exception: pytest.ExceptionInfo - def mock_get(self, system_id: str, system_data: Optional[dict]): + def mock_get(self, system_id: str, system_in_data: Optional[dict]): """Mocks database methods appropriately to test the 'get' repo method :param system_id: ID of the system that will be obtained - :param system_data: Dictionary containing the basic system information (excluding id or creation and modified - times as these will be automatically handled) + :param system_in_data: Dictionary containing the system data as would be required for a SystemIn database + model (i.e. No id or created and modified times required) """ self._expected_system_out = ( - SystemOut(**SystemIn(**system_data).model_dump(), id=CustomObjectId(system_id)) if system_data else None + SystemOut(**SystemIn(**system_in_data).model_dump(), id=CustomObjectId(system_id)) + if system_in_data + else None ) self.test_helpers.mock_find_one( @@ -280,7 +271,7 @@ def test_get(self): system_id = str(ObjectId()) - self.mock_get(system_id, SYSTEM_A_INFO) + self.mock_get(system_id, SYSTEM_IN_DATA_NO_PARENT_A) self.call_get(system_id) self.check_get_success() @@ -365,13 +356,14 @@ class ListDSL(SystemRepoDSL): _parent_id_filter: str _obtained_systems_out: list[SystemOut] - def mock_list(self, systems_data: list[dict]): + def mock_list(self, systems_in_data: list[dict]): """Mocks database methods appropriately to test the 'list' repo method - :param systems_data: List of system data to use within the mocked data""" + :param systems_data: List of dictionaries containing the system data as would be required for a + SystemIn database model (i.e. no id or created and modified times required)""" self._expected_systems_out = [ - SystemOut(**SystemIn(**system_data).model_dump(), id=ObjectId()) for system_data in systems_data + SystemOut(**SystemIn(**system_in_data).model_dump(), id=ObjectId()) for system_in_data in systems_in_data ] self.test_helpers.mock_find( @@ -402,21 +394,21 @@ class TestList(ListDSL): def test_list(self): """Test listing all Systems""" - self.mock_list([SYSTEM_A_INFO, SYSTEM_B_INFO]) + self.mock_list([SYSTEM_IN_DATA_NO_PARENT_A, SYSTEM_IN_DATA_NO_PARENT_B]) self.call_list(parent_id=None) self.check_list_success() def test_list_with_parent_id_filter(self): """Test listing all Systems with a given parent_id""" - self.mock_list([SYSTEM_A_INFO, SYSTEM_B_INFO]) + self.mock_list([SYSTEM_IN_DATA_NO_PARENT_A, SYSTEM_IN_DATA_NO_PARENT_B]) self.call_list(parent_id=str(ObjectId())) self.check_list_success() def test_list_with_null_parent_id_filter(self): """Test listing all Systems with a 'null' parent_id""" - self.mock_list([SYSTEM_A_INFO, SYSTEM_B_INFO]) + self.mock_list([SYSTEM_IN_DATA_NO_PARENT_A, SYSTEM_IN_DATA_NO_PARENT_B]) self.call_list(parent_id="null") self.check_list_success() @@ -450,42 +442,45 @@ def set_update_data(self, new_system_data: dict): def mock_update( self, system_id: str, - new_system_data: dict, - stored_system_data: Optional[dict], - new_parent_system_data: Optional[dict] = None, - duplicate_system_data: Optional[dict] = None, + new_system_in_data: dict, + stored_system_in_data: Optional[dict], + new_parent_system_in_data: Optional[dict] = None, + duplicate_system_in_data: Optional[dict] = None, valid_move_result: bool = True, ): """Mocks database methods appropriately to test the 'update' repo method :param system_id: ID of the system that will be obtained - :param new_system_data: Dictionary containing the new basic system information (excluding id or creation and - modified times as these will be automatically handled) - :param stored_system_data: Dictionary containing the existing stored basic system information - :param new_parent_system_data: Either None or a dictionary containing the basic parent system data - :param duplicate_system_data: Either None or a dictionary containing basic system data for a duplicate system + :param new_system_in_data: Dictionary containing the new system information as would be required for a SystemIn + database model (i.e. no id or created and modified times required) + :param stored_system_in_data: Dictionary containing the system information for the existing stored System + as would be required for a SystemIn database model + :param new_parent_system_in_data: Either None or a dictionary containing the new parent system data as would be + required for a SystemIn database model + :param duplicate_system_in_data: Either None or a dictionary containing the data for a duplicate system as would + be required for a SystemIn database model :param valid_move_result: Whether to mock in a valid or invalid move result i.e. when True will simulating moving the system one of its own children """ - self.set_update_data(new_system_data) + self.set_update_data(new_system_in_data) # When a parent_id is given, need to mock the find_one for it too - if new_system_data["parent_id"]: + if new_system_in_data["parent_id"]: # If new_parent_system_data is given as none, then it is intentionally supposed to be, otherwise # pass through SystemIn first to ensure it has creation and modified times self.test_helpers.mock_find_one( self.systems_collection, ( - {**SystemIn(**new_parent_system_data).model_dump(), "_id": new_system_data["parent_id"]} - if new_parent_system_data + {**SystemIn(**new_parent_system_in_data).model_dump(), "_id": new_system_in_data["parent_id"]} + if new_parent_system_in_data else None ), ) # Stored system self._stored_system = ( - SystemOut(**SystemIn(**stored_system_data).model_dump(), id=CustomObjectId(system_id)) - if stored_system_data + SystemOut(**SystemIn(**stored_system_in_data).model_dump(), id=CustomObjectId(system_id)) + if stored_system_in_data else None ) self.test_helpers.mock_find_one( @@ -493,13 +488,15 @@ def mock_update( ) # Duplicate check - self._moving_system = stored_system_data and (new_system_data["parent_id"] != stored_system_data["parent_id"]) - if (stored_system_data and (self._system_in.name != self._stored_system.name)) or self._moving_system: + self._moving_system = stored_system_in_data and ( + new_system_in_data["parent_id"] != stored_system_in_data["parent_id"] + ) + if (stored_system_in_data and (self._system_in.name != self._stored_system.name)) or self._moving_system: self.test_helpers.mock_find_one( self.systems_collection, ( - {**SystemIn(**duplicate_system_data).model_dump(), "_id": ObjectId()} - if duplicate_system_data + {**SystemIn(**duplicate_system_in_data).model_dump(), "_id": ObjectId()} + if duplicate_system_in_data else None ), ) @@ -606,7 +603,7 @@ def test_update(self): system_id = str(ObjectId()) - self.mock_update(system_id, SYSTEM_A_INFO, SYSTEM_B_INFO) + self.mock_update(system_id, SYSTEM_IN_DATA_NO_PARENT_A, SYSTEM_IN_DATA_NO_PARENT_B) self.call_update(system_id) self.check_update_success() @@ -615,7 +612,7 @@ def test_update_with_invalid_id(self): system_id = "invalid-id" - self.set_update_data(SYSTEM_A_INFO) + self.set_update_data(SYSTEM_IN_DATA_NO_PARENT_A) self.call_update_expecting_error(system_id, InvalidObjectIdError) self.check_update_failed_with_exception("Invalid ObjectId value 'invalid-id'") @@ -624,7 +621,7 @@ def test_update_no_changes(self): system_id = str(ObjectId()) - self.mock_update(system_id, SYSTEM_A_INFO, SYSTEM_A_INFO) + self.mock_update(system_id, SYSTEM_IN_DATA_NO_PARENT_A, SYSTEM_IN_DATA_NO_PARENT_B) self.call_update(system_id) self.check_update_success() @@ -635,10 +632,10 @@ def test_update_parent_id(self): self.mock_update( system_id=system_id, - new_system_data={**SYSTEM_A_INFO, "parent_id": str(ObjectId())}, - stored_system_data=SYSTEM_A_INFO, - new_parent_system_data=SYSTEM_B_INFO, - duplicate_system_data=None, + new_system_in_data={**SYSTEM_IN_DATA_NO_PARENT_A, "parent_id": str(ObjectId())}, + stored_system_in_data=SYSTEM_IN_DATA_NO_PARENT_A, + new_parent_system_in_data=SYSTEM_IN_DATA_NO_PARENT_B, + duplicate_system_in_data=None, valid_move_result=True, ) self.call_update(system_id) @@ -650,10 +647,10 @@ def test_update_parent_id_to_child_of_self(self): self.mock_update( system_id=system_id, - new_system_data={**SYSTEM_A_INFO, "parent_id": str(ObjectId())}, - stored_system_data=SYSTEM_A_INFO, - new_parent_system_data=SYSTEM_B_INFO, - duplicate_system_data=None, + new_system_in_data={**SYSTEM_IN_DATA_NO_PARENT_A, "parent_id": str(ObjectId())}, + stored_system_in_data=SYSTEM_IN_DATA_NO_PARENT_B, + new_parent_system_in_data=SYSTEM_IN_DATA_NO_PARENT_B, + duplicate_system_in_data=None, valid_move_result=False, ) self.call_update_expecting_error(system_id, InvalidActionError) @@ -666,7 +663,10 @@ def test_update_with_non_existent_parent_id(self): new_parent_id = str(ObjectId()) self.mock_update( - system_id, {**SYSTEM_A_INFO, "parent_id": new_parent_id}, SYSTEM_A_INFO, new_parent_system_data=None + system_id, + {**SYSTEM_IN_DATA_NO_PARENT_A, "parent_id": new_parent_id}, + SYSTEM_IN_DATA_NO_PARENT_A, + new_parent_system_in_data=None, ) self.call_update_expecting_error(system_id, MissingRecordError) self.check_update_failed_with_exception(f"No parent System found with ID: {new_parent_id}") @@ -678,7 +678,10 @@ def test_update_name_to_duplicate_within_parent(self): new_name = "New Duplicate Name" self.mock_update( - system_id, {**SYSTEM_A_INFO, "name": new_name}, SYSTEM_A_INFO, duplicate_system_data=SYSTEM_A_INFO + system_id, + {**SYSTEM_IN_DATA_NO_PARENT_A, "name": new_name}, + SYSTEM_IN_DATA_NO_PARENT_A, + duplicate_system_in_data=SYSTEM_IN_DATA_NO_PARENT_A, ) self.call_update_expecting_error(system_id, DuplicateRecordError) self.check_update_failed_with_exception("Duplicate System found within the parent System") @@ -692,10 +695,10 @@ def test_update_parent_id_with_duplicate_within_parent(self): self.mock_update( system_id, - {**SYSTEM_A_INFO, "parent_id": new_parent_id}, - SYSTEM_A_INFO, - new_parent_system_data=SYSTEM_B_INFO, - duplicate_system_data=SYSTEM_A_INFO, + {**SYSTEM_IN_DATA_NO_PARENT_A, "parent_id": new_parent_id}, + SYSTEM_IN_DATA_NO_PARENT_A, + new_parent_system_in_data=SYSTEM_IN_DATA_NO_PARENT_B, + duplicate_system_in_data=SYSTEM_IN_DATA_NO_PARENT_A, ) self.call_update_expecting_error(system_id, DuplicateRecordError) self.check_update_failed_with_exception("Duplicate System found within the parent System") @@ -713,8 +716,8 @@ def mock_delete( """Mocks database methods appropriately to test the 'delete' repo method :param deleted_count: Number of documents deleted successfully - :param child_system_data: Dictionary containing a child system's basic system information (or None) - :param child_item_data: Dictionary containing a child items's basic system information (or None) + :param child_system_data: Dictionary containing a child system's data (or None) + :param child_item_data: Dictionary containing a child items's data (or None) """ self.test_helpers.mock_find_one(self.systems_collection, child_system_data) @@ -777,7 +780,7 @@ def test_delete_with_child_system(self): system_id = str(ObjectId()) - self.mock_delete(deleted_count=1, child_system_data=SYSTEM_A_INFO) + self.mock_delete(deleted_count=1, child_system_data=SYSTEM_IN_DATA_NO_PARENT_A) self.call_delete_expecting_error(system_id, ChildElementsExistError) self.check_delete_failed_with_exception(f"System with ID {system_id} has child elements and cannot be deleted") diff --git a/test/unit/services/test_system.py b/test/unit/services/test_system.py index b943ed41..83898dec 100644 --- a/test/unit/services/test_system.py +++ b/test/unit/services/test_system.py @@ -2,6 +2,7 @@ Unit tests for the `SystemService` service """ +from test.unit.mock_data import SYSTEM_POST_DATA_NO_PARENT_A, SYSTEM_POST_DATA_NO_PARENT_B from test.unit.services.conftest import ServiceTestHelpers from typing import Optional from unittest.mock import MagicMock, Mock, patch @@ -16,25 +17,6 @@ from inventory_management_system_api.services import utils from inventory_management_system_api.services.system import SystemService -# TODO: Move into common place? These are different to repo ones as these dont have code like the repo ones do -SYSTEM_A_INFO = { - "name": "Test name a", - "location": "Test location", - "owner": "Test owner", - "importance": "low", - "description": "Test description", - "parent_id": None, -} - -SYSTEM_B_INFO = { - "name": "Test name b", - "location": "Test location 2", - "owner": "Test owner 2", - "importance": "high", - "description": "Test description 2", - "parent_id": None, -} - class SystemServiceDSL: """Base class for SystemService unit tests""" @@ -51,7 +33,8 @@ def setup( system_repository_mock, system_service, # Ensures all created and modified times are mocked throughout - model_mixins_datetime_now_mock, # pylint: disable=unused-argument + # pylint: disable=unused-argument + model_mixins_datetime_now_mock, ): """Setup fixtures""" @@ -68,20 +51,22 @@ class CreateDSL(SystemServiceDSL): """Base class for create tests""" _system_post: SystemPostSchema - _expected_system_in: dict + _expected_system_in: SystemIn _expected_system_out: SystemOut _created_system: SystemOut _create_exception: pytest.ExceptionInfo - def mock_create(self, system_data: dict): + def mock_create(self, system_post_data: dict): """Mocks repo methods appropriately to test the 'create' service method - :param system_data: Dictionary containing the basic system information (excluding id, code or creation and - modified times as these will be automatically handled) + :param system_post_data: Dictionary containing the basic system data as would be required for a + SystemPostSchema (i.e. no id, code or created and modified times required) """ - self._system_post = SystemPostSchema(**system_data) + self._system_post = SystemPostSchema(**system_post_data) - self._expected_system_in = SystemIn(**system_data, code=utils.generate_code(system_data["name"], "system")) + self._expected_system_in = SystemIn( + **system_post_data, code=utils.generate_code(system_post_data["name"], "system") + ) self._expected_system_out = SystemOut(**self._expected_system_in.model_dump(), id=ObjectId()) self.test_helpers.mock_create(self.mock_system_repository, self._expected_system_out) @@ -106,14 +91,14 @@ class TestCreate(CreateDSL): def test_create(self): """Test creating a System""" - self.mock_create(SYSTEM_A_INFO) + self.mock_create(SYSTEM_POST_DATA_NO_PARENT_A) self.call_create() self.check_create_success() def test_create_with_parent_id(self): """Test creating a System with a parent ID""" - self.mock_create({**SYSTEM_A_INFO, "parent_id": str(ObjectId())}) + self.mock_create({**SYSTEM_POST_DATA_NO_PARENT_A, "parent_id": str(ObjectId())}) self.call_create() self.check_create_success() @@ -246,38 +231,39 @@ class UpdateDSL(SystemServiceDSL): _updated_system: MagicMock _update_exception: pytest.ExceptionInfo - def mock_update(self, system_id: str, system_update_data: dict, stored_system_data: Optional[dict]): + def mock_update(self, system_id: str, system_patch_data: dict, stored_system_post_data: Optional[dict]): """Mocks repository methods appropriately to test the 'update' service method :param system_id: ID of the system that will be obtained - :param system_update_data: Dictionary containing the new basic system information (excluding id, code or - creation and modified times as these will be automatically handled) - :param stored_system_data: Dictionary containing the existing stored basic system information + :param system_patch_data: Dictionary containing the patch data as would be required for a + SystemPatchSchema (i.e. no id, code or created and modified times required) + :param stored_system_post_data: Dictionary containing the system data for the existing stored System + as would be required for a SystemPostSchema (i.e. no id, code or created and + modified times required) """ # Stored system self._stored_system = ( SystemOut( **SystemIn( - **stored_system_data, code=utils.generate_code(stored_system_data["name"], "system") + **stored_system_post_data, code=utils.generate_code(stored_system_post_data["name"], "system") ).model_dump(), id=CustomObjectId(system_id), ) - if stored_system_data + if stored_system_post_data else None ) self.test_helpers.mock_get(self.mock_system_repository, self._stored_system) # Patch schema - self._system_patch = SystemPatchSchema(**system_update_data) + self._system_patch = SystemPatchSchema(**system_patch_data) # Updated system - # TODO: Is this really necessary - can just use return value and then don't need this helper function self._expected_system_out = MagicMock() self.test_helpers.mock_update(self.mock_system_repository, self._expected_system_out) # Construct the expected input for the repository - merged_system_data = {**(stored_system_data or {}), **system_update_data} + merged_system_data = {**(stored_system_post_data or {}), **system_patch_data} self._expected_system_in = SystemIn( **merged_system_data, code=utils.generate_code(merged_system_data["name"], "system"), @@ -331,7 +317,11 @@ def test_update_all_fields(self): system_id = str(ObjectId()) - self.mock_update(system_id, SYSTEM_B_INFO, SYSTEM_A_INFO) + self.mock_update( + system_id, + system_patch_data=SYSTEM_POST_DATA_NO_PARENT_B, + stored_system_post_data=SYSTEM_POST_DATA_NO_PARENT_A, + ) self.call_update(system_id) self.check_update_success() @@ -340,7 +330,11 @@ def test_update_description_field_only(self): system_id = str(ObjectId()) - self.mock_update(system_id, {"description": "A new description"}, SYSTEM_A_INFO) + self.mock_update( + system_id, + system_patch_data={"description": "A new description"}, + stored_system_post_data=SYSTEM_POST_DATA_NO_PARENT_A, + ) self.call_update(system_id) self.check_update_success() @@ -349,7 +343,7 @@ def test_update_with_non_existent_id(self): system_id = str(ObjectId()) - self.mock_update(system_id, SYSTEM_B_INFO, None) + self.mock_update(system_id, system_patch_data=SYSTEM_POST_DATA_NO_PARENT_B, stored_system_post_data=None) self.call_update_expecting_error(system_id, MissingRecordError) self.check_update_failed_with_exception(f"No System found with ID: {system_id}") From 5f2f03be6aa90b6cab8ca2e6d6f7b2aa430eb7b5 Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Mon, 24 Jun 2024 11:09:29 +0000 Subject: [PATCH 4/9] Initial refactor for systems e2e tests #302 --- test/e2e/conftest.py | 2 +- test/e2e/test_system.py | 887 ++++++++++++++++++++-------------------- 2 files changed, 441 insertions(+), 448 deletions(-) diff --git a/test/e2e/conftest.py b/test/e2e/conftest.py index ee494037..e6d7cec5 100644 --- a/test/e2e/conftest.py +++ b/test/e2e/conftest.py @@ -2,8 +2,8 @@ Module providing test fixtures for the e2e tests. """ -from typing import Optional from test.conftest import VALID_ACCESS_TOKEN +from typing import Optional import pytest from fastapi.testclient import TestClient diff --git a/test/e2e/test_system.py b/test/e2e/test_system.py index aba326c5..af60b90e 100644 --- a/test/e2e/test_system.py +++ b/test/e2e/test_system.py @@ -19,588 +19,581 @@ from typing import Any, Optional from unittest.mock import ANY +import pytest from bson import ObjectId +from fastapi import Response +from fastapi.testclient import TestClient from inventory_management_system_api.core.consts import BREADCRUMBS_TRAIL_MAX_LENGTH -SYSTEM_POST_REQUIRED_ONLY = { - "name": "System Test", +# TODO: Unify with others - could perhaps even reuse here too +SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY = { + "name": "System Test Required Values Only", "importance": "low", } -SYSTEM_POST_REQUIRED_ONLY_EXPECTED = { - **SYSTEM_POST_REQUIRED_ONLY, +SYSTEM_GET_DATA_REQUIRED_VALUES_ONLY = { + **SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, **CREATED_MODIFIED_VALUES_EXPECTED, "id": ANY, "parent_id": None, "description": None, "location": None, "owner": None, - "code": "system-test", + "code": "system-test-required-values-only", } +SYSTEM_POST_DATA_ALL_VALUES = { + **SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, + "name": "System Test All Values", + "parent_id": None, + "description": "Test description", + "location": "Test location", + "owner": "Test owner", +} + +SYSTEM_GET_DATA_ALL_VALUES = { + **SYSTEM_POST_DATA_ALL_VALUES, + **CREATED_MODIFIED_VALUES_EXPECTED, + "id": ANY, + "parent_id": None, + "code": "system-test-all-values", +} + + +class CreateDSL: + """Base class for create tests""" + + test_client: TestClient + + _post_response: Response + + @pytest.fixture(autouse=True) + def setup(self, test_client): + """Setup fixtures""" + + self.test_client = test_client + + def post_system(self, system_post_data: dict) -> Optional[str]: + """Posts a System with the given data, returns the id of the created system if successful""" + self._post_response = self.test_client.post("/v1/systems", json=system_post_data) + + return self._post_response.json()["id"] if self._post_response.status_code == 201 else None + + def check_post_system_success(self, expected_system_get_data: dict): + """Checks that a prior call to 'post_system' gave a successful response with the expected data returned""" + + assert self._post_response.status_code == 201 + assert self._post_response.json() == expected_system_get_data + + def check_post_system_failed_with_message(self, status_code: int, detail: str): + """Checks that a prior call to 'post_system' gave a failed response with the expected code and error message""" + + assert self._post_response.status_code == status_code + assert self._post_response.json()["detail"] == detail + + def check_post_system_failed_with_validation_message(self, status_code: int, message: str): + """Checks that a prior call to 'post_system' gave a failed response with the expected code and pydantic validation + error message""" + + assert self._post_response.status_code == status_code + assert self._post_response.json()["detail"][0]["msg"] == message + + +class TestCreate(CreateDSL): + """Tests for creating a System""" + + def test_create_with_only_required_values_provided(self): + """Test creating a System with only required values provided""" + + self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) + self.check_post_system_success(SYSTEM_GET_DATA_REQUIRED_VALUES_ONLY) + + def test_create_with_all_values_provided(self): + """Test creating a System with all values provided""" + + self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + self.check_post_system_success(SYSTEM_GET_DATA_ALL_VALUES) + + def test_create_with_valid_parent_id(self): + """Test creating a System with a valid parent id""" + + parent_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "parent_id": parent_id}) + self.check_post_system_success({**SYSTEM_GET_DATA_ALL_VALUES, "parent_id": parent_id}) + + def test_create_with_non_existent_parent_id(self): + """Test creating a System with a non-existent parent id""" + + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "parent_id": str(ObjectId())}) + self.check_post_system_failed_with_message(422, "The specified parent System does not exist") + + def test_create_with_invalid_parent_id(self): + """Test creating a System with an invalid parent id""" + + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "parent_id": "invalid-id"}) + self.check_post_system_failed_with_message(422, "The specified parent System does not exist") + + def test_create_with_duplicate_name_within_parent(self): + """Test creating a System with the same name as another within the same parent""" + + parent_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + # 2nd post should be the duplicate + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "parent_id": parent_id}) + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "parent_id": parent_id}) + self.check_post_system_failed_with_message( + 409, "A System with the same name already exists within the same parent System" + ) + + def test_create_with_invalid_importance(self): + """Test creating a System with an invalid importance""" + + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "importance": "invalid-importance"}) + self.check_post_system_failed_with_validation_message(422, "Input should be 'low', 'medium' or 'high'") + + +class GetDSL(CreateDSL): + """Base class for get tests""" + + _get_response: Response + + def get_system(self, system_id: str): + """Gets a System with the given id""" + + self._get_response = self.test_client.get(f"/v1/systems/{system_id}") + + def check_get_success(self, expected_system_get_data: dict): + """Checks that a prior call to 'get_system' gave a successful response with the expected data returned""" + + assert self._get_response.status_code == 200 + assert self._get_response.json() == expected_system_get_data + + def check_get_failed_with_message(self, status_code: int, detail: str): + """Checks that a prior call to 'get_system' gave a failed response with the expected code and error message""" + + assert self._get_response.status_code == status_code + assert self._get_response.json()["detail"] == detail + + +class TestGet(GetDSL): + """Tests for getting a System""" + + def test_get(self): + """Test getting a System""" + + system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + self.get_system(system_id) + self.check_get_success(SYSTEM_GET_DATA_ALL_VALUES) + + def test_get_with_non_existent_id(self): + """Test getting a System with a non-existent id""" -def _post_nested_systems(test_client, entities: list[dict]): - """Utility function for posting a set of mock systems where each successive entity should - be the parent of the next""" + self.get_system(str(ObjectId())) + self.check_get_failed_with_message(404, "System not found") - systems = [] - parent_id = None - for entity in entities: - system = test_client.post("/v1/systems", json={**entity, "parent_id": parent_id}).json() - parent_id = system["id"] - systems.append(system) + def test_get_with_invalid_id(self): + """Test getting a System with an invalid id""" - return (*systems,) + self.get_system("invalid-id") + self.check_get_failed_with_message(404, "System not found") -def _post_systems(test_client): - """Utility function for posting all mock systems defined at the top of this file""" +class GetBreadcrumbsDSL(CreateDSL): + """Base class for breadcrumbs tests""" - (system_a, system_b, *_) = _post_nested_systems(test_client, [SYSTEM_POST_A, SYSTEM_POST_B]) - (system_c, *_) = _post_nested_systems(test_client, [SYSTEM_POST_C]) + _get_response: Response + _posted_systems_get_data = [] - return system_a, system_b, system_c + def post_nested_systems(self, number: int) -> list[str]: + """Posts the given number of nested systems where each successive one has the previous as its parent""" + parent_id = None + for i in range(0, number): + system_id = self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "name": f"System {i}", "parent_id": parent_id}) + self._posted_systems_get_data.append(self._post_response.json()) + parent_id = system_id -def _post_n_systems(test_client, number): - """Utility function to post a given number of nested systems (all based on system A)""" - return _post_nested_systems( - test_client, - [ - { - **SYSTEM_POST_A, - "name": f"System {i}", - } - for i in range(0, number) - ], - ) + return [system["id"] for system in self._posted_systems_get_data] + def get_system_breadcrumbs(self, system_id: str): + """Gets a System's breadcrumbs with the given id""" -def _test_partial_update_system( - test_client, update_values: dict[str, Any], additional_expected_values: Optional[dict[str, Any]] = None -): - """ - Utility method that tests updating a system + self._get_response = self.test_client.get(f"/v1/systems/{system_id}/breadcrumbs") - :param update_values: Values to update - :param additional_expected_values: Any additional values expected from the output e.g. code - """ - if additional_expected_values is None: - additional_expected_values = {} + def get_last_system_breadcrumbs(self): + """Gets the last System posted's breadcrumbs""" - # Create one to update - response = test_client.post("/v1/systems", json=SYSTEM_POST_A) - system = response.json() + self.get_system_breadcrumbs(self._posted_systems_get_data[-1]["id"]) - # Update - response = test_client.patch(f"/v1/systems/{system['id']}", json=update_values) + def check_get_breadcrumbs_success(self, expected_trail_length: int, expected_full_trail: bool): + """Checks that a prior call to 'get_system_breadcrumbs' gave a successful response with the expected data returned""" - assert response.status_code == 200 - assert response.json() == { - **system, - **CREATED_MODIFIED_VALUES_EXPECTED, - **update_values, - **additional_expected_values, - } + assert self._get_response.status_code == 200 + assert self._get_response.json() == { + "trail": [ + [system["id"], system["name"]] + # When the expected trail length is < the number of systems posted, only use the last + # few systems inside the trail + for system in self._posted_systems_get_data[ + (len(self._posted_systems_get_data) - expected_trail_length) : + ] + ], + "full_trail": expected_full_trail, + } + def check_get_breadcrumbs_failed_with_message(self, status_code: int, detail: str): + """Checks that a prior call to 'get_system_breadcrumbs' gave a failed response with the expected code and + error message""" -def test_create_system(test_client): - """ - Test creating a System - """ - response = test_client.post("/v1/systems", json=SYSTEM_POST_A) + assert self._get_response.status_code == status_code + assert self._get_response.json()["detail"] == detail - assert response.status_code == 201 - system = response.json() +class TestGetBreadcrumbs(GetBreadcrumbsDSL): + """Tests for getting a System's breadcrumbs""" - assert system == SYSTEM_POST_A_EXPECTED + def test_get_breadcrumbs_when_no_parent(self): + """Test getting a System's breadcrumbs when the system has no parent""" + self.post_nested_systems(1) + self.get_last_system_breadcrumbs() + self.check_get_breadcrumbs_success(expected_trail_length=1, expected_full_trail=True) -def test_create_system_with_only_required_values_given(test_client): - """ - Test creating a System when only the required values are given - """ - response = test_client.post("/v1/systems", json=SYSTEM_POST_REQUIRED_ONLY) + def test_get_breadcrumbs_when_trail_length_less_than_maximum(self): + """Test getting a System's breadcrumbs when the full system trail should be less than the maximum trail + length""" - assert response.status_code == 201 + self.post_nested_systems(BREADCRUMBS_TRAIL_MAX_LENGTH - 1) + self.get_last_system_breadcrumbs() + self.check_get_breadcrumbs_success( + expected_trail_length=BREADCRUMBS_TRAIL_MAX_LENGTH - 1, expected_full_trail=True + ) - system = response.json() + def test_get_breadcrumbs_when_trail_length_maximum(self): + """Test getting a System's breadcrumbs when the full system trail should be equal to the maximum trail + length""" - assert system == SYSTEM_POST_REQUIRED_ONLY_EXPECTED + self.post_nested_systems(BREADCRUMBS_TRAIL_MAX_LENGTH) + self.get_last_system_breadcrumbs() + self.check_get_breadcrumbs_success(expected_trail_length=BREADCRUMBS_TRAIL_MAX_LENGTH, expected_full_trail=True) + def test_get_breadcrumbs_when_trail_length_greater_maximum(self): + """Test getting a System's breadcrumbs when the full system trail exceeds the maximum trail length""" -def test_create_system_with_valid_parent_id(test_client): - """ - Test creating a System with a valid parent ID - """ - # Parent - response = test_client.post("/v1/systems", json=SYSTEM_POST_A) - parent_system = response.json() + self.post_nested_systems(BREADCRUMBS_TRAIL_MAX_LENGTH + 1) + self.get_last_system_breadcrumbs() + self.check_get_breadcrumbs_success( + expected_trail_length=BREADCRUMBS_TRAIL_MAX_LENGTH, expected_full_trail=False + ) - # Child - response = test_client.post("/v1/systems", json={**SYSTEM_POST_B, "parent_id": parent_system["id"]}) + def test_get_breadcrumbs_with_non_existent_id(self): + """Test getting a System's breadcrumbs when given a non-existent system id""" - assert response.status_code == 201 - system = response.json() - assert system == {**SYSTEM_POST_B_EXPECTED, "parent_id": parent_system["id"]} + self.get_system_breadcrumbs(str(ObjectId())) + self.check_get_breadcrumbs_failed_with_message(404, "System not found") + def test_get_breadcrumbs_with_invalid_id(self): + """Test getting a System's breadcrumbs when given an invalid system id""" -def test_create_system_with_duplicate_name_within_parent(test_client): - """ - Test creating a System with a duplicate name within the parent System - """ - # Parent - response = test_client.post("/v1/systems", json=SYSTEM_POST_A) - parent_system = response.json() + self.get_system_breadcrumbs("invalid_id") + self.check_get_breadcrumbs_failed_with_message(404, "System not found") - # Child - post twice as will have the same name - test_client.post("/v1/systems", json={**SYSTEM_POST_B, "parent_id": parent_system["id"]}) - response = test_client.post("/v1/systems", json={**SYSTEM_POST_B, "parent_id": parent_system["id"]}) - assert response.status_code == 409 - assert response.json()["detail"] == "A System with the same name already exists within the same parent System" +class ListDSL(CreateDSL): + """Base class for list tests""" + _get_response: Response -def test_create_system_with_invalid_parent_id(test_client): - """ - Test creating a System with an invalid parent ID - """ - response = test_client.post("/v1/systems", json={**SYSTEM_POST_A, "parent_id": "invalid"}) + def list_systems(self, filters: dict): + """Gets a list Systems with the given filters""" - assert response.status_code == 422 - assert response.json()["detail"] == "The specified parent System does not exist" + self._get_response = self.test_client.get("/v1/systems", params=filters) + def post_test_system_with_child(self) -> list[dict]: + """Posts a System with a single child and returns their expected responses when returned by the list endpoint""" -def test_create_system_with_non_existent_parent_id(test_client): - """ - Test creating a System with a non-existent parent ID - """ - response = test_client.post("/v1/systems", json={**SYSTEM_POST_A, "parent_id": str(ObjectId())}) + parent_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + self.post_system({**SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, "parent_id": parent_id}) - assert response.status_code == 422 - assert response.json()["detail"] == "The specified parent System does not exist" + return [SYSTEM_GET_DATA_ALL_VALUES, {**SYSTEM_GET_DATA_REQUIRED_VALUES_ONLY, "parent_id": parent_id}] + def check_list_success(self, expected_systems_get_data: list[dict]): + """Checks that a prior call to 'list' gave a successful response with the expected data returned""" -def test_create_system_with_invalid_importance(test_client): - """ - Test creating a System with an invalid importance - """ - response = test_client.post("/v1/systems", json={**SYSTEM_POST_A, "importance": "invalid"}) + assert self._get_response.status_code == 200 + assert self._get_response.json() == expected_systems_get_data - assert response.status_code == 422 - assert response.json()["detail"][0]["msg"] == "Input should be 'low', 'medium' or 'high'" + def check_list_failed_with_message(self, status_code: int, detail: str): + """Checks that a prior call to 'list' gave a failed response with the expected code and error message""" + assert self._get_response.status_code == status_code + assert self._get_response.json()["detail"] == detail -def test_get_systems(test_client): - """ - Test getting a list of Systems - """ - system_a, system_b, system_c = _post_systems(test_client) - # Get all systems (no filters) - response = test_client.get("/v1/systems") +class TestList(ListDSL): + """Tests for getting a list of Systems""" - assert response.status_code == 200 - assert response.json() == [system_a, system_b, system_c] + def test_list_with_no_filters(self): + """Test getting a list of all Systems with no filters provided + Posts a system with a child and expects both to be returned. + """ -def test_get_systems_with_parent_id_filter(test_client): - """ - Test getting a list of Systems with a parent_id filter - """ - _, system_b, _ = _post_systems(test_client) + systems = self.post_test_system_with_child() + self.list_systems(filters={}) + self.check_list_success(systems) - # Get only those with the given parent_id - response = test_client.get("/v1/systems", params={"parent_id": system_b["parent_id"]}) + def test_list_with_parent_id_filter(self): + """Test getting a list of all Systems with a parent_id filter provided - assert response.status_code == 200 - assert response.json() == [system_b] + Posts a system with a child and then filter using the parent_id expecting only the second system + to be returned. + """ + systems = self.post_test_system_with_child() + self.list_systems(filters={"parent_id": systems[1]["parent_id"]}) + self.check_list_success([systems[1]]) -def test_get_systems_with_null_parent_id_filter(test_client): - """ - Test getting a list of Systems with a parent_id filter of "null" - """ + def test_list_with_null_parent_id_filter(self): + """Test getting a list of all Systems with a parent_id filter of "null" provided - system_a, _, system_c = _post_systems(test_client) + Posts a system with a child and then filter using a parent_id of "null" expecting only + the first parent system to be returned. + """ - # Get only those with the given parent parent_id - response = test_client.get("/v1/systems", params={"parent_id": "null"}) + systems = self.post_test_system_with_child() + self.list_systems(filters={"parent_id": "null"}) + self.check_list_success([systems[0]]) - assert response.status_code == 200 - assert response.json() == [system_a, system_c] + def test_list_with_parent_id_filter_with_no_matching_results(self): + """Test getting a list of all Systems with a parent_id filter that returns no results""" + self.list_systems(filters={"parent_id": str(ObjectId())}) + self.check_list_success([]) -def test_get_systems_with_parent_id_filter_no_matching_results(test_client): - """ - Test getting a list of Systems with a parent_id filter when there is no - matching results in the database - """ - _, _, _ = _post_systems(test_client) + def test_list_with_invalid_parent_id_filter(self): + """Test getting a list of all Systems with an invalid parent_id filter returns no results""" - # Get only those with the given parent_id - response = test_client.get("/v1/systems", params={"parent_id": str(ObjectId())}) + self.list_systems(filters={"parent_id": "invalid-id"}) + self.check_list_success([]) - assert response.status_code == 200 - assert response.json() == [] +# TODO: Put nested system stuff in create rather than get then change this to that +class UpdateDSL(GetBreadcrumbsDSL): + """Base class for update tests""" -def test_get_systems_with_invalid_parent_id_filter(test_client): - """ - Test getting a list of Systems when given invalid parent_id filter - """ - response = test_client.get("/v1/systems", params={"parent_id": "invalid"}) + _patch_response: Response - assert response.status_code == 200 - assert response.json() == [] + def patch_system(self, system_id: str, system_patch_data: dict): + """Updates a System with the given id""" + self._patch_response = self.test_client.patch(f"/v1/systems/{system_id}", json=system_patch_data) -def test_get_system(test_client): - """ - Test getting a System - """ - # Post one first - response = test_client.post("/v1/systems", json=SYSTEM_POST_A) - system = response.json() - system_id = system["id"] + def check_patch_system_response_success(self, expected_system_get_data: dict): + """Checks the response of patching a property succeeded as expected""" - # Ensure can get it again - response = test_client.get(f"/v1/systems/{system_id}") + assert self._patch_response.status_code == 200 + assert self._patch_response.json() == expected_system_get_data - assert response.status_code == 200 - assert response.json() == SYSTEM_POST_A_EXPECTED + def check_patch_system_failed_with_message(self, status_code: int, detail: str): + """Checks that a prior call to 'patch_system' gave a failed response with the expected code and error message""" + assert self._patch_response.status_code == status_code + assert self._patch_response.json()["detail"] == detail -def test_get_system_with_invalid_id(test_client): - """ - Test getting a System with an invalid ID - """ - response = test_client.get("/v1/systems/invalid") - assert response.status_code == 404 - assert response.json()["detail"] == "System not found" +class TestUpdate(UpdateDSL): + """Tests for updating a System""" + def test_partial_update_all_fields(self): + """Test updating every field of a System""" -def test_get_system_with_non_existent_id(test_client): - """ - Test getting a System with a non-existent ID - """ - response = test_client.get(f"/v1/systems/{str(ObjectId())}") + system_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) + self.patch_system(system_id, SYSTEM_POST_DATA_ALL_VALUES) + self.check_patch_system_response_success(SYSTEM_GET_DATA_ALL_VALUES) - assert response.status_code == 404 - assert response.json()["detail"] == "System not found" + def test_partial_update_parent_id(self): + """Test updating the parent_id of a System""" + parent_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) + system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) -def test_get_system_breadcrumbs_when_no_parent(test_client): - """ - Test getting the breadcrumbs for a system with no parents - """ - (system_c, *_) = _post_nested_systems(test_client, [SYSTEM_POST_C]) + self.patch_system(system_id, {"parent_id": parent_id}) + self.check_patch_system_response_success({**SYSTEM_GET_DATA_ALL_VALUES, "parent_id": parent_id}) - response = test_client.get(f"/v1/systems/{system_c['id']}/breadcrumbs") + def test_partial_update_parent_id_to_one_with_a_duplicate_name(self): + """Test updating the parent_id of a System so that its name conflicts with one already in that other + system""" - assert response.status_code == 200 - assert response.json() == {"trail": [[system_c["id"], system_c["name"]]], "full_trail": True} + # System with child + parent_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) + self.post_system({**SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, "name": "Conflicting Name", "parent_id": parent_id}) + system_id = self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "name": "Conflicting Name"}) -def test_get_system_breadcrumbs_when_trail_length_less_than_maximum(test_client): - """ - Test getting the breadcrumbs for a system with less than the the maximum trail length - """ - systems = _post_n_systems(test_client, BREADCRUMBS_TRAIL_MAX_LENGTH - 1) + self.patch_system(system_id, {"parent_id": parent_id}) + self.check_patch_system_failed_with_message( + 409, "A System with the same name already exists within the parent System" + ) - # Get breadcrumbs for last added - response = test_client.get(f"/v1/systems/{systems[-1]['id']}/breadcrumbs") + def test_partial_update_parent_id_to_child_of_self(self): + """Test updating the parent_id of a System to one of its own children""" - assert response.status_code == 200 - assert response.json() == {"trail": [[system["id"], system["name"]] for system in systems], "full_trail": True} + system_ids = self.post_nested_systems(2) + self.patch_system(system_ids[0], {"parent_id": system_ids[1]}) + self.check_patch_system_failed_with_message(422, "Cannot move a system to one of its own children") + def test_partial_update_parent_id_to_non_existent(self): + """Test updating the parent_id of a System to a non-existent System""" -def test_get_system_breadcrumbs_when_trail_length_maximum(test_client): - """ - Test getting the breadcrumbs for a system with the maximum trail length - """ - systems = _post_n_systems(test_client, BREADCRUMBS_TRAIL_MAX_LENGTH) + system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + self.patch_system(system_id, {"parent_id": str(ObjectId())}) + self.check_patch_system_failed_with_message(422, "The specified parent System does not exist") - # Get breadcrumbs for last added - response = test_client.get(f"/v1/systems/{systems[-1]['id']}/breadcrumbs") + def test_partial_update_parent_id_to_invalid(self): + """Test updating the parent_id of a System to an invalid id""" - assert response.status_code == 200 - assert response.json() == {"trail": [[system["id"], system["name"]] for system in systems], "full_trail": True} + system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + self.patch_system(system_id, {"parent_id": "invalid-id"}) + self.check_patch_system_failed_with_message(422, "The specified parent System does not exist") + def test_partial_update_name_to_duplicate(self): + """Test updating the name of a System to conflict with a pre-existing one""" -def test_get_system_breadcrumbs_when_trail_length_greater_than_maximum(test_client): - """ - Test getting the breadcrumbs for a system with greater than the the maximum trail length - """ - systems = _post_n_systems(test_client, BREADCRUMBS_TRAIL_MAX_LENGTH + 1) + self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) + system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + self.patch_system(system_id, {"name": SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY["name"]}) + self.check_patch_system_failed_with_message( + 409, "A System with the same name already exists within the parent System" + ) - # Get breadcrumbs for last added - response = test_client.get(f"/v1/systems/{systems[-1]['id']}/breadcrumbs") + def test_partial_update_name_capitalisation(self): + """Test updating the capitalisation of the name of a System (to ensure it the check doesn't confuse with duplicates)""" - assert response.status_code == 200 - assert response.json() == {"trail": [[system["id"], system["name"]] for system in systems[1:]], "full_trail": False} + system_id = self.post_system({**SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, "name": "Test system"}) + self.patch_system(system_id, {"name": "Test System"}) + self.check_patch_system_response_success( + {**SYSTEM_GET_DATA_REQUIRED_VALUES_ONLY, "name": "Test System", "code": "test-system"} + ) + def test_partial_update_with_non_existent_id(self): + """Test updating a non-existent System""" -def test_get_system_breadcrumbs_with_invalid_id(test_client): - """ - Test getting the breadcrumbs for a system when the given id is invalid - """ - response = test_client.get("/v1/systems/invalid/breadcrumbs") + self.patch_system(str(ObjectId()), {}) + self.check_patch_system_failed_with_message(404, "System not found") - assert response.status_code == 404 - assert response.json()["detail"] == "System not found" + def test_partial_update_invalid_id(self): + """Test updating a System with an invalid id""" + self.patch_system("invalid-id", {}) + self.check_patch_system_failed_with_message(404, "System not found") -def test_get_system_breadcrumbs_with_non_existent_id(test_client): - """ - Test getting the breadcrumbs for a non-existent system - """ - response = test_client.get(f"/v1/systems/{str(ObjectId())}/breadcrumbs") - assert response.status_code == 404 - assert response.json()["detail"] == "System not found" +class DeleteDSL(CreateDSL): + """Base class for delete tests""" + _delete_response: Response -def test_partial_update_system_parent_id(test_client): - """ - Test updating a System's parent_id - """ - parent_system = test_client.post("/v1/systems", json=SYSTEM_POST_B).json() + def delete_system(self, system_id: str): + """Deletes a System with the given id""" - _test_partial_update_system(test_client, {"parent_id": parent_system["id"]}, {}) + self._delete_response = self.test_client.delete(f"/v1/systems/{system_id}") + def check_delete_success(self): + """Checks that a prior call to 'delete_system' gave a successful response with the expected data returned""" -def test_partial_update_system_parent_id_to_child_id(test_client): - """ - Test updating a System's parent_id to be the id of one of its children - """ - nested_systems = _post_n_systems(test_client, 4) + assert self._delete_response.status_code == 204 - # Attempt to move first into one of its children - response = test_client.patch(f"/v1/systems/{nested_systems[0]['id']}", json={"parent_id": nested_systems[3]["id"]}) + def check_delete_failed_with_message(self, status_code: int, detail: str): + """Checks that a prior call to 'delete_system' gave a failed response with the expected code and error message""" - assert response.status_code == 422 - assert response.json()["detail"] == "Cannot move a system to one of its own children" + assert self._delete_response.status_code == status_code + assert self._delete_response.json()["detail"] == detail -def test_partial_update_system_invalid_parent_id(test_client): - """ - Test updating a System's parent_id when the ID is invalid - """ - # Create one to update - response = test_client.post("/v1/systems", json=SYSTEM_POST_A) - system = response.json() +class TestDelete(DeleteDSL): + """Tests for deleting a System""" - # Update - response = test_client.patch(f"/v1/systems/{system['id']}", json={"parent_id": "invalid"}) + def test_delete(self): + """Test deleting a System""" - assert response.status_code == 422 - assert response.json()["detail"] == "The specified parent System does not exist" + system_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) + self.delete_system(system_id) + self.check_delete_success() + def test_delete_with_child_system(self): + """Test deleting a System with a child system""" -def test_partial_update_system_non_existent_parent_id(test_client): - """ - Test updating a System's parent_id when the ID is non-existent - """ - # Create one to update - response = test_client.post("/v1/systems", json=SYSTEM_POST_A) - system = response.json() + system_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) + self.post_system({**SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, "parent_id": system_id}) + self.delete_system(system_id) + self.check_delete_failed_with_message(409, "System has child elements and cannot be deleted") - # Update - response = test_client.patch(f"/v1/systems/{system['id']}", json={"parent_id": str(ObjectId())}) + def test_delete_with_child_item(self): + """Test deleting a System with a child system""" - assert response.status_code == 422 - assert response.json()["detail"] == "The specified parent System does not exist" + # TODO: Cleanup in future + system_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) + self.post_system({**SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, "parent_id": system_id}) -def test_partial_update_system_parent_id_duplicate_name(test_client): - """ - Test updating a System's parent_id when the new parent has a child with a duplicate name - """ - # Parent to move to - parent_system = test_client.post("/v1/systems", json=SYSTEM_POST_A).json() + # Create a child item + # pylint: disable=duplicate-code + response = self.test_client.post("/v1/units", json=UNIT_POST_A) + unit_mm = response.json() - # Two identical systems, one already with a parent the other without - test_client.post( - "/v1/systems", json={**SYSTEM_POST_B, "name": "Duplicate name", "parent_id": parent_system["id"]} - ).json() - system = test_client.post("/v1/systems", json={**SYSTEM_POST_B, "name": "Duplicate name"}).json() + response = self.test_client.post("/v1/units", json=UNIT_POST_B) + unit_cm = response.json() - # Change the parent of system to be the same as system1 - response = test_client.patch(f"/v1/systems/{system['id']}", json={"parent_id": parent_system["id"]}) + units = [unit_mm, unit_cm] - assert response.status_code == 409 - assert response.json()["detail"] == "A System with the same name already exists within the parent System" + response = self.test_client.post( + "/v1/catalogue-categories", + json={ + **CATALOGUE_CATEGORY_POST_A, + "properties": replace_unit_values_with_ids_in_properties( + CATALOGUE_CATEGORY_POST_A["properties"], units + ), + }, + ) + catalogue_category = response.json() + response = self.test_client.post("/v1/manufacturers", json=MANUFACTURER_POST) + manufacturer_id = response.json()["id"] -def test_partial_update_system_name(test_client): - """ - Test updating a System's name - """ - _test_partial_update_system(test_client, {"name": "Updated name"}, {"code": "updated-name"}) + catalogue_item_post = { + **CATALOGUE_ITEM_POST_A, + "catalogue_category_id": catalogue_category["id"], + "manufacturer_id": manufacturer_id, + "properties": add_ids_to_properties(catalogue_category["properties"], CATALOGUE_ITEM_POST_A["properties"]), + } + response = self.test_client.post("/v1/catalogue-items", json=catalogue_item_post) + catalogue_item_id = response.json()["id"] + response = self.test_client.post("/v1/usage-statuses", json=USAGE_STATUS_POST_B) + usage_status_id = response.json()["id"] -def test_partial_update_capitalisation_of_system_name(test_client): - """ - Test updating a capitalisation of the System's name - """ - _test_partial_update_system( - test_client, - { - "name": "SyStEm A", - }, - ) + item_post = { + **ITEM_POST, + "catalogue_item_id": catalogue_item_id, + "system_id": system_id, + "usage_status_id": usage_status_id, + "properties": add_ids_to_properties(catalogue_category["properties"], ITEM_POST["properties"]), + } + self.test_client.post("/v1/items", json=item_post) + # pylint: enable=duplicate-code + self.delete_system(system_id) + self.check_delete_failed_with_message(409, "System has child elements and cannot be deleted") -def test_partial_update_all_other_fields(test_client): - """ - Test updating the rest of a systems fields not tested above - """ - _test_partial_update_system( - test_client, - { - "description": "Updated description", - "location": "Updated location", - "owner": "Updated owner", - "importance": "high", - }, - ) + def test_delete_with_non_existent_id(self): + """Test deleting a non-existent System""" + self.delete_system(str(ObjectId())) + self.check_delete_failed_with_message(404, "System not found") -def test_partial_update_system_invalid_id(test_client): - """ - Test updating a System when the ID is invalid - """ - response = test_client.patch("/v1/systems/invalid", json={"name": "Updated name"}) - - assert response.status_code == 404 - assert response.json()["detail"] == "System not found" + def test_delete_with_invalid_id(self): + """Test deleting a System with an invalid id""" - -def test_partial_update_system_non_existent_id(test_client): - """ - Test updating a System when the ID is non-existent - """ - response = test_client.patch(f"/v1/systems/{str(ObjectId())}", json={"name": "Updated name"}) - - assert response.status_code == 404 - assert response.json()["detail"] == "System not found" - - -def test_delete_system(test_client): - """ - Test deleting a System - """ - # Create one to delete - response = test_client.post("/v1/systems", json=SYSTEM_POST_A) - system = response.json() - - # Delete - response = test_client.delete(f"/v1/systems/{system['id']}") - - assert response.status_code == 204 - response = test_client.get(f"/v1/systems/{system['id']}") - assert response.status_code == 404 - - -def test_delete_system_with_invalid_id(test_client): - """ - Test deleting a System with an invalid ID - """ - # Delete - response = test_client.delete("/v1/systems/invalid") - - assert response.status_code == 404 - assert response.json()["detail"] == "System not found" - - -def test_delete_system_with_non_existent_id(test_client): - """ - Test deleting a System with a non-existent ID - """ - # Delete - response = test_client.delete(f"/v1/systems/{str(ObjectId())}") - - assert response.status_code == 404 - assert response.json()["detail"] == "System not found" - - -def test_delete_system_with_child_system(test_client): - """ - Test deleting a System that has subsystem - """ - # Create one to delete - # Parent - response = test_client.post("/v1/systems", json=SYSTEM_POST_A) - parent_system = response.json() - - # Child - test_client.post("/v1/systems", json={**SYSTEM_POST_B, "parent_id": parent_system["id"]}) - - # Delete - response = test_client.delete(f"/v1/systems/{parent_system['id']}") - - assert response.status_code == 409 - assert response.json()["detail"] == "System has child elements and cannot be deleted" - - -def test_delete_system_with_child_item(test_client): - """ - Test deleting a System that contains an item - """ - # Create one to delete - response = test_client.post("/v1/systems", json=SYSTEM_POST_A) - system_id = response.json()["id"] - - # Create a child item - # pylint: disable=duplicate-code - response = test_client.post("/v1/units", json=UNIT_POST_A) - unit_mm = response.json() - - response = test_client.post("/v1/units", json=UNIT_POST_B) - unit_cm = response.json() - - units = [unit_mm, unit_cm] - - response = test_client.post( - "/v1/catalogue-categories", - json={ - **CATALOGUE_CATEGORY_POST_A, - "properties": replace_unit_values_with_ids_in_properties(CATALOGUE_CATEGORY_POST_A["properties"], units), - }, - ) - catalogue_category = response.json() - - response = test_client.post("/v1/manufacturers", json=MANUFACTURER_POST) - manufacturer_id = response.json()["id"] - - catalogue_item_post = { - **CATALOGUE_ITEM_POST_A, - "catalogue_category_id": catalogue_category["id"], - "manufacturer_id": manufacturer_id, - "properties": add_ids_to_properties(catalogue_category["properties"], CATALOGUE_ITEM_POST_A["properties"]), - } - response = test_client.post("/v1/catalogue-items", json=catalogue_item_post) - catalogue_item_id = response.json()["id"] - - response = test_client.post("/v1/usage-statuses", json=USAGE_STATUS_POST_B) - usage_status_id = response.json()["id"] - - item_post = { - **ITEM_POST, - "catalogue_item_id": catalogue_item_id, - "system_id": system_id, - "usage_status_id": usage_status_id, - "properties": add_ids_to_properties(catalogue_category["properties"], ITEM_POST["properties"]), - } - test_client.post("/v1/items", json=item_post) - # pylint: enable=duplicate-code - - # Delete - response = test_client.delete(f"/v1/systems/{system_id}") - - assert response.status_code == 409 - assert response.json()["detail"] == "System has child elements and cannot be deleted" + self.delete_system("invalid_id") + self.check_delete_failed_with_message(404, "System not found") From 4bb51dade0d42c3efc72c4a4b8453393c6d88607 Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Mon, 24 Jun 2024 11:29:15 +0000 Subject: [PATCH 5/9] Move all mock data for system tests to same place and improve names #302 --- test/e2e/test_system.py | 116 ++++++++++---------------- test/mock_data.py | 79 ++++++++++++++++++ test/unit/mock_data.py | 38 --------- test/unit/repositories/test_system.py | 2 +- test/unit/services/test_system.py | 2 +- 5 files changed, 123 insertions(+), 114 deletions(-) create mode 100644 test/mock_data.py delete mode 100644 test/unit/mock_data.py diff --git a/test/e2e/test_system.py b/test/e2e/test_system.py index af60b90e..6edcd88c 100644 --- a/test/e2e/test_system.py +++ b/test/e2e/test_system.py @@ -4,20 +4,17 @@ from test.conftest import add_ids_to_properties from test.e2e.conftest import replace_unit_values_with_ids_in_properties -from test.e2e.mock_schemas import ( - CREATED_MODIFIED_VALUES_EXPECTED, - SYSTEM_POST_A, - SYSTEM_POST_A_EXPECTED, - SYSTEM_POST_B, - SYSTEM_POST_B_EXPECTED, - SYSTEM_POST_C, - USAGE_STATUS_POST_B, -) +from test.e2e.mock_schemas import USAGE_STATUS_POST_B from test.e2e.test_catalogue_item import CATALOGUE_CATEGORY_POST_A, CATALOGUE_ITEM_POST_A from test.e2e.test_item import ITEM_POST, MANUFACTURER_POST from test.e2e.test_unit import UNIT_POST_A, UNIT_POST_B -from typing import Any, Optional -from unittest.mock import ANY +from test.mock_data import ( + SYSTEM_GET_DATA_ALL_VALUES_NO_PARENT, + SYSTEM_GET_DATA_REQUIRED_VALUES_ONLY, + SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT, + SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, +) +from typing import Optional import pytest from bson import ObjectId @@ -26,40 +23,6 @@ from inventory_management_system_api.core.consts import BREADCRUMBS_TRAIL_MAX_LENGTH -# TODO: Unify with others - could perhaps even reuse here too -SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY = { - "name": "System Test Required Values Only", - "importance": "low", -} - -SYSTEM_GET_DATA_REQUIRED_VALUES_ONLY = { - **SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, - **CREATED_MODIFIED_VALUES_EXPECTED, - "id": ANY, - "parent_id": None, - "description": None, - "location": None, - "owner": None, - "code": "system-test-required-values-only", -} - -SYSTEM_POST_DATA_ALL_VALUES = { - **SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, - "name": "System Test All Values", - "parent_id": None, - "description": "Test description", - "location": "Test location", - "owner": "Test owner", -} - -SYSTEM_GET_DATA_ALL_VALUES = { - **SYSTEM_POST_DATA_ALL_VALUES, - **CREATED_MODIFIED_VALUES_EXPECTED, - "id": ANY, - "parent_id": None, - "code": "system-test-all-values", -} - class CreateDSL: """Base class for create tests""" @@ -93,8 +56,8 @@ def check_post_system_failed_with_message(self, status_code: int, detail: str): assert self._post_response.json()["detail"] == detail def check_post_system_failed_with_validation_message(self, status_code: int, message: str): - """Checks that a prior call to 'post_system' gave a failed response with the expected code and pydantic validation - error message""" + """Checks that a prior call to 'post_system' gave a failed response with the expected code and pydantic + validation error message""" assert self._post_response.status_code == status_code assert self._post_response.json()["detail"][0]["msg"] == message @@ -112,35 +75,35 @@ def test_create_with_only_required_values_provided(self): def test_create_with_all_values_provided(self): """Test creating a System with all values provided""" - self.post_system(SYSTEM_POST_DATA_ALL_VALUES) - self.check_post_system_success(SYSTEM_GET_DATA_ALL_VALUES) + self.post_system(SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT) + self.check_post_system_success(SYSTEM_GET_DATA_ALL_VALUES_NO_PARENT) def test_create_with_valid_parent_id(self): """Test creating a System with a valid parent id""" - parent_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) - self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "parent_id": parent_id}) - self.check_post_system_success({**SYSTEM_GET_DATA_ALL_VALUES, "parent_id": parent_id}) + parent_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT) + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT, "parent_id": parent_id}) + self.check_post_system_success({**SYSTEM_GET_DATA_ALL_VALUES_NO_PARENT, "parent_id": parent_id}) def test_create_with_non_existent_parent_id(self): """Test creating a System with a non-existent parent id""" - self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "parent_id": str(ObjectId())}) + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT, "parent_id": str(ObjectId())}) self.check_post_system_failed_with_message(422, "The specified parent System does not exist") def test_create_with_invalid_parent_id(self): """Test creating a System with an invalid parent id""" - self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "parent_id": "invalid-id"}) + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT, "parent_id": "invalid-id"}) self.check_post_system_failed_with_message(422, "The specified parent System does not exist") def test_create_with_duplicate_name_within_parent(self): """Test creating a System with the same name as another within the same parent""" - parent_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + parent_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT) # 2nd post should be the duplicate - self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "parent_id": parent_id}) - self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "parent_id": parent_id}) + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT, "parent_id": parent_id}) + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT, "parent_id": parent_id}) self.check_post_system_failed_with_message( 409, "A System with the same name already exists within the same parent System" ) @@ -148,7 +111,7 @@ def test_create_with_duplicate_name_within_parent(self): def test_create_with_invalid_importance(self): """Test creating a System with an invalid importance""" - self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "importance": "invalid-importance"}) + self.post_system({**SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT, "importance": "invalid-importance"}) self.check_post_system_failed_with_validation_message(422, "Input should be 'low', 'medium' or 'high'") @@ -181,9 +144,9 @@ class TestGet(GetDSL): def test_get(self): """Test getting a System""" - system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT) self.get_system(system_id) - self.check_get_success(SYSTEM_GET_DATA_ALL_VALUES) + self.check_get_success(SYSTEM_GET_DATA_ALL_VALUES_NO_PARENT) def test_get_with_non_existent_id(self): """Test getting a System with a non-existent id""" @@ -209,7 +172,9 @@ def post_nested_systems(self, number: int) -> list[str]: parent_id = None for i in range(0, number): - system_id = self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "name": f"System {i}", "parent_id": parent_id}) + system_id = self.post_system( + {**SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT, "name": f"System {i}", "parent_id": parent_id} + ) self._posted_systems_get_data.append(self._post_response.json()) parent_id = system_id @@ -226,7 +191,8 @@ def get_last_system_breadcrumbs(self): self.get_system_breadcrumbs(self._posted_systems_get_data[-1]["id"]) def check_get_breadcrumbs_success(self, expected_trail_length: int, expected_full_trail: bool): - """Checks that a prior call to 'get_system_breadcrumbs' gave a successful response with the expected data returned""" + """Checks that a prior call to 'get_system_breadcrumbs' gave a successful response with the expected data + returned""" assert self._get_response.status_code == 200 assert self._get_response.json() == { @@ -312,10 +278,10 @@ def list_systems(self, filters: dict): def post_test_system_with_child(self) -> list[dict]: """Posts a System with a single child and returns their expected responses when returned by the list endpoint""" - parent_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + parent_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT) self.post_system({**SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, "parent_id": parent_id}) - return [SYSTEM_GET_DATA_ALL_VALUES, {**SYSTEM_GET_DATA_REQUIRED_VALUES_ONLY, "parent_id": parent_id}] + return [SYSTEM_GET_DATA_ALL_VALUES_NO_PARENT, {**SYSTEM_GET_DATA_REQUIRED_VALUES_ONLY, "parent_id": parent_id}] def check_list_success(self, expected_systems_get_data: list[dict]): """Checks that a prior call to 'list' gave a successful response with the expected data returned""" @@ -409,17 +375,17 @@ def test_partial_update_all_fields(self): """Test updating every field of a System""" system_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) - self.patch_system(system_id, SYSTEM_POST_DATA_ALL_VALUES) - self.check_patch_system_response_success(SYSTEM_GET_DATA_ALL_VALUES) + self.patch_system(system_id, SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT) + self.check_patch_system_response_success(SYSTEM_GET_DATA_ALL_VALUES_NO_PARENT) def test_partial_update_parent_id(self): """Test updating the parent_id of a System""" parent_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) - system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT) self.patch_system(system_id, {"parent_id": parent_id}) - self.check_patch_system_response_success({**SYSTEM_GET_DATA_ALL_VALUES, "parent_id": parent_id}) + self.check_patch_system_response_success({**SYSTEM_GET_DATA_ALL_VALUES_NO_PARENT, "parent_id": parent_id}) def test_partial_update_parent_id_to_one_with_a_duplicate_name(self): """Test updating the parent_id of a System so that its name conflicts with one already in that other @@ -429,7 +395,7 @@ def test_partial_update_parent_id_to_one_with_a_duplicate_name(self): parent_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) self.post_system({**SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, "name": "Conflicting Name", "parent_id": parent_id}) - system_id = self.post_system({**SYSTEM_POST_DATA_ALL_VALUES, "name": "Conflicting Name"}) + system_id = self.post_system({**SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT, "name": "Conflicting Name"}) self.patch_system(system_id, {"parent_id": parent_id}) self.check_patch_system_failed_with_message( @@ -446,14 +412,14 @@ def test_partial_update_parent_id_to_child_of_self(self): def test_partial_update_parent_id_to_non_existent(self): """Test updating the parent_id of a System to a non-existent System""" - system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT) self.patch_system(system_id, {"parent_id": str(ObjectId())}) self.check_patch_system_failed_with_message(422, "The specified parent System does not exist") def test_partial_update_parent_id_to_invalid(self): """Test updating the parent_id of a System to an invalid id""" - system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT) self.patch_system(system_id, {"parent_id": "invalid-id"}) self.check_patch_system_failed_with_message(422, "The specified parent System does not exist") @@ -461,14 +427,15 @@ def test_partial_update_name_to_duplicate(self): """Test updating the name of a System to conflict with a pre-existing one""" self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) - system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES) + system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT) self.patch_system(system_id, {"name": SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY["name"]}) self.check_patch_system_failed_with_message( 409, "A System with the same name already exists within the parent System" ) def test_partial_update_name_capitalisation(self): - """Test updating the capitalisation of the name of a System (to ensure it the check doesn't confuse with duplicates)""" + """Test updating the capitalisation of the name of a System (to ensure it the check doesn't confuse with + duplicates)""" system_id = self.post_system({**SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, "name": "Test system"}) self.patch_system(system_id, {"name": "Test System"}) @@ -505,7 +472,8 @@ def check_delete_success(self): assert self._delete_response.status_code == 204 def check_delete_failed_with_message(self, status_code: int, detail: str): - """Checks that a prior call to 'delete_system' gave a failed response with the expected code and error message""" + """Checks that a prior call to 'delete_system' gave a failed response with the expected code and error + message""" assert self._delete_response.status_code == status_code assert self._delete_response.json()["detail"] == detail diff --git a/test/mock_data.py b/test/mock_data.py new file mode 100644 index 00000000..63db2670 --- /dev/null +++ b/test/mock_data.py @@ -0,0 +1,79 @@ +""" +Mock data for use in unit tests + +Names should ideally be descriptive enough to recognise what they are without looking at the data itself. +Letters may be appended in places to indicate the data is of the same type, but has different specific values +to others. + +_POST_DATA - Is for a `PostSchema` schema +_IN_DATA - Is for an `In` model +_GET_DATA - Is for an entity schema - Used in assertions for e2e tests +""" + +from unittest.mock import ANY + +# Used for _GET_DATA's as when comparing these will not be possible to know +# at runtime +CREATED_MODIFIED_GET_DATA_EXPECTED = {"created_time": ANY, "modified_time": ANY} + + +SYSTEM_POST_DATA_NO_PARENT_A = { + "parent_id": None, + "name": "Test name A", + "description": "Test description A", + "location": "Test location A", + "owner": "Test owner A", + "importance": "low", +} + +SYSTEM_POST_DATA_NO_PARENT_B = { + "parent_id": None, + "name": "Test name B", + "description": "Test description B", + "location": "Test location B", + "owner": "Test owner B", + "importance": "low", +} + +SYSTEM_IN_DATA_NO_PARENT_A = { + **SYSTEM_POST_DATA_NO_PARENT_A, + "code": "test-name-a", +} + +SYSTEM_IN_DATA_NO_PARENT_B = { + **SYSTEM_POST_DATA_NO_PARENT_B, + "code": "test-name-b", +} + +SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY = { + "name": "System Test Required Values Only", + "importance": "low", +} + +SYSTEM_GET_DATA_REQUIRED_VALUES_ONLY = { + **SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, + **CREATED_MODIFIED_GET_DATA_EXPECTED, + "id": ANY, + "parent_id": None, + "description": None, + "location": None, + "owner": None, + "code": "system-test-required-values-only", +} + +SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT = { + **SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, + "name": "System Test All Values", + "parent_id": None, + "description": "Test description", + "location": "Test location", + "owner": "Test owner", +} + +SYSTEM_GET_DATA_ALL_VALUES_NO_PARENT = { + **SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT, + **CREATED_MODIFIED_GET_DATA_EXPECTED, + "id": ANY, + "parent_id": None, + "code": "system-test-all-values", +} diff --git a/test/unit/mock_data.py b/test/unit/mock_data.py deleted file mode 100644 index 268e62db..00000000 --- a/test/unit/mock_data.py +++ /dev/null @@ -1,38 +0,0 @@ -""" -Mock data for use in unit tests - -Names should ideally be descriptive enough to recognise what they are without looking at the data itself. -Letters may be appended in places to indicate the data is of the same type, but has different specific values -to others. - -_POST_DATA - Is for a `PostSchema` schema -_IN_DATA - Is for an `In` model -""" - -SYSTEM_POST_DATA_NO_PARENT_A = { - "parent_id": None, - "name": "Test name A", - "description": "Test description A", - "location": "Test location A", - "owner": "Test owner A", - "importance": "low", -} - -SYSTEM_POST_DATA_NO_PARENT_B = { - "parent_id": None, - "name": "Test name B", - "description": "Test description B", - "location": "Test location B", - "owner": "Test owner B", - "importance": "low", -} - -SYSTEM_IN_DATA_NO_PARENT_A = { - **SYSTEM_POST_DATA_NO_PARENT_A, - "code": "test-name-a", -} - -SYSTEM_IN_DATA_NO_PARENT_B = { - **SYSTEM_POST_DATA_NO_PARENT_B, - "code": "test-name-b", -} diff --git a/test/unit/repositories/test_system.py b/test/unit/repositories/test_system.py index aee8c124..f25e3b28 100644 --- a/test/unit/repositories/test_system.py +++ b/test/unit/repositories/test_system.py @@ -2,7 +2,7 @@ Unit tests for the `SystemRepo` repository """ -from test.unit.mock_data import SYSTEM_IN_DATA_NO_PARENT_A, SYSTEM_IN_DATA_NO_PARENT_B +from test.mock_data import SYSTEM_IN_DATA_NO_PARENT_A, SYSTEM_IN_DATA_NO_PARENT_B from test.unit.repositories.conftest import RepositoryTestHelpers from test.unit.repositories.test_utils import ( MOCK_BREADCRUMBS_QUERY_RESULT_LESS_THAN_MAX_LENGTH, diff --git a/test/unit/services/test_system.py b/test/unit/services/test_system.py index 83898dec..387c5c7c 100644 --- a/test/unit/services/test_system.py +++ b/test/unit/services/test_system.py @@ -2,7 +2,7 @@ Unit tests for the `SystemService` service """ -from test.unit.mock_data import SYSTEM_POST_DATA_NO_PARENT_A, SYSTEM_POST_DATA_NO_PARENT_B +from test.mock_data import SYSTEM_POST_DATA_NO_PARENT_A, SYSTEM_POST_DATA_NO_PARENT_B from test.unit.services.conftest import ServiceTestHelpers from typing import Optional from unittest.mock import MagicMock, Mock, patch From 3155decffe01cf44c42ad74e14ca0f8ca4b1326b Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Mon, 24 Jun 2024 11:58:42 +0000 Subject: [PATCH 6/9] Fix test interference and remove unnecessary mock data #302 --- test/e2e/mock_schemas.py | 15 --------------- test/e2e/test_system.py | 16 ++++++++++++---- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/test/e2e/mock_schemas.py b/test/e2e/mock_schemas.py index 6a9a0264..65b700d9 100644 --- a/test/e2e/mock_schemas.py +++ b/test/e2e/mock_schemas.py @@ -118,21 +118,6 @@ "id": ANY, "code": "system-b", } - -SYSTEM_POST_C = { - "name": "System C", - "description": "System description", - "location": "Test location", - "owner": "Me", - "importance": "low", -} -SYSTEM_POST_C_EXPECTED = { - **SYSTEM_POST_C, - **CREATED_MODIFIED_VALUES_EXPECTED, - "id": ANY, - "parent_id": None, - "code": "system-c", -} # pylint: enable=duplicate-code USAGE_STATUS_POST_A = {"value": "New"} diff --git a/test/e2e/test_system.py b/test/e2e/test_system.py index 6edcd88c..9cc3cdad 100644 --- a/test/e2e/test_system.py +++ b/test/e2e/test_system.py @@ -165,7 +165,13 @@ class GetBreadcrumbsDSL(CreateDSL): """Base class for breadcrumbs tests""" _get_response: Response - _posted_systems_get_data = [] + _posted_systems_get_data: list[dict] + + @pytest.fixture(autouse=True) + def setup_breadcrumbs_dsl(self): + """Setup fixtures""" + + self._posted_systems_get_data = [] def post_nested_systems(self, number: int) -> list[str]: """Posts the given number of nested systems where each successive one has the previous as its parent""" @@ -344,7 +350,6 @@ def test_list_with_invalid_parent_id_filter(self): self.check_list_success([]) -# TODO: Put nested system stuff in create rather than get then change this to that class UpdateDSL(GetBreadcrumbsDSL): """Base class for update tests""" @@ -456,7 +461,7 @@ def test_partial_update_invalid_id(self): self.check_patch_system_failed_with_message(404, "System not found") -class DeleteDSL(CreateDSL): +class DeleteDSL(GetDSL): """Base class for delete tests""" _delete_response: Response @@ -489,6 +494,9 @@ def test_delete(self): self.delete_system(system_id) self.check_delete_success() + self.get_system(system_id) + self.check_get_failed_with_message(404, "System not found") + def test_delete_with_child_system(self): """Test deleting a System with a child system""" @@ -500,7 +508,7 @@ def test_delete_with_child_system(self): def test_delete_with_child_item(self): """Test deleting a System with a child system""" - # TODO: Cleanup in future + # THIS SHOULD BE CLEANED UP IN FUTURE system_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) self.post_system({**SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, "parent_id": system_id}) From 9e25e1ae213c33a6ed07c87b63db137feaac5a34 Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Mon, 24 Jun 2024 13:53:06 +0000 Subject: [PATCH 7/9] Add some comments and improve name consistency #302 --- test/e2e/test_system.py | 77 ++++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 35 deletions(-) diff --git a/test/e2e/test_system.py b/test/e2e/test_system.py index 9cc3cdad..50a3a1f1 100644 --- a/test/e2e/test_system.py +++ b/test/e2e/test_system.py @@ -29,16 +29,21 @@ class CreateDSL: test_client: TestClient - _post_response: Response + _post_response: list[Response] @pytest.fixture(autouse=True) def setup(self, test_client): """Setup fixtures""" self.test_client = test_client + self._post_response = [] def post_system(self, system_post_data: dict) -> Optional[str]: - """Posts a System with the given data, returns the id of the created system if successful""" + """Posts a System with the given data, returns the id of the created system if successful + + :param system_post_data: Dictionary containing the system data that should be posted + :return: ID of the created system (or None if not successful) + """ self._post_response = self.test_client.post("/v1/systems", json=system_post_data) return self._post_response.json()["id"] if self._post_response.status_code == 201 else None @@ -125,13 +130,13 @@ def get_system(self, system_id: str): self._get_response = self.test_client.get(f"/v1/systems/{system_id}") - def check_get_success(self, expected_system_get_data: dict): + def check_get_system_success(self, expected_system_get_data: dict): """Checks that a prior call to 'get_system' gave a successful response with the expected data returned""" assert self._get_response.status_code == 200 assert self._get_response.json() == expected_system_get_data - def check_get_failed_with_message(self, status_code: int, detail: str): + def check_get_system_failed_with_message(self, status_code: int, detail: str): """Checks that a prior call to 'get_system' gave a failed response with the expected code and error message""" assert self._get_response.status_code == status_code @@ -146,25 +151,26 @@ def test_get(self): system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT) self.get_system(system_id) - self.check_get_success(SYSTEM_GET_DATA_ALL_VALUES_NO_PARENT) + self.check_get_system_success(SYSTEM_GET_DATA_ALL_VALUES_NO_PARENT) def test_get_with_non_existent_id(self): """Test getting a System with a non-existent id""" self.get_system(str(ObjectId())) - self.check_get_failed_with_message(404, "System not found") + self.check_get_system_failed_with_message(404, "System not found") def test_get_with_invalid_id(self): """Test getting a System with an invalid id""" self.get_system("invalid-id") - self.check_get_failed_with_message(404, "System not found") + self.check_get_system_failed_with_message(404, "System not found") -class GetBreadcrumbsDSL(CreateDSL): +class GetBreadcrumbsDSL(GetDSL): """Base class for breadcrumbs tests""" _get_response: Response + _posted_systems_get_data: list[dict] @pytest.fixture(autouse=True) @@ -173,8 +179,12 @@ def setup_breadcrumbs_dsl(self): self._posted_systems_get_data = [] - def post_nested_systems(self, number: int) -> list[str]: - """Posts the given number of nested systems where each successive one has the previous as its parent""" + def post_nested_systems(self, number: int) -> list[Optional[str]]: + """Posts the given number of nested systems where each successive one has the previous as its parent + + :param number: Number of Systems to create + :return: List of ids of the created Systems + """ parent_id = None for i in range(0, number): @@ -194,18 +204,18 @@ def get_system_breadcrumbs(self, system_id: str): def get_last_system_breadcrumbs(self): """Gets the last System posted's breadcrumbs""" - self.get_system_breadcrumbs(self._posted_systems_get_data[-1]["id"]) + self.get_system_breadcrumbs(self._post_response.json()["id"]) def check_get_breadcrumbs_success(self, expected_trail_length: int, expected_full_trail: bool): """Checks that a prior call to 'get_system_breadcrumbs' gave a successful response with the expected data - returned""" + returned + """ assert self._get_response.status_code == 200 assert self._get_response.json() == { "trail": [ [system["id"], system["name"]] # When the expected trail length is < the number of systems posted, only use the last - # few systems inside the trail for system in self._posted_systems_get_data[ (len(self._posted_systems_get_data) - expected_trail_length) : ] @@ -271,12 +281,10 @@ def test_get_breadcrumbs_with_invalid_id(self): self.check_get_breadcrumbs_failed_with_message(404, "System not found") -class ListDSL(CreateDSL): +class ListDSL(GetBreadcrumbsDSL): """Base class for list tests""" - _get_response: Response - - def list_systems(self, filters: dict): + def get_systems(self, filters: dict): """Gets a list Systems with the given filters""" self._get_response = self.test_client.get("/v1/systems", params=filters) @@ -289,13 +297,13 @@ def post_test_system_with_child(self) -> list[dict]: return [SYSTEM_GET_DATA_ALL_VALUES_NO_PARENT, {**SYSTEM_GET_DATA_REQUIRED_VALUES_ONLY, "parent_id": parent_id}] - def check_list_success(self, expected_systems_get_data: list[dict]): + def check_get_systems_success(self, expected_systems_get_data: list[dict]): """Checks that a prior call to 'list' gave a successful response with the expected data returned""" assert self._get_response.status_code == 200 assert self._get_response.json() == expected_systems_get_data - def check_list_failed_with_message(self, status_code: int, detail: str): + def check_get_systems_failed_with_message(self, status_code: int, detail: str): """Checks that a prior call to 'list' gave a failed response with the expected code and error message""" assert self._get_response.status_code == status_code @@ -312,8 +320,8 @@ def test_list_with_no_filters(self): """ systems = self.post_test_system_with_child() - self.list_systems(filters={}) - self.check_list_success(systems) + self.get_systems(filters={}) + self.check_get_systems_success(systems) def test_list_with_parent_id_filter(self): """Test getting a list of all Systems with a parent_id filter provided @@ -323,8 +331,8 @@ def test_list_with_parent_id_filter(self): """ systems = self.post_test_system_with_child() - self.list_systems(filters={"parent_id": systems[1]["parent_id"]}) - self.check_list_success([systems[1]]) + self.get_systems(filters={"parent_id": systems[1]["parent_id"]}) + self.check_get_systems_success([systems[1]]) def test_list_with_null_parent_id_filter(self): """Test getting a list of all Systems with a parent_id filter of "null" provided @@ -334,23 +342,23 @@ def test_list_with_null_parent_id_filter(self): """ systems = self.post_test_system_with_child() - self.list_systems(filters={"parent_id": "null"}) - self.check_list_success([systems[0]]) + self.get_systems(filters={"parent_id": "null"}) + self.check_get_systems_success([systems[0]]) def test_list_with_parent_id_filter_with_no_matching_results(self): """Test getting a list of all Systems with a parent_id filter that returns no results""" - self.list_systems(filters={"parent_id": str(ObjectId())}) - self.check_list_success([]) + self.get_systems(filters={"parent_id": str(ObjectId())}) + self.check_get_systems_success([]) def test_list_with_invalid_parent_id_filter(self): """Test getting a list of all Systems with an invalid parent_id filter returns no results""" - self.list_systems(filters={"parent_id": "invalid-id"}) - self.check_list_success([]) + self.get_systems(filters={"parent_id": "invalid-id"}) + self.check_get_systems_success([]) -class UpdateDSL(GetBreadcrumbsDSL): +class UpdateDSL(ListDSL): """Base class for update tests""" _patch_response: Response @@ -461,7 +469,7 @@ def test_partial_update_invalid_id(self): self.check_patch_system_failed_with_message(404, "System not found") -class DeleteDSL(GetDSL): +class DeleteDSL(UpdateDSL): """Base class for delete tests""" _delete_response: Response @@ -495,14 +503,13 @@ def test_delete(self): self.check_delete_success() self.get_system(system_id) - self.check_get_failed_with_message(404, "System not found") + self.check_get_system_failed_with_message(404, "System not found") def test_delete_with_child_system(self): """Test deleting a System with a child system""" - system_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) - self.post_system({**SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, "parent_id": system_id}) - self.delete_system(system_id) + system_ids = self.post_nested_systems(2) + self.delete_system(system_ids[0]) self.check_delete_failed_with_message(409, "System has child elements and cannot be deleted") def test_delete_with_child_item(self): From 03920e1a6426010e3925442e0e3682fbf5116c19 Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Tue, 25 Jun 2024 09:42:15 +0000 Subject: [PATCH 8/9] Fixes for various type errors addressing comments #302 --- test/e2e/test_system.py | 5 ++-- test/unit/repositories/test_system.py | 34 +++++++++++++-------------- test/unit/services/test_system.py | 4 ++-- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/test/e2e/test_system.py b/test/e2e/test_system.py index 50a3a1f1..804e5c2c 100644 --- a/test/e2e/test_system.py +++ b/test/e2e/test_system.py @@ -18,8 +18,8 @@ import pytest from bson import ObjectId -from fastapi import Response from fastapi.testclient import TestClient +from httpx import Response from inventory_management_system_api.core.consts import BREADCRUMBS_TRAIL_MAX_LENGTH @@ -29,14 +29,13 @@ class CreateDSL: test_client: TestClient - _post_response: list[Response] + _post_response: Response @pytest.fixture(autouse=True) def setup(self, test_client): """Setup fixtures""" self.test_client = test_client - self._post_response = [] def post_system(self, system_post_data: dict) -> Optional[str]: """Posts a System with the given data, returns the id of the created system if successful diff --git a/test/unit/repositories/test_system.py b/test/unit/repositories/test_system.py index f25e3b28..8cd9a8b4 100644 --- a/test/unit/repositories/test_system.py +++ b/test/unit/repositories/test_system.py @@ -88,7 +88,7 @@ def mock_create( # When a parent_id is given, need to mock the find_one for it too if system_in_data["parent_id"]: - # If parent_system_data is given as none, then it is intentionally supposed to be, otherwise + # If parent_system_data is given as None, then it is intentionally supposed to be, otherwise # pass through SystemIn first to ensure it has creation and modified times self.test_helpers.mock_find_one( self.systems_collection, @@ -116,7 +116,7 @@ def call_create(self): self._created_system = self.system_repository.create(self._system_in, session=self.mock_session) - def call_create_expecting_error(self, error_type: type): + def call_create_expecting_error(self, error_type: type[BaseException]): """Calls the SystemRepo `create` method with the appropriate data from a prior call to `mock_create` while expecting an error to be raised""" @@ -210,7 +210,7 @@ class GetDSL(SystemRepoDSL): """Base class for get tests""" _obtained_system_id: str - _expected_system_out: SystemOut + _expected_system_out: Optional[SystemOut] _obtained_system: Optional[SystemOut] _get_exception: pytest.ExceptionInfo @@ -238,7 +238,7 @@ def call_get(self, system_id: str): self._obtained_system_id = system_id self._obtained_system = self.system_repository.get(system_id, session=self.mock_session) - def call_get_expecting_error(self, system_id: str, error_type: type): + def call_get_expecting_error(self, system_id: str, error_type: type[BaseException]): """Calls the SystemRepo `get` method with the appropriate data from a prior call to `mock_get` while expecting an error to be raised""" @@ -296,17 +296,17 @@ def test_get_with_invalid_id(self): class GetBreadcrumbsDSL(SystemRepoDSL): """Base class for breadcrumbs tests""" - _breadcrumbs_query_result: dict + _breadcrumbs_query_result: list[dict] _mock_aggregation_pipeline = MagicMock() _expected_breadcrumbs: MagicMock _obtained_system_id: str _obtained_breadcrumbs: MagicMock - def mock_breadcrumbs(self, breadcrumbs_query_result: dict): + def mock_breadcrumbs(self, breadcrumbs_query_result: list[dict]): """Mocks database methods appropriately to test the 'get_breadcrumbs' repo method - :param breadcrumbs_query_result: Dictionary containing the breadcrumbs query result from the aggregation - pipeline + :param breadcrumbs_query_result: List of dictionaries containing the breadcrumbs query result from the + aggregation pipeline """ self._breadcrumbs_query_result = breadcrumbs_query_result self._mock_aggregation_pipeline = MagicMock() @@ -353,14 +353,14 @@ class ListDSL(SystemRepoDSL): """Base class for list tests""" _expected_systems_out: list[SystemOut] - _parent_id_filter: str + _parent_id_filter: Optional[str] _obtained_systems_out: list[SystemOut] def mock_list(self, systems_in_data: list[dict]): """Mocks database methods appropriately to test the 'list' repo method - :param systems_data: List of dictionaries containing the system data as would be required for a - SystemIn database model (i.e. no id or created and modified times required)""" + :param systems_in_data: List of dictionaries containing the system data as would be required for a + SystemIn database model (i.e. no id or created and modified times required)""" self._expected_systems_out = [ SystemOut(**SystemIn(**system_in_data).model_dump(), id=ObjectId()) for system_in_data in systems_in_data @@ -425,7 +425,7 @@ class UpdateDSL(SystemRepoDSL): # pylint:disable=too-many-instance-attributes _system_in: SystemIn - _stored_system: SystemOut + _stored_system: Optional[SystemOut] _expected_system_out: SystemOut _updated_system_id: str _updated_system: SystemOut @@ -488,10 +488,10 @@ def mock_update( ) # Duplicate check - self._moving_system = stored_system_in_data and ( + self._moving_system = stored_system_in_data is not None and ( new_system_in_data["parent_id"] != stored_system_in_data["parent_id"] ) - if (stored_system_in_data and (self._system_in.name != self._stored_system.name)) or self._moving_system: + if (self._stored_system and (self._system_in.name != self._stored_system.name)) or self._moving_system: self.test_helpers.mock_find_one( self.systems_collection, ( @@ -522,7 +522,7 @@ def call_update(self, system_id: str): self._updated_system_id = system_id self._updated_system = self.system_repository.update(system_id, self._system_in, session=self.mock_session) - def call_update_expecting_error(self, system_id: str, error_type: type): + def call_update_expecting_error(self, system_id: str, error_type: type[BaseException]): """Calls the SystemRepo `update` method with the appropriate data from a prior call to `mock_update` (or `set_update_data`) while expecting an error to be raised""" @@ -549,7 +549,7 @@ def check_update_success(self): ) # Duplicate check (which only runs if moving or changing the name) - if (self._system_in.name != self._stored_system.name) or self._moving_system: + if (self._stored_system and (self._system_in.name != self._stored_system.name)) or self._moving_system: expected_find_one_calls.append( call( { @@ -730,7 +730,7 @@ def call_delete(self, system_id: str): self._delete_system_id = system_id self.system_repository.delete(system_id, session=self.mock_session) - def call_delete_expecting_error(self, system_id: str, error_type: type): + def call_delete_expecting_error(self, system_id: str, error_type: type[BaseException]): """Calls the SystemRepo `delete` method while expecting an error to be raised""" self._delete_system_id = system_id diff --git a/test/unit/services/test_system.py b/test/unit/services/test_system.py index 387c5c7c..694fd100 100644 --- a/test/unit/services/test_system.py +++ b/test/unit/services/test_system.py @@ -184,7 +184,7 @@ def test_get_breadcrumbs(self): class ListDSL(SystemServiceDSL): """Base class for list tests""" - _parent_id_filter: str + _parent_id_filter: Optional[str] _expected_systems: MagicMock _obtained_systems: MagicMock @@ -275,7 +275,7 @@ def call_update(self, system_id: str): self._updated_system_id = system_id self._updated_system = self.system_service.update(system_id, self._system_patch) - def call_update_expecting_error(self, system_id: str, error_type: type): + def call_update_expecting_error(self, system_id: str, error_type: type[BaseException]): """Calls the SystemService `update` method with the appropriate data from a prior call to `mock_update` while expecting an error to be raised""" From 17c139716f9065a984b665b0490f3bde772b2d96 Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Tue, 25 Jun 2024 11:04:27 +0000 Subject: [PATCH 9/9] Add comment on wrapped utils and a todo as suggested #302 --- test/e2e/test_system.py | 3 ++- test/unit/repositories/test_system.py | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/test/e2e/test_system.py b/test/e2e/test_system.py index 804e5c2c..bd8581e0 100644 --- a/test/e2e/test_system.py +++ b/test/e2e/test_system.py @@ -514,7 +514,8 @@ def test_delete_with_child_system(self): def test_delete_with_child_item(self): """Test deleting a System with a child system""" - # THIS SHOULD BE CLEANED UP IN FUTURE + # pylint:disable=fixme + # TODO: THIS SHOULD BE CLEANED UP IN FUTURE system_id = self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY) self.post_system({**SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY, "parent_id": system_id}) diff --git a/test/unit/repositories/test_system.py b/test/unit/repositories/test_system.py index 8cd9a8b4..0fd1c117 100644 --- a/test/unit/repositories/test_system.py +++ b/test/unit/repositories/test_system.py @@ -52,6 +52,9 @@ def setup(self, test_helpers, database_mock): self.mock_session = MagicMock() + # Here we only wrap the utils so they keep their original functionality - this is done here + # to avoid having to mock the code generation function as the output will be passed to SystemOut + # with pydantic validation and so will error otherwise with patch("inventory_management_system_api.repositories.system.utils") as mock_utils: self.mock_utils = mock_utils yield