ec2_ami_info/tests: add unit-tests #1252
Conversation
ansibullbot
added
community_review
module
|
recheck |
| images = ec2_client.describe_images(aws_retry=True, **request_args) | ||
| except (ClientError, BotoCoreError) as err: | ||
| raise AmiInfoFailure(err, "error describing images") | ||
| return images |
There was a problem hiding this comment.
| images = ec2_client.describe_images(aws_retry=True, **request_args) | |
| except (ClientError, BotoCoreError) as err: | |
| raise AmiInfoFailure(err, "error describing images") | |
| return images | |
| return ec2_client.describe_images(aws_retry=True, **request_args) | |
| except (ClientError, BotoCoreError) as err: | |
| raise AmiInfoFailure(err, "error describing images") |
There was a problem hiding this comment.
keeping consistency with other methods, I prefer this way, but open to the suggested change as well.
images = ec2_client.describe_images(aws_retry=True, **request_args)
except (ClientError, BotoCoreError) as err:
raise AmiInfoFailure(err, "error describing images")
return images
plugins/modules/ec2_ami_info.py
Outdated
| for image in images: | ||
| try: | ||
| image['tags'] = boto3_tag_list_to_ansible_dict(image.get('tags', [])) | ||
| if module.params.get("describe_image_attributes"): | ||
| launch_permissions = ec2_client.describe_image_attribute(aws_retry=True, Attribute='launchPermission', | ||
| ImageId=image['image_id'])['LaunchPermissions'] | ||
| launch_permissions = get_image_attribute(ec2_client, image)['LaunchPermissions'] |
There was a problem hiding this comment.
| launch_permissions = get_image_attribute(ec2_client, image)['LaunchPermissions'] | |
| launch_permissions = get_image_attribute(ec2_client, image).get('LaunchPermissions') |
There was a problem hiding this comment.
In this case you need to default on [] to avoid a TypeError later:
launch_permissions = get_image_attribute(ec2_client, image).get('LaunchPermissions', [])
This being said, the use of .get() may also be a bit risky and hide a real problem, like e.g if the API call return something totally unexpected.
| module_name = "ansible_collections.amazon.aws.plugins.modules.ec2_ami_info" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("executable_users,filters,image_ids,owners,expected", [ |
There was a problem hiding this comment.
executable_users has only one value, this can be removed from the parameterized or add other values to increase coverage
There was a problem hiding this comment.
Added a assertion with executable_users.
|
|
||
| ec2_client.describe_images.call_count == 2 | ||
| ec2_client.describe_images.assert_called_with(aws_retry=True, **request_args) | ||
| assert len(get_images_result['Images']) == 1 |
There was a problem hiding this comment.
| assert len(get_images_result['Images']) == 1 | |
| assert get_images_result == ec2_client.describe_images.return_value |
| executable_users, filters, image_ids, owners) == expected | ||
|
|
||
|
|
||
| def test_get_images(): |
There was a problem hiding this comment.
you should add a test case for the failure
There was a problem hiding this comment.
| assert len(get_images_result['Images']) == 1 | ||
|
|
||
|
|
||
| def test_get_image_attribute(): |
There was a problem hiding this comment.
same here for the failure test case
|
|
||
| ec2_client.describe_image_attribute.call_count == 1 | ||
| ec2_client.describe_image_attribute.assert_called_with(aws_retry=True, Attribute='launchPermission', ImageId=image['image_id']) | ||
| assert len(get_image_attribute_result['LaunchPermissions']) == 2 |
There was a problem hiding this comment.
you should test as much as possible the full result
There was a problem hiding this comment.
This structure was passed to Mock and here we've got what Mock has returned. If something goes wrong, it will be within Mock which is not what we're testing here. I don't think this will help.
There was a problem hiding this comment.
this is not true, the mock is just inside the function we are testing, so if inside the testing function the result returned by the mock function is modified this test will not failed but it should
| @patch(module_name + '.get_image_attribute') | ||
| @patch(module_name + '.get_images') | ||
| def test_list_ec2_images(m_get_images, m_get_image_attribute): | ||
| ec2_client = MagicMock() |
There was a problem hiding this comment.
as this is repeated many times, you can create a fixture
|
regate |
SUMMARY
list_ec2_imagesto moveec2_client.describe_image_attributeandec2_client.describe_imagesto separate methods.COMPONENT NAME
ec2_ami_info