From 63ace82f625bff377a241434bc42a670fe7b9a9b Mon Sep 17 00:00:00 2001 From: Rajesh Duraisamy <118321621+rajdnp@users.noreply.github.com> Date: Thu, 11 May 2023 10:55:58 -0700 Subject: [PATCH] Improving logging for Ctv1 (#996) * Improving logging for Ctv1 --- src/rpdk/core/contract/resource_client.py | 47 ++++++++++++++++--- .../suite/resource/handler_commons.py | 27 +++++++++++ .../contract/suite/resource/handler_create.py | 5 +- .../contract/suite/resource/handler_update.py | 29 ++++++++++-- tests/contract/test_resource_client.py | 40 ++++++++++++++++ 5 files changed, 136 insertions(+), 12 deletions(-) diff --git a/src/rpdk/core/contract/resource_client.py b/src/rpdk/core/contract/resource_client.py index 0e0fd37f..b08196d4 100644 --- a/src/rpdk/core/contract/resource_client.py +++ b/src/rpdk/core/contract/resource_client.py @@ -281,12 +281,20 @@ def has_only_writable_identifiers(self): ) def assert_write_only_property_does_not_exist(self, resource_model): + + error_list = [] if self.write_only_paths: - assert not any( - self.key_error_safe_traverse(resource_model, write_only_property) - for write_only_property in self.write_only_paths - ), "The model MUST NOT return properties defined as \ - writeOnlyProperties in the resource schema" + for write_only_property in self.write_only_paths: + val = self.key_error_safe_traverse(resource_model, write_only_property) + if val: + error_list.append(write_only_property[1]) + assertion_error_message = ( + "The model MUST NOT return properties defined as " + "writeOnlyProperties in the resource schema " + "\n Write only properties in resource model : %s \n Output Resource Model : %s \n" + % (error_list, resource_model) + ) + assert not any(error_list), assertion_error_message def get_metadata(self): try: @@ -450,7 +458,8 @@ def compare_model(self, inputs, outputs, path=()): "All properties specified in the request MUST " "be present in the model returned, and they MUST" " match exactly, with the exception of properties" - " defined as writeOnlyProperties in the resource schema" + " defined as writeOnlyProperties in the resource schema \n Request Model : %s \n Returned Model : %s \n" + % (inputs, outputs) ) try: if isinstance(inputs, dict): @@ -476,6 +485,16 @@ def compare_model(self, inputs, outputs, path=()): new_path, ) else: + if inputs[key] != outputs[key]: + assertion_error_message = ( + "%s Value for property %s in Request Model(%s) and Response Model(%s) does not match" + % ( + assertion_error_message, + key, + inputs[key], + outputs[key], + ) + ) assert inputs[key] == outputs[key], assertion_error_message else: assert inputs == outputs, assertion_error_message @@ -628,6 +647,22 @@ def is_primary_identifier_equal( match the primaryIdentifier passed into the request" ) from e + @staticmethod + def get_primary_identifier(primary_identifier_path, model): + try: + pid_list = [] + for primary_identifier in primary_identifier_path: + data = traverse(model, fragment_list(primary_identifier, "properties"))[ + 0 + ] + pid_list.append(data) + return pid_list + except KeyError as e: + raise AssertionError( + "The primaryIdentifier returned in every progress event must\ + match the primaryIdentifier passed into the request \n" + ) from e + def _make_payload( self, action, diff --git a/src/rpdk/core/contract/suite/resource/handler_commons.py b/src/rpdk/core/contract/suite/resource/handler_commons.py index c6b21b76..eedfe70e 100644 --- a/src/rpdk/core/contract/suite/resource/handler_commons.py +++ b/src/rpdk/core/contract/suite/resource/handler_commons.py @@ -102,6 +102,33 @@ def test_model_in_list(resource_client, current_resource_model): ) +def error_test_model_in_list(resource_client, current_resource_model, message): + resource_models = get_resource_model_list(resource_client, current_resource_model) + assertion_error_message = message + for resource_model in resource_models: + resource_model_primary_identifier = resource_client.get_primary_identifier( + resource_client.primary_identifier_paths, resource_model + ) + current_model_primary_identifier = resource_client.get_primary_identifier( + resource_client.primary_identifier_paths, current_resource_model + ) + if resource_model_primary_identifier != current_model_primary_identifier: + assertion_error_message = ( + "%s \n Resource Model primary identifier %s does not match with " + "Current Resource Model primary identifier %s \n Resource Model : %s" + " \n Currrent Model : %s " + % ( + message, + resource_model_primary_identifier[0], + current_model_primary_identifier[0], + resource_model, + current_resource_model, + ) + ) + return assertion_error_message + return assertion_error_message + + @response_contains_primary_identifier @response_contains_unchanged_primary_identifier @response_contains_resource_model_equal_updated_model diff --git a/src/rpdk/core/contract/suite/resource/handler_create.py b/src/rpdk/core/contract/suite/resource/handler_create.py index 979b9c15..a02213ac 100644 --- a/src/rpdk/core/contract/suite/resource/handler_create.py +++ b/src/rpdk/core/contract/suite/resource/handler_create.py @@ -13,6 +13,7 @@ skip_not_writable_identifier, ) from rpdk.core.contract.suite.resource.handler_commons import ( + error_test_model_in_list, test_create_failure_if_repeat_writeable_id, test_create_success, test_delete_success, @@ -75,7 +76,9 @@ def contract_create_read(created_resource, resource_client): @pytest.mark.read def contract_create_list(created_resource, resource_client): _input_model, created_model, _request = created_resource - assert test_model_in_list(resource_client, created_model) + assert test_model_in_list(resource_client, created_model), error_test_model_in_list( + resource_client, created_model, "" + ) test_read_success(resource_client, created_model) diff --git a/src/rpdk/core/contract/suite/resource/handler_update.py b/src/rpdk/core/contract/suite/resource/handler_update.py index 0a5df956..654b57f5 100644 --- a/src/rpdk/core/contract/suite/resource/handler_update.py +++ b/src/rpdk/core/contract/suite/resource/handler_update.py @@ -11,6 +11,7 @@ skip_not_tag_updatable, ) from rpdk.core.contract.suite.resource.handler_commons import ( + error_test_model_in_list, test_input_equals_output, test_model_in_list, test_read_success, @@ -55,10 +56,26 @@ def contract_update_read(updated_resource, resource_client): updated_model, updated_input_model, ) = updated_resource + create_primary_identifiers = resource_client.get_primary_identifier( + resource_client.primary_identifier_paths, _created_model + ) + update_primary_identifiers = resource_client.get_primary_identifier( + resource_client.primary_identifier_paths, updated_model + ) + assertion_error_message = ( + "The primaryIdentifier returned must match " + "the primaryIdentifier passed into the request " + "Create Model primary identifier %s does not match with Update Model primary identifier %s \n Create Model : %s \n Update Model : %s " + % ( + create_primary_identifiers, + update_primary_identifiers, + _created_model, + updated_model, + ) + ) assert resource_client.is_primary_identifier_equal( resource_client.primary_identifier_paths, _created_model, updated_model - ), "The primaryIdentifier returned must match\ - the primaryIdentifier passed into the request" + ), assertion_error_message read_response = test_read_success(resource_client, updated_model) test_input_equals_output( resource_client, updated_input_model, read_response["resourceModel"] @@ -81,9 +98,11 @@ def contract_update_list(updated_resource, resource_client): resource_client.primary_identifier_paths, _created_model, updated_model ), "The primaryIdentifier returned must match\ the primaryIdentifier passed into the request" - assert test_model_in_list( - resource_client, updated_model - ), "A list handler MUST always return an updated model" + assert test_model_in_list(resource_client, updated_model), error_test_model_in_list( + resource_client, + updated_model, + "A list handler MUST always return an updated model", + ) @pytest.mark.update diff --git a/tests/contract/test_resource_client.py b/tests/contract/test_resource_client.py index 8bb7958d..6bf76fdc 100644 --- a/tests/contract/test_resource_client.py +++ b/tests/contract/test_resource_client.py @@ -7,6 +7,7 @@ import pytest +import rpdk.core.contract.resource_client as rclient from rpdk.core.boto_helpers import LOWER_CAMEL_CRED_KEYS from rpdk.core.contract.interface import Action, HandlerErrorCode, OperationStatus from rpdk.core.contract.resource_client import ( @@ -17,6 +18,7 @@ prune_properties_if_not_exist_in_path, prune_properties_which_dont_exist_in_path, ) +from rpdk.core.contract.suite.resource.handler_commons import error_test_model_in_list from rpdk.core.exceptions import InvalidProjectError from rpdk.core.test import ( DEFAULT_ENDPOINT, @@ -149,6 +151,8 @@ EMPTY_SCHEMA = {"handlers": {"create": [], "delete": [], "read": []}} +RESOURCE_MODEL_LIST = [{"Id": "abc123", "b": "2"}] + @pytest.fixture def resource_client(): @@ -390,6 +394,42 @@ def resource_client_inputs_property_transform(): return client +def test_error_test_model_in_list(resource_client): + patch_resource_model_list = patch( + "rpdk.core.contract.suite.resource.handler_commons.get_resource_model_list", + autospec=True, + return_value=RESOURCE_MODEL_LIST, + ) + resource_client.primary_identifier_paths = {("properties", "Id")} + current_resource_model = {"Id": "xyz456", "b": "2"} + with patch_resource_model_list: + assertion_error_message = error_test_model_in_list( + resource_client, current_resource_model, "" + ) + assert ( + "abc123 does not match with Current Resource Model primary identifier xyz456" + in assertion_error_message + ) + + +def test_get_primary_identifier_success(): + primary_identifier_path = {("properties", "a")} + model = {"a": 1, "b": 3, "c": 4} + plist = rclient.ResourceClient.get_primary_identifier( + primary_identifier_path, model + ) + assert plist[0] == 1 + + +def test_get_primary_identifier_fail(): + primary_identifier_path = {("properties", "a")} + model = {"b": 3, "c": 4} + try: + rclient.ResourceClient.get_primary_identifier(primary_identifier_path, model) + except AssertionError: + logging.debug("This test expects Assertion Exception to be thrown") + + def test_prune_properties(): document = { "foo": "bar",