From 733934e8033b55cb454e88420bae1ac9d4e84faf Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Fri, 9 Aug 2024 14:50:56 +0000 Subject: [PATCH] Fix reamining tests, linting, TODOs and clean up #347 --- test/e2e/mock_schemas.py | 62 +----------- test/e2e/test_catalogue_category.py | 9 +- test/e2e/test_catalogue_item.py | 13 ++- test/e2e/test_item.py | 95 ++++++++----------- test/e2e/test_system.py | 1 - test/e2e/test_usage_status.py | 18 ++-- .../repositories/test_catalogue_category.py | 2 +- test/unit/services/test_catalogue_category.py | 2 +- test/unit/services/test_catalogue_item.py | 2 +- test/unit/services/test_item.py | 2 +- 10 files changed, 72 insertions(+), 134 deletions(-) diff --git a/test/e2e/mock_schemas.py b/test/e2e/mock_schemas.py index b33eebb5..13fb176f 100644 --- a/test/e2e/mock_schemas.py +++ b/test/e2e/mock_schemas.py @@ -4,67 +4,13 @@ # Leaf with allowed values in properties -from unittest.mock import ANY - -CREATED_MODIFIED_VALUES_EXPECTED = {"created_time": ANY, "modified_time": ANY} -# pylint: disable=duplicate-code -CATALOGUE_CATEGORY_POST_ALLOWED_VALUES = { - "name": "Category Allowed Values", - "is_leaf": True, - "properties": [ - { - "name": "Property A", - "type": "number", - "unit": "mm", - "mandatory": False, - "allowed_values": {"type": "list", "values": [2, 4, 6]}, - }, - { - "name": "Property B", - "type": "string", - "unit": None, - "mandatory": True, - "allowed_values": {"type": "list", "values": ["red", "green"]}, - }, - ], -} - -# To be posted on CATALOGUE_CATEGORY_POST_ALLOWED_VALUES -CATALOGUE_ITEM_POST_ALLOWED_VALUES = { - "name": "Catalogue Item D", - "description": "This is Catalogue Item D", - "cost_gbp": 300.00, - "cost_to_rework_gbp": 120.99, - "days_to_replace": 1.5, - "days_to_rework": 3.0, - "drawing_number": "789xyz", - "is_obsolete": False, - "properties": [{"name": "Property A", "value": 4}, {"name": "Property B", "value": "red"}], -} +# pylint: disable=fixme +# TODO: Remove this file - replace by mock_data.py -ITEM_POST_ALLOWED_VALUES = { - "is_defective": False, - "warranty_end_date": "2015-11-15T23:59:59Z", - "serial_number": "xyz123", - "delivered_date": "2012-12-05T12:00:00Z", - "notes": "Test notes", - "properties": [{"name": "Property A", "value": 6}, {"name": "Property B", "value": "green"}], -} +from unittest.mock import ANY -ITEM_POST_ALLOWED_VALUES_EXPECTED = { - **ITEM_POST_ALLOWED_VALUES, - **CREATED_MODIFIED_VALUES_EXPECTED, - "id": ANY, - "purchase_order_number": None, - "usage_status": "New", - "usage_status_id": ANY, - "asset_number": None, - "properties": [ - {"name": "Property A", "unit": "mm", "value": 6}, - {"name": "Property B", "value": "green", "unit": None}, - ], -} +CREATED_MODIFIED_VALUES_EXPECTED = {"created_time": ANY, "modified_time": ANY} SYSTEM_POST_A = { "name": "System A", diff --git a/test/e2e/test_catalogue_category.py b/test/e2e/test_catalogue_category.py index 52968b3b..d2fbd432 100644 --- a/test/e2e/test_catalogue_category.py +++ b/test/e2e/test_catalogue_category.py @@ -1,9 +1,9 @@ -# pylint: disable=too-many-lines """ End-to-End tests for the catalogue category router. """ # 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-public-methods @@ -24,7 +24,9 @@ CATALOGUE_CATEGORY_PROPERTY_DATA_BOOLEAN_MANDATORY, CATALOGUE_CATEGORY_PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT, CATALOGUE_ITEM_DATA_REQUIRED_VALUES_ONLY, - MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY, UNIT_POST_DATA_MM) + MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY, + UNIT_POST_DATA_MM, +) from typing import Optional import pytest @@ -32,8 +34,7 @@ from fastapi.testclient import TestClient from httpx import Response -from inventory_management_system_api.core.consts import \ - BREADCRUMBS_TRAIL_MAX_LENGTH +from inventory_management_system_api.core.consts import BREADCRUMBS_TRAIL_MAX_LENGTH class CreateDSL: diff --git a/test/e2e/test_catalogue_item.py b/test/e2e/test_catalogue_item.py index a1db339b..ba22b187 100644 --- a/test/e2e/test_catalogue_item.py +++ b/test/e2e/test_catalogue_item.py @@ -1,16 +1,15 @@ -# pylint: disable=too-many-lines """ End-to-End tests for the catalogue item router. """ # 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-public-methods from test.e2e.conftest import E2ETestHelpers from test.e2e.mock_schemas import SYSTEM_POST_A, USAGE_STATUS_POST_A -from test.e2e.test_catalogue_category import \ - CreateDSL as CatalogueCategoryCreateDSL +from test.e2e.test_catalogue_category import CreateDSL as CatalogueCategoryCreateDSL from test.e2e.test_manufacturer import CreateDSL as ManufacturerCreateDSL from test.mock_data import ( BASE_CATALOGUE_CATEGORY_DATA_WITH_PROPERTIES_MM, @@ -44,7 +43,8 @@ PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_1, PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42, PROPERTY_GET_DATA_STRING_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_VALUE1, - UNIT_POST_DATA_MM) + UNIT_POST_DATA_MM, +) from typing import Any, Optional import pytest @@ -269,9 +269,8 @@ def check_post_catalogue_item_success(self, expected_catalogue_item_get_data: di returned. :param expected_catalogue_item_get_data: Dictionary containing the expected catalogue item data returned as - would be required for a `CatalogueItemSchema`. Does not need mandatory - IDs (e.g. `manufacturer_id`) as they will be to check they are as - expected. + would be required for a `CatalogueItemSchema`. Does not need mandatory IDs (e.g. + `manufacturer_id`) as they will be added automatically to check they are as expected. """ assert self._post_response_catalogue_item.status_code == 201 diff --git a/test/e2e/test_item.py b/test/e2e/test_item.py index a2ee3f82..06cc5fc3 100644 --- a/test/e2e/test_item.py +++ b/test/e2e/test_item.py @@ -1,30 +1,17 @@ -# pylint: disable=too-many-lines """ -End-to-End tests for the catalogue item router. +End-to-End tests for the item router. """ # 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-public-methods # pylint: disable=too-many-ancestors -from test.conftest import add_ids_to_properties -from test.e2e.conftest import E2ETestHelpers, replace_unit_values_with_ids_in_properties -from test.e2e.mock_schemas import ( - CATALOGUE_CATEGORY_POST_ALLOWED_VALUES, - CATALOGUE_ITEM_POST_ALLOWED_VALUES, - CREATED_MODIFIED_VALUES_EXPECTED, - ITEM_POST_ALLOWED_VALUES, - ITEM_POST_ALLOWED_VALUES_EXPECTED, - SYSTEM_POST_A, - SYSTEM_POST_B, - USAGE_STATUS_POST_A, -) +from test.e2e.conftest import E2ETestHelpers from test.e2e.test_catalogue_item import CreateDSL as CatalogueItemCreateDSL from test.e2e.test_system import CreateDSL as SystemCreateDSL -from test.e2e.test_unit import UNIT_POST_A, UNIT_POST_B from test.mock_data import ( - CATALOGUE_CATEGORY_POST_DATA_LEAF_NO_PARENT_NO_PROPERTIES, CATALOGUE_CATEGORY_PROPERTY_DATA_BOOLEAN_MANDATORY, CATALOGUE_CATEGORY_PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST, CATALOGUE_CATEGORY_PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT, @@ -54,7 +41,6 @@ USAGE_STATUS_DATA_NEW, ) from typing import Any, Optional -from unittest.mock import ANY import pytest from bson import ObjectId @@ -78,13 +64,39 @@ def setup_item_create_dsl(self): self.system_id = None self.usage_status_value_id_dict = {} + def merge_properties_in_expected_item_get_data(self, expected_item_get_data: dict) -> dict: + """ + Merges any existing properties in an already posted catalogue item into expected get data returned for an item. + + Assumes the last catalogue item posted was the one the properties should be merged with. + + :param expected_item_get_data: Dictionary containing the expected item data returned as would be required for a + `ItemSchema`. Does not need mandatory IDs (e.g. `system_id`). + :return: Dictionary containing the same `expected_item_get_data` but with any missing properties found inside + the last posted catalogue item merged in. + """ + + if "properties" in expected_item_get_data: + catalogue_item_data = self._post_response_catalogue_item.json() + expected_merged_properties = [] + supplied_properties_dict = { + supplied_property["name"]: supplied_property + for supplied_property in expected_item_get_data["properties"] + } + for prop in catalogue_item_data["properties"]: + supplied_property = supplied_properties_dict.get(prop["name"]) + expected_merged_properties.append(supplied_property if supplied_property else prop) + + return {**expected_item_get_data, "properties": expected_merged_properties} + return expected_item_get_data + def add_ids_to_expected_item_get_data(self, expected_item_get_data) -> dict: """ Adds required IDs to some expected item get data based on what has already been posted. - :param expected_item_get_data: Dictionary containing the expected catalogue item data returned as would be - required for an `ItemSchema`. Does not need mandatory IDs (e.g. `system_id`) as - they will be added here. + :param expected_item_get_data: Dictionary containing the expected item data returned as would be required for an + `ItemSchema`. Does not need mandatory IDs (e.g. `system_id`) as they will be + added here. """ # Where there are properties add the property ID, unit ID and unit value expected_item_get_data = E2ETestHelpers.add_property_ids_to_properties( @@ -154,10 +166,10 @@ def post_item(self, item_data: dict) -> Optional[str]: """ Posts an item with the given data and returns the ID of the created item if successful. - :param item_data: Dictionary containing the basic catalogue item data as would be required for a - `ItemPostSchema` but with mandatory IDs missing and any `id`'s replaced by the `name` value - in its properties as the IDs will be added automatically. - :return: ID of the created catalogue item (or `None` if not successful). + :param item_data: Dictionary containing the basic item data as would be required for a `ItemPostSchema` but with + mandatory IDs missing and any `id`'s replaced by the `name` value in its properties as the IDs + will be added automatically. + :return: ID of the created item (or `None` if not successful). """ # Replace any unit values with unit IDs @@ -289,24 +301,12 @@ def check_post_item_success(self, expected_item_get_data: dict) -> None: Also merges in any properties that were defined in the catalogue item but are not given in the expected data. :param expected_item_get_data: Dictionary containing the expected item data returned as would be required for a - `ItemSchema`. Does not need mandatory IDs (e.g. `system_id`) as they will be to - check they are as expected. + `ItemSchema`. Does not need mandatory IDs (e.g. `system_id`) as they will be + added automatically to check they are as expected. """ # Where properties are involved in the catalogue item, need to merge them - catalogue_item_data = self._post_response_catalogue_item.json() - expected_item_get_data = expected_item_get_data.copy() - if "properties" in expected_item_get_data: - expected_merged_properties = [] - supplied_properties_dict = { - supplied_property["name"]: supplied_property - for supplied_property in expected_item_get_data["properties"] - } - for prop in catalogue_item_data["properties"]: - supplied_property = supplied_properties_dict.get(prop["name"]) - expected_merged_properties.append(supplied_property if supplied_property else prop) - - expected_item_get_data["properties"] = expected_merged_properties + expected_item_get_data = self.merge_properties_in_expected_item_get_data(expected_item_get_data) assert self._post_response_item.status_code == 201 assert self._post_response_item.json() == self.add_ids_to_expected_item_get_data(expected_item_get_data) @@ -608,7 +608,7 @@ def get_item(self, item_id: str) -> None: """ Gets an item with the given ID. - :param item_id: ID of the catalogue item to be obtained. + :param item_id: ID of the item to be obtained. """ self._get_response_item = self.test_client.get(f"/v1/items/{item_id}") @@ -838,21 +838,8 @@ def check_patch_item_response_success(self, expected_item_get_data: dict) -> Non they are as expected. """ - # TODO: Move this to common function - its done in the CreateDSL too # Where properties are involved in the catalogue item, need to merge them - catalogue_item_data = self._post_response_catalogue_item.json() - expected_item_get_data = expected_item_get_data.copy() - if "properties" in expected_item_get_data: - expected_merged_properties = [] - supplied_properties_dict = { - supplied_property["name"]: supplied_property - for supplied_property in expected_item_get_data["properties"] - } - for prop in catalogue_item_data["properties"]: - supplied_property = supplied_properties_dict.get(prop["name"]) - expected_merged_properties.append(supplied_property if supplied_property else prop) - - expected_item_get_data["properties"] = expected_merged_properties + expected_item_get_data = self.merge_properties_in_expected_item_get_data(expected_item_get_data) assert self._patch_response_item.status_code == 200 assert self._patch_response_item.json() == self.add_ids_to_expected_item_get_data(expected_item_get_data) diff --git a/test/e2e/test_system.py b/test/e2e/test_system.py index 3f2799c6..122d7f18 100644 --- a/test/e2e/test_system.py +++ b/test/e2e/test_system.py @@ -5,7 +5,6 @@ # Expect some duplicate code inside tests as the tests for the different entities can be very similar # pylint: disable=duplicate-code -from test.conftest import add_ids_to_properties from test.e2e.conftest import E2ETestHelpers from test.e2e.mock_schemas import USAGE_STATUS_POST_B from test.mock_data import ( diff --git a/test/e2e/test_usage_status.py b/test/e2e/test_usage_status.py index bbe78023..0476649a 100644 --- a/test/e2e/test_usage_status.py +++ b/test/e2e/test_usage_status.py @@ -14,7 +14,12 @@ USAGE_STATUS_POST_D, USAGE_STATUS_POST_D_EXPECTED, ) -from test.e2e.test_item import CATALOGUE_CATEGORY_POST_A, CATALOGUE_ITEM_POST_A, ITEM_POST, MANUFACTURER_POST +from test.mock_data import ( + CATALOGUE_CATEGORY_POST_DATA_LEAF_NO_PARENT_NO_PROPERTIES, + CATALOGUE_ITEM_DATA_REQUIRED_VALUES_ONLY, + ITEM_DATA_REQUIRED_VALUES_ONLY, + MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY, +) from bson import ObjectId @@ -129,18 +134,20 @@ def test_delete_with_a_non_existent_id(test_client): def test_delete_usage_status_that_is_a_part_of_item(test_client): """Test trying to delete a usage status that is a part of an Item""" # pylint: disable=duplicate-code - response = test_client.post("/v1/catalogue-categories", json=CATALOGUE_CATEGORY_POST_A) + response = test_client.post( + "/v1/catalogue-categories", json=CATALOGUE_CATEGORY_POST_DATA_LEAF_NO_PARENT_NO_PROPERTIES + ) catalogue_category = response.json() response = test_client.post("/v1/systems", json=SYSTEM_POST_A) system_id = response.json()["id"] - response = test_client.post("/v1/manufacturers", json=MANUFACTURER_POST) + response = test_client.post("/v1/manufacturers", json=MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY) manufacturer_id = response.json()["id"] catalogue_item_post = { - **CATALOGUE_ITEM_POST_A, + **CATALOGUE_ITEM_DATA_REQUIRED_VALUES_ONLY, "catalogue_category_id": catalogue_category["id"], "manufacturer_id": manufacturer_id, "properties": add_ids_to_properties( @@ -160,11 +167,10 @@ def test_delete_usage_status_that_is_a_part_of_item(test_client): usage_status_id = response.json()["id"] item_post = { - **ITEM_POST, + **ITEM_DATA_REQUIRED_VALUES_ONLY, "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"]), } # pylint: enable=duplicate-code response = test_client.post("/v1/items", json=item_post) diff --git a/test/unit/repositories/test_catalogue_category.py b/test/unit/repositories/test_catalogue_category.py index 8cbb4e9a..9ea4aecd 100644 --- a/test/unit/repositories/test_catalogue_category.py +++ b/test/unit/repositories/test_catalogue_category.py @@ -1,10 +1,10 @@ -# pylint: disable=too-many-lines """ Unit tests for the `CatalogueCategoryRepo` repository. """ # Expect some duplicate code inside tests as the tests for the different entities can be very similar # pylint: disable=duplicate-code +# pylint: disable=too-many-lines from test.mock_data import ( CATALOGUE_CATEGORY_IN_DATA_LEAF_NO_PARENT_NO_PROPERTIES, diff --git a/test/unit/services/test_catalogue_category.py b/test/unit/services/test_catalogue_category.py index 689ef7d7..827ba7eb 100644 --- a/test/unit/services/test_catalogue_category.py +++ b/test/unit/services/test_catalogue_category.py @@ -1,9 +1,9 @@ -# pylint: disable=too-many-lines """ Unit tests for the `CatalogueCategoryService` service. """ # 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 from test.mock_data import ( diff --git a/test/unit/services/test_catalogue_item.py b/test/unit/services/test_catalogue_item.py index b4530ca3..ebdb156c 100644 --- a/test/unit/services/test_catalogue_item.py +++ b/test/unit/services/test_catalogue_item.py @@ -1,9 +1,9 @@ -# pylint: disable=too-many-lines """ Unit tests for the `CatalogueCategoryService` service. """ # 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 from test.mock_data import ( diff --git a/test/unit/services/test_item.py b/test/unit/services/test_item.py index c04fb9fa..b88d9602 100644 --- a/test/unit/services/test_item.py +++ b/test/unit/services/test_item.py @@ -1,9 +1,9 @@ -# pylint: disable=too-many-lines """ Unit tests for the `ItemService` service. """ # 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 from test.mock_data import (