From 85a17d1aee18b2b86f3eb4bc1b0100a4b2b16429 Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Fri, 6 Dec 2024 14:48:45 +0000 Subject: [PATCH] Update and add more tests #417 --- .../services/catalogue_item.py | 2 - test/e2e/test_setting.py | 7 ++ test/unit/repositories/test_setting.py | 72 ++++++++++++------- test/unit/services/conftest.py | 16 +++-- test/unit/services/test_catalogue_item.py | 51 +++++++++++-- 5 files changed, 114 insertions(+), 34 deletions(-) diff --git a/inventory_management_system_api/services/catalogue_item.py b/inventory_management_system_api/services/catalogue_item.py index fccb30df..bd2a42f3 100644 --- a/inventory_management_system_api/services/catalogue_item.py +++ b/inventory_management_system_api/services/catalogue_item.py @@ -97,8 +97,6 @@ def create(self, catalogue_item: CatalogueItemPostSchema) -> CatalogueItemOut: # Perform actual creation in a transaction as we need to ensure they cannot be added when in the process of # recalculating spares through the spares definition being set with start_session_transaction("creating catalogue item") as session: - # TODO: Update tests to include this new logic - # TODO: Handle if setting doesn't exist # Write lock the spares definition - if this fails it either means another catalogue item is being created # (unlikely) or that the spares definition has been set and is still recalculating self._setting_repository.write_lock(SparesDefinitionOut, session) diff --git a/test/e2e/test_setting.py b/test/e2e/test_setting.py index db6d5d4d..7b079df3 100644 --- a/test/e2e/test_setting.py +++ b/test/e2e/test_setting.py @@ -309,6 +309,13 @@ def check_catalogue_items_spares(self, expected_number_of_spares_list: list[Opti class TestSparesDefinitionDSL(SparesDefinitionDSL): """Tests for spares definition.""" + def test_create_catalogue_item_after_spares_definition_set(self): + """Test creating a catalogue item after the spares definition is set.""" + + self.put_spares_definition_and_post_prerequisites(SETTING_SPARES_DEFINITION_DATA_NEW) + self.post_items_and_prerequisites_with_usage_statuses([[]]) + self.check_catalogue_items_spares([0]) + def test_set_spares_definition_with_existing_items(self): """Test setting the spares definition when there are existing items.""" diff --git a/test/unit/repositories/test_setting.py b/test/unit/repositories/test_setting.py index 506682f1..60248b13 100644 --- a/test/unit/repositories/test_setting.py +++ b/test/unit/repositories/test_setting.py @@ -85,10 +85,10 @@ def mock_upsert( self._expected_setting_out = out_model_type(**new_setting_out_data) + RepositoryTestHelpers.mock_find_one(self.settings_collection, new_setting_out_data) + if out_model_type is SparesDefinitionOut: self.settings_collection.aggregate.return_value = [new_setting_out_data] - else: - RepositoryTestHelpers.mock_find_one(self.settings_collection, new_setting_out_data) def call_upsert(self) -> None: """Calls the `SettingRepo` `upsert` method with the appropriate data from a prior call to `mock_upsert`.""" @@ -138,33 +138,33 @@ class GetDSL(SettingRepoDSL): _obtained_out_model_type: Type[SettingOutBaseT] _expected_setting_out: SettingOutBaseT + _expect_return_before_aggregate: bool _obtained_setting: Optional[SettingOutBaseT] def mock_get( self, out_model_type: Type[SettingOutBaseT], + setting_database_data: Optional[dict], setting_out_data: Optional[dict], ) -> None: """ Mocks database methods appropriately to test the `get` repo method. :param out_model_type: The type of the setting's output model to be obtained. + :param setting_database_data: Either `None` or a dictionary containing the setting data as would be returned + by a `find_one` query. :param setting_out_data: Either `None` or a dictionary containing the setting data as would be required for the `Out` database model. """ self._expected_setting_out = out_model_type(**setting_out_data) if setting_out_data is not None else None + self._expect_return_before_aggregate = setting_database_data is None or ( + len(setting_database_data.keys()) == 2 and "_lock" in setting_database_data + ) + + RepositoryTestHelpers.mock_find_one(self.settings_collection, setting_database_data) if out_model_type is SparesDefinitionOut: self.settings_collection.aggregate.return_value = [setting_out_data] if setting_out_data is not None else [] - else: - RepositoryTestHelpers.mock_find_one( - self.settings_collection, - ( - self._expected_setting_out.model_dump(by_alias=True) - if self._expected_setting_out is not None - else None - ), - ) def call_get(self, out_model_type: Type[SettingOutBaseT]) -> None: """ @@ -179,14 +179,15 @@ def call_get(self, out_model_type: Type[SettingOutBaseT]) -> None: def check_get_success(self) -> None: """Checks that a prior call to `call_get` worked as expected.""" - if self._obtained_out_model_type is SparesDefinitionOut: - self.settings_collection.aggregate.assert_called_once_with( - SPARES_DEFINITION_GET_AGGREGATION_PIPELINE, session=self.mock_session - ) - else: - self.settings_collection.find_one.assert_called_once_with( - {"_id": self._obtained_out_model_type.SETTING_ID}, session=self.mock_session - ) + self.settings_collection.find_one.assert_called_once_with( + {"_id": self._obtained_out_model_type.SETTING_ID}, session=self.mock_session + ) + + if not self._expect_return_before_aggregate: + if self._obtained_out_model_type is SparesDefinitionOut: + self.settings_collection.aggregate.assert_called_once_with( + SPARES_DEFINITION_GET_AGGREGATION_PIPELINE, session=self.mock_session + ) assert self._obtained_setting == self._expected_setting_out @@ -197,28 +198,48 @@ class TestGet(GetDSL): def test_get(self): """Test getting a setting.""" - self.mock_get(ExampleSettingOut, {"_id": "example_setting"}) + self.mock_get(ExampleSettingOut, {"_id": "example_setting"}, {"_id": "example_setting"}) self.call_get(ExampleSettingOut) self.check_get_success() def test_get_non_existent(self): """Test getting a setting that is non-existent.""" - self.mock_get(ExampleSettingOut, None) + self.mock_get(ExampleSettingOut, None, None) + self.call_get(ExampleSettingOut) + self.check_get_success() + + def test_get_existent_but_not_assigned(self): + """Test getting a setting that is existent but only due to a write lock.""" + + self.mock_get(ExampleSettingOut, {"_id": "example_setting", "_lock": None}, None) self.call_get(ExampleSettingOut) self.check_get_success() def test_get_spares_definition(self): """Test getting the spares definition setting.""" - self.mock_get(SparesDefinitionOut, SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED) + self.mock_get( + SparesDefinitionOut, + SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED, + SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED, + ) self.call_get(SparesDefinitionOut) self.check_get_success() def test_get_non_existent_spares_definition(self): """Test getting the spares definition setting when it is non-existent.""" - self.mock_get(SparesDefinitionOut, None) + self.mock_get(SparesDefinitionOut, None, None) + self.call_get(SparesDefinitionOut) + self.check_get_success() + + def test_get_existent_spares_definition_but_not_assinged(self): + """Test getting the spares definition setting when it is existent but only due to a write lock.""" + + self.mock_get( + SparesDefinitionOut, {"_id": SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED["_id"], "_lock": None}, None + ) self.call_get(SparesDefinitionOut) self.check_get_success() @@ -242,7 +263,10 @@ def check_write_lock_success(self) -> None: """Checks that a prior call to `call_write_lock` worked as expected.""" self.settings_collection.update_one.assert_called_once_with( - {"_id": self._write_lock_out_model_type.SETTING_ID}, {"$set": {"_lock": None}}, session=self.mock_session + {"_id": self._write_lock_out_model_type.SETTING_ID}, + {"$set": {"_lock": None}}, + upsert=True, + session=self.mock_session, ) diff --git a/test/unit/services/conftest.py b/test/unit/services/conftest.py index c2cda357..4da4edb3 100644 --- a/test/unit/services/conftest.py +++ b/test/unit/services/conftest.py @@ -161,18 +161,26 @@ def fixture_catalogue_category_property_service( @pytest.fixture(name="catalogue_item_service") def fixture_catalogue_item_service( - catalogue_item_repository_mock: Mock, catalogue_category_repository_mock: Mock, manufacturer_repository_mock: Mock + catalogue_item_repository_mock: Mock, + catalogue_category_repository_mock: Mock, + manufacturer_repository_mock: Mock, + setting_repository_mock: Mock, ) -> CatalogueItemService: """ - Fixture to create a `CatalogueItemService` instance with a mocked `CatalogueItemRepo` and `CatalogueCategoryRepo` - dependencies. + Fixture to create a `CatalogueItemService` instance with a mocked `CatalogueItemRepo`, `CatalogueCategoryRepo`, + `ManufacturerRepo` and `SettingRepo` dependencies. :param catalogue_item_repository_mock: Mocked `CatalogueItemRepo` instance. :param catalogue_category_repository_mock: Mocked `CatalogueCategoryRepo` instance. + :param manufacturer_repository_mock: Mocked `ManufacturerRepo` instance. + :param setting_repository_mock: Mocked `SettingRepo` instance. :return: `CatalogueItemService` instance with the mocked dependencies. """ return CatalogueItemService( - catalogue_item_repository_mock, catalogue_category_repository_mock, manufacturer_repository_mock + catalogue_item_repository_mock, + catalogue_category_repository_mock, + manufacturer_repository_mock, + setting_repository_mock, ) diff --git a/test/unit/services/test_catalogue_item.py b/test/unit/services/test_catalogue_item.py index 4b6cfc23..c30cc1ec 100644 --- a/test/unit/services/test_catalogue_item.py +++ b/test/unit/services/test_catalogue_item.py @@ -5,6 +5,9 @@ # Expect some duplicate code inside tests as the tests for the different entities can be very similar # pylint: disable=too-many-lines # pylint: disable=duplicate-code +# pylint:disable=too-many-arguments +# pylint:disable=too-many-positional-arguments +# pylint:disable=too-many-instance-attributes from test.mock_data import ( BASE_CATALOGUE_CATEGORY_IN_DATA_WITH_PROPERTIES_MM, @@ -18,6 +21,7 @@ CATALOGUE_ITEM_DATA_WITH_ALL_PROPERTIES, MANUFACTURER_IN_DATA_A, PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42, + SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED, ) from test.unit.services.conftest import BaseCatalogueServiceDSL, ServiceTestHelpers from typing import Optional @@ -36,6 +40,7 @@ from inventory_management_system_api.models.catalogue_category import CatalogueCategoryIn, CatalogueCategoryOut from inventory_management_system_api.models.catalogue_item import CatalogueItemIn, CatalogueItemOut from inventory_management_system_api.models.manufacturer import ManufacturerIn, ManufacturerOut +from inventory_management_system_api.models.setting import SparesDefinitionOut from inventory_management_system_api.schemas.catalogue_item import ( CATALOGUE_ITEM_WITH_CHILD_NON_EDITABLE_FIELDS, CatalogueItemPatchSchema, @@ -53,6 +58,8 @@ class CatalogueItemServiceDSL(BaseCatalogueServiceDSL): mock_catalogue_category_repository: Mock mock_manufacturer_repository: Mock mock_unit_repository: Mock + mock_setting_repository: Mock + mock_start_session_transaction: Mock catalogue_item_service: CatalogueItemService # pylint:disable=too-many-arguments @@ -64,6 +71,7 @@ def setup( catalogue_category_repository_mock, manufacturer_repository_mock, unit_repository_mock, + setting_repository_mock, catalogue_item_service, # Ensures all created and modified times are mocked throughout # pylint: disable=unused-argument @@ -75,17 +83,23 @@ def setup( self.mock_catalogue_category_repository = catalogue_category_repository_mock self.mock_manufacturer_repository = manufacturer_repository_mock self.mock_unit_repository = unit_repository_mock + self.mock_setting_repository = setting_repository_mock self.catalogue_item_service = catalogue_item_service with patch("inventory_management_system_api.services.catalogue_item.utils", wraps=utils) as wrapped_utils: - self.wrapped_utils = wrapped_utils - yield + with patch( + "inventory_management_system_api.services.catalogue_item.start_session_transaction" + ) as mocked_start_session_transaction: + self.wrapped_utils = wrapped_utils + self.mock_start_session_transaction = mocked_start_session_transaction + yield class CreateDSL(CatalogueItemServiceDSL): """Base class for `create` tests.""" _catalogue_category_out: Optional[CatalogueCategoryOut] + _spares_definition_out: Optional[SparesDefinitionOut] _catalogue_item_post: CatalogueItemPostSchema _expected_catalogue_item_in: CatalogueItemIn _expected_catalogue_item_out: CatalogueItemOut @@ -98,6 +112,7 @@ def mock_create( catalogue_category_in_data: Optional[dict] = None, manufacturer_in_data: Optional[dict] = None, obsolete_replacement_catalogue_item_data: Optional[dict] = None, + spares_definition_out_data: Optional[dict] = None, ) -> None: """ Mocks repo methods appropriately to test the `create` service method. @@ -113,6 +128,8 @@ def mock_create( obsolete replacement as would be required for a `CatalogueItemPostSchema` but with any `unit_id`'s replaced by the `unit` value in its properties as the IDs will be added automatically. + :param spares_definition_out_data: Either `None` or a dictionary containing the spares definition data as would + be required for a `SparesDefinitionOut` database model. """ # Generate mandatory IDs to be inserted where needed @@ -181,6 +198,12 @@ def mock_create( self._catalogue_category_out.properties, property_post_schemas ) + # Mock the spares definition get + self._spares_definition_out = ( + SparesDefinitionOut(**spares_definition_out_data) if spares_definition_out_data is not None else None + ) + ServiceTestHelpers.mock_get(self.mock_setting_repository, self._spares_definition_out) + self._catalogue_item_post = CatalogueItemPostSchema( **{**catalogue_item_data, **ids_to_insert, "properties": property_post_schemas} ) @@ -191,7 +214,7 @@ def mock_create( **ids_to_insert, "properties": expected_properties_in, }, - number_of_spares=None, + number_of_spares=None if self._spares_definition_out is None else 0, ) self._expected_catalogue_item_out = CatalogueItemOut( **self._expected_catalogue_item_in.model_dump(), id=ObjectId() @@ -238,7 +261,15 @@ def check_create_success(self) -> None: self._catalogue_category_out.properties, self._catalogue_item_post.properties ) - self.mock_catalogue_item_repository.create.assert_called_once_with(self._expected_catalogue_item_in) + self.mock_start_session_transaction.assert_called_with("creating catalogue item") + expected_session = self.mock_start_session_transaction.return_value.__enter__.return_value + + # Ensure write locked the with spares definition + self.mock_setting_repository.write_lock.assert_called_once_with(SparesDefinitionOut, expected_session) + + self.mock_catalogue_item_repository.create.assert_called_once_with( + self._expected_catalogue_item_in, session=expected_session + ) assert self._created_catalogue_item == self._expected_catalogue_item_out @@ -280,6 +311,18 @@ def test_create_with_all_properties(self): self.call_create() self.check_create_success() + def test_create_with_spares_definition_defined(self): + """Test creating a catalogue item when there is a spares definition defined.""" + + self.mock_create( + CATALOGUE_ITEM_DATA_REQUIRED_VALUES_ONLY, + catalogue_category_in_data=CATALOGUE_CATEGORY_IN_DATA_LEAF_NO_PARENT_NO_PROPERTIES, + manufacturer_in_data=MANUFACTURER_IN_DATA_A, + spares_definition_out_data=SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED, + ) + self.call_create() + self.check_create_success() + def test_create_with_non_existent_catalogue_category_id(self): """Test creating a catalogue item with a non-existent catalogue category ID."""