Add unit test for `coriolis.api.v1.*` modules #283

Cristi1324 · 2023-11-24T15:53:50Z

This PR add unit tests for Coriolis/api/v1/*.py

coriolis/tests/api/v1/test_diagnostics.py

Dany9966 · 2023-11-27T12:29:13Z

coriolis/tests/api/v1/test_endpoint_actions.py

+        body = mock.sentinel.body
+        is_valid = 'mock_status'
+        message = 'mock_message'
+        mock_validate_connection.return_value = [is_valid, message]


This should be a pair, not a list. Please assign (is_valid, message) to this return_value. Also, is_valid should be a bool value.

Dany9966 · 2023-11-27T12:33:49Z

coriolis/tests/api/v1/test_endpoint_actions.py

+    @mock.patch.object(api.API, 'validate_connection')
+    @mock.patch('coriolis.policies.endpoints.ENDPOINTS_POLICY_PREFIX',
+                'mock_prefix')
+    def test_validate_connection_exept_not_found(


Please fix typo: test_validate_connection_except_not_found

Dany9966 · 2023-11-27T12:35:12Z

coriolis/tests/api/v1/test_endpoint_actions.py

+    @mock.patch.object(api.API, 'validate_connection')
+    @mock.patch('coriolis.policies.endpoints.ENDPOINTS_POLICY_PREFIX',
+                'mock_prefix')
+    def test_validate_connection_exept_invalid_parameter_value(


Same as above, fix exept typo

Dany9966 · 2023-11-27T12:37:55Z

coriolis/tests/api/v1/test_endpoint_destination_minion_pool_options.py

+        self,
+        mock_get_endpoint_destination_minion_pool_options,
+        mock_destination_minion_pool_options_collection,
+        mock_get_diagnostics_policy_label,


NIT: please rename this to mock_decode_base64_param or similar

Dany9966 · 2023-11-27T13:43:40Z

coriolis/tests/api/v1/test_endpoints.py

+        mock_context = mock.Mock()
+        mock_req.environ = {'coriolis.context': mock_context}
+        body = mock.sentinel.body
+        mock__validate_create_body.return_value = [


This should be a tuple, not a list. Please replace

coriolis/tests/api/v1/test_endpoints.py

coriolis/tests/api/v1/test_migrations.py

Dany9966 · 2023-11-27T14:09:50Z

coriolis/tests/api/v1/test_migrations.py

+            result
+        )
+
+        if data['migration']['replica_id'] is not None:


Let's not repeat the original method's logic. It'd be helpful to add a "expected_api_method" and "validation_expected" keys alongside the migration data (not inside it). Therefore, you can mock the api method accordinlgy:

# `pop()` will remove the key: value from data, so you can pass it along to `create` expected_api_method = data.pop('expected_api_method') validation_expected = data.pop('validation_expected', False) # ... with mock.patch.object(api.API, expected_api_method) as mock_api_method: # ... result = self.migrations.create(mock_req, data) # .... # we don't care about the call args that much here if validation_expected: mock__validate_migration_input.assert_called_once() mock_api_method.assert_called_once() mock_single.assert_called_with(mock_api_method.return_value)

Dany9966 · 2023-11-27T14:18:55Z

coriolis/tests/api/v1/test_migrations.py

+        mock_req = mock.Mock()
+        mock_context = mock.Mock()
+        mock_req.environ = {'coriolis.context': mock_context}
+        mock_values = [(f'mock_value_{i}') for i in range(1, 15)]


Yeah.. let's not do this please. If you REALLY want to check these, use mock sentinels instead. IMO you can skip these long checks, the only thing that really matters is that the right API method is called if 'replica_id' is in migration or not. You can also check validate_user_scripts and normalize_user_scripts calls since you have to mock them.

Dany9966 · 2023-11-28T15:02:28Z

coriolis/tests/api/v1/test_diagnostics.py

+
+        result = self.diag_api.index(mock_req)
+
+        mock_get_diagnostics_policy_label.assert_called_with("get")


Please change all these assertions to assert_called_once_with, since these only get called once (as they should in this, and most cases)

Dany9966 · 2023-11-28T15:15:13Z

coriolis/tests/api/v1/test_endpoint_instances.py

+        (mock_get_endpoint_instance.
+            assert_called_once_with)(
+                mock_context, endpoint_id,
+                mock_decode_base64_param.return_value,


Please set side_effect to mock_decode_base_64_param, so you don't pass the same thing twice here:

mock_decode_base64_param.side_effect = [id, env] # ... mock_get_endpoint_instance.assert_called_once_with( mock_context, endpoint_id, id, env)

Dany9966 · 2023-11-28T15:31:06Z

coriolis/tests/api/v1/test_endpoint_source_options.py

+
+    def setUp(self):
+        super(
+            EndpointSourceOptionsControllerTestCase,


Please don't format code like this if the character limit allows otherwise. This fits in the limit, please adopt this style, format only where necessary:

def setUp(self): super(EndpointDestinationOptionsControllerTestCase, self).setUp() self.endpoint_api = endpoint.EndpointDestinationOptionsController()

Please apply this treatment wherever you see fit

Dany9966 · 2023-11-28T15:32:41Z

coriolis/tests/api/v1/test_endpoint_source_options.py

+                option_names=options)
+        (mock_source_options_collection.
+            assert_called_once_with)(
+                mock_get_endpoint_source_options.return_value)


This also looks like unnecessary formatting. Please replace with something like this:

mock_get_endpoint_destination_options.assert_called_once_with( mock_context, endpoint_id, env=env, option_names=options) mock_destination_options_collection.assert_called_once_with( mock_get_endpoint_destination_options.return_value)

Apply this also wherever you see fit.

Dany9966 · 2023-11-28T15:50:07Z

coriolis/tests/api/v1/test_endpoints.py

+        }
+        endpoint = testutils.get_wrapped_function(
+            self.endpoint_api._validate_create_body)(
+                self,


Please pass self.endpoint_api here, as that's an object of the class. self is a TestCase object here

Dany9966 · 2023-11-28T15:52:08Z

coriolis/tests/api/v1/test_endpoints.py

+        }
+        endpoint = testutils.get_wrapped_function(
+            self.endpoint_api._validate_update_body)(
+                self,


Same as above

Dany9966 · 2023-11-28T15:52:33Z

coriolis/tests/api/v1/test_endpoints.py

+        }
+        endpoint = testutils.get_wrapped_function(
+            self.endpoint_api._validate_update_body)(
+                self,


Same as above

Dany9966 · 2023-11-28T15:52:46Z

coriolis/tests/api/v1/test_endpoints.py

+            KeyError,
+            testutils.get_wrapped_function(
+                self.endpoint_api._validate_update_body),
+            self,


Same as above

Dany9966 · 2023-11-28T15:57:11Z

coriolis/tests/api/v1/test_migrations.py

+    @mock.patch('coriolis.api.v1.migrations.CONF')
+    def test_show(
+        self,
+        mock_CONF,


NIT: please rename this to mock_conf

Dany9966 · 2023-11-28T16:01:39Z

coriolis/tests/api/v1/test_migrations.py

+                mock__validate_migration_input.assert_called_with(
+                    mock_context, data)
+            mock_api_method.assert_called_once()
+            mock_single.assert_called_with(mock_api_method.return_value)


This is otherwise good so far, please add tests for the rest of the methods of coriolis.api.v1.migrations

Dany9966 · 2023-11-29T10:38:35Z

coriolis/tests/api/v1/test_endpoint_source_minion_pool_options.py

+from unittest import mock
+
+from coriolis.api.v1 import endpoint_source_minion_pool_options \
+    as endpoint


This import should be a single line please

Dany9966 · 2023-11-29T10:39:19Z

coriolis/tests/api/v1/test_endpoint_source_minion_pool_options.py

+        super(
+            EndpointSourceMinionPoolOptionsControllerTestCase,
+            self
+        ).setUp()


This also fits in a single line

Dany9966 · 2023-11-29T10:46:34Z

coriolis/tests/api/v1/test_endpoint_source_options.py

+        super(EndpointSourceOptionsControllerTestCase, self).setUp()
+        self.endpoint_api = endpoint.EndpointSourceOptionsController()
+
+    @mock.patch('coriolis.policies.endpoints.ENDPOINTS_POLICY_PREFIX',


Is there a reason you mocked this? This is actually a constant value, it doesn't need mocking. Please remove this mock, and also remove it from the other places you patched this constant.

Dany9966 · 2023-11-29T10:49:19Z

coriolis/tests/api/v1/test_endpoints.py

+from coriolis.tests import test_base
+from coriolis.tests import testutils
+
+from webob import exc


This import should be in the "third-party" group. Please move it above the coriolis imports:

from unittest import mock from webob import exc from coriolis.api.v1 import endpoints # ...

Also, please apply this to al the places you have this imported

Dany9966 · 2023-11-29T10:51:07Z

coriolis/tests/api/v1/test_endpoints.py

+        super(EndpointControllerTestCase, self).setUp()
+        self.endpoint_api = endpoints.EndpointController()
+
+    @mock.patch.object(endpoint_policies, 'get_endpoints_policy_label')


This method also just returns a string, nothing special that needs mocking. Please remove this mock and others like it.

Dany9966 · 2023-11-29T11:31:11Z

coriolis/tests/api/v1/test_migrations.py

+                        'origin_minion_pool_id': 'mock_origin_minion_pool_id',
+                        'instance_osmorphing_minion_pool_mappings': {
+                            'mock_instance_1', 'mock_instance_2'},
+                        'instances': {},


This is going to fail, yes, but not because you set replication_count to 12, but because you didn't set the same instances list as instance_osmorphing_minion_pool_mappings. Please set instances to [mock_instance_1', 'mock_instance_2']

Dany9966 · 2023-11-29T11:45:25Z

coriolis/tests/api/v1/test_migrations.py

+            include_task_info=False
+        )
+
+    @ddt.data(


Please use ddt.file_data here, so you can neatly put your test data inside a yml file

Dany9966 · 2023-11-29T11:48:02Z

coriolis/tests/api/v1/test_migrations.py

+                            'mock_destination_minion_pool_id',
+                        'instance_osmorphing_minion_pool_mappings': {
+                            'mock_instance_1', 'mock_instance_2'},
+                        'instances': {'mock_instance_1', 'mock_instance_2'},


To more accurately match the real-world data, please assign instance_osmorphing_minion_pool_mappings a map, like this:
{'mock_instance_1': 'mock_pool', 'mock_instance_2': 'mock_pool'} ;
and instances should be a list of instances:
['mock_instance_1', 'mock_instance_2']

Dany9966 · 2023-11-29T11:49:26Z

coriolis/tests/api/v1/test_migrations.py

+                data['migration']['instances'],
+            )
+
+    @ddt.data(


Same as above, please use a yml file using ddt.file_data

coriolis/tests/api/v1/test_migrations.py

Dany9966 · 2023-12-04T13:39:53Z

coriolis/tests/api/v1/test_diagnostics.py

+
+
+class DiagnosticsControllerTestCase(test_base.CoriolisBaseTestCase):
+    "Test suite for the Coriolis api v1."""


Please either remove this, or make sure this is a docstring, with a more suggestive string:

Suggested change

"Test suite for the Coriolis api v1."""

"""Test suite for the Coriolis Diagnostics v1 API"""

Dany9966 · 2023-12-04T13:43:41Z

coriolis/tests/api/v1/test_diagnostics.py

+
+    @mock.patch.object(api.API, 'get')
+    @mock.patch.object(diagnostic_view, 'collection')
+    @mock.patch.object(diagnostics, 'get_diagnostics_policy_label')


Don't mock this please. Instead I've asked you to mock context, so you can check whether mock_context.can is called with the expected policy tag, which in this case should be migration:diagnostics:get

coriolis/tests/api/v1/test_endpoint_actions.py

coriolis/tests/api/v1/test_endpoints.py

Dany9966 · 2023-12-04T13:59:48Z

coriolis/tests/api/v1/test_migration_actions.py

+        super(MigrationActionsControllerTestCase, self).setUp()
+        self.migration_actions = migration_actions.MigrationActionsController()
+
+    @mock.patch.object(migration_policies, 'get_migrations_policy_label')


Please stop mocking anything related to policy_label.

Dany9966 · 2023-12-04T14:04:28Z

coriolis/tests/api/v1/test_migrations.py

+            mock_validate_network_map,
+            mock_validate_target_environment,
+            mock_validate_storage_mappings,
+            migrations,


This shadows from coriolis.api.v1 import migrations from this file. Please rename it to somehing like config

Dany9966 · 2023-12-05T09:56:31Z

coriolis/tests/api/v1/test_diagnostics.py

+
+
+class DiagnosticsControllerTestCase(test_base.CoriolisBaseTestCase):
+    "Test suite for the Coriolis Diagnostics v1 API"""


This is still not a docstring. A Python docstring is defined between 3 double quotes, like this: """ doc string messsage """
Please pay more attention on the changes I propose in comments:

Suggested change

"Test suite for the Coriolis Diagnostics v1 API"""

"""Test suite for the Coriolis Diagnostics v1 API"""

coriolis/tests/api/v1/test_endpoint_actions.py

coriolis/tests/api/v1/test_endpoint_destination_minion_pool_options.py

Dany9966 · 2023-12-05T10:05:12Z

coriolis/tests/api/v1/test_endpoint_destination_options.py

+class EndpointDestinationOptionsControllerTestCase(
+    test_base.CoriolisBaseTestCase
+):
+    "Test suite for the Coriolis Endpoint Destination Options v1 API"""


Turn it into a docstring:

Suggested change

"Test suite for the Coriolis Endpoint Destination Options v1 API"""

"""Test suite for the Coriolis Endpoint Destination Options v1 API"""

coriolis/tests/api/v1/test_endpoint_destination_options.py

coriolis/tests/api/v1/test_endpoint_instances.py

coriolis/tests/api/v1/test_endpoint_networks.py

coriolis/tests/api/v1/test_endpoint_source_minion_pool_options.py

coriolis/tests/api/v1/test_endpoint_source_options.py

coriolis/tests/api/v1/test_endpoint_storage.py

Dany9966 · 2023-12-05T10:35:55Z

coriolis/tests/api/v1/data/minion_pool_exceptions.yml

@@ -0,0 +1,5 @@
+- config:


This config is practically empty. I think you could just pass the exception names directly:

- NotFound - InvalidParameterValue

coriolis/tests/api/v1/test_minion_pool_actions.py

Dany9966 · 2023-12-05T10:52:04Z

coriolis/tests/api/v1/test_minion_pool_actions.py

+    @mock.patch.object(minion_pool_view, 'single')
+    @mock.patch.object(api.API, 'deallocate_minion_pool')
+    @ddt.file_data('data/minion_pool_exceptions.yml')
+    @ddt.unpack


Same as above

coriolis/tests/api/v1/test_minion_pools.py

Dany9966 · 2023-12-06T12:26:06Z

coriolis/tests/api/v1/data/minion_pool_retention_strategy.yml

+- config:
+  exception_raised: True
+  pool_retention_strategy: null


You can lose the config, as it is empty, therefore irrelevant:

Suggested change

- config:

exception_raised: True

pool_retention_strategy: null

- exception_raised: True

pool_retention_strategy: null

Dany9966 · 2023-12-06T12:26:25Z

coriolis/tests/api/v1/test_minion_pools.py

+    @ddt.file_data('data/minion_pool_retention_strategy.yml')
+    def test__check_pool_retention_strategy(
+        self,
+        config,


Please remove this, as it is not used

Dany9966 · 2023-12-06T12:33:41Z

coriolis/tests/api/v1/data/minion_pool_numeric_values.yml

+- config:
+    minimum_minions: 1
+    maximum_minions: 1
+    minion_max_idle_time: 0


This in fact raises an exception, but you never call it to check. Please check explanation below

coriolis/tests/api/v1/test_minion_pools.py

…odule

Dany9966

LGTM so far. Please continue adding tests for the rest of V1 modules.

coriolis/api/v1/utils.py

coriolis/tests/api/v1/test_utils.py

Dany9966 · 2024-01-31T15:34:53Z

coriolis/tests/api/v1/test_utils.py

+            self.assertEqual(
+                expected_result,
+                result
+            )


Please add test for format_keyerror_message

Dany9966 requested changes Nov 27, 2023

View reviewed changes

Cristi1324 force-pushed the api-v1-unit-tests branch 3 times, most recently from 67d7b5a to 7b666f8 Compare November 28, 2023 13:08

Dany9966 requested changes Nov 28, 2023

View reviewed changes

Cristi1324 force-pushed the api-v1-unit-tests branch 3 times, most recently from a55dd79 to 23bf910 Compare November 28, 2023 18:35

Dany9966 requested changes Nov 29, 2023

View reviewed changes

Cristi1324 force-pushed the api-v1-unit-tests branch 2 times, most recently from 0d09662 to 47a8267 Compare December 4, 2023 12:56

Dany9966 requested changes Dec 4, 2023

View reviewed changes

Cristi1324 force-pushed the api-v1-unit-tests branch 2 times, most recently from 56afda7 to d8ebcef Compare December 5, 2023 10:14

Dany9966 requested changes Dec 5, 2023

View reviewed changes

Cristi1324 force-pushed the api-v1-unit-tests branch 3 times, most recently from fdd764c to dad61b4 Compare December 5, 2023 10:34

Dany9966 requested changes Dec 5, 2023

View reviewed changes

Cristi1324 force-pushed the api-v1-unit-tests branch 8 times, most recently from b08fc28 to 1139e1a Compare December 5, 2023 15:28

Dany9966 requested changes Dec 6, 2023

View reviewed changes

Cristi1324 force-pushed the api-v1-unit-tests branch from 1139e1a to 619b3ff Compare December 6, 2023 14:39

Cristi1324 changed the title ~~Add unit test for Coriolis api v1~~ Add unit test for coriolis/tests/api/v1/* Dec 6, 2023

Cristi1324 added 18 commits January 11, 2024 10:23

Add tests for coriolis.api.v1.endpoint_instances module

b202920

Add tests for coriolis.api.v1.endpoint_networks module

9d60621

Add tests for coriolis.api.v1.endpoint_source_minion_pool_options m…

2c2835a

…odule

Add tests for coriolis.api.v1.endpoint_source_options module

daf230c

Add tests for coriolis.api.v1.endpoint_storage module

5fb8aaf

Add tests for coriolis.api.v1.endpoints module

27561c6

Add tests for coriolis.api.v1.migration_actions module

edf91d8

Add tests for coriolis.api.v1.migrations module

3bafaab

Add tests for coriolis.api.v1.minion_pool_actions module

dde6622

Add tests for coriolis.api.v1.minion_pools module

68a8ee5

Add tests for coriolis.api.v1.provider_schemas module

23fe38c

Add tests for coriolis.api.v1.providers module

ed8c77e

Add tests for coriolis.api.v1.regions module

7189f00

Add tests for coriolis.api.v1.replica_actions module

1fad770

Add tests for coriolis.api.v1.replica_schedules module

cd9fad8

Add tests for replica_tasks_execution_actions module

b7decaa

Add tests for coriolis.api.v1.replica_tasks_executions module

4dd78a1

Add tests for coriolis.api.v1.replicas module

04193c3

Cristi1324 force-pushed the api-v1-unit-tests branch from aa5ca3e to 04193c3 Compare January 11, 2024 08:39

Dany9966 reviewed Jan 12, 2024

View reviewed changes

Cristi1324 force-pushed the api-v1-unit-tests branch from b7cba43 to 2629ce0 Compare January 15, 2024 12:12

Cristi1324 added 2 commits January 17, 2024 11:53

Add tests for coriolis.api.v1.router module

22bd874

Add tests for coriolis.api.v1.services module

62b2252

Cristi1324 force-pushed the api-v1-unit-tests branch from 2629ce0 to a2164d9 Compare January 17, 2024 12:29

Dany9966 requested changes Jan 31, 2024

View reviewed changes

Cristi1324 force-pushed the api-v1-unit-tests branch from b3788f2 to b4fcc68 Compare January 31, 2024 17:12

Cristi1324 added 2 commits January 31, 2024 23:11

Add tests for coriolis.api.v1.utils module

1233c00

Fix normalize_user_scripts in coriolis.api.v1.utils module

149774f

Cristi1324 force-pushed the api-v1-unit-tests branch from b4fcc68 to 149774f Compare January 31, 2024 21:11

Dany9966 merged commit 578a37b into cloudbase:master Feb 1, 2024
4 checks passed


		result = self.diag_api.index(mock_req)

		mock_get_diagnostics_policy_label.assert_called_with("get")



		class DiagnosticsControllerTestCase(test_base.CoriolisBaseTestCase):
		"Test suite for the Coriolis api v1."""

	"Test suite for the Coriolis api v1."""
	"""Test suite for the Coriolis Diagnostics v1 API"""



		class DiagnosticsControllerTestCase(test_base.CoriolisBaseTestCase):
		"Test suite for the Coriolis Diagnostics v1 API"""

	"Test suite for the Coriolis Endpoint Destination Options v1 API"""
	"""Test suite for the Coriolis Endpoint Destination Options v1 API"""

Add unit test for coriolis.api.v1.* modules #283

Add unit test for coriolis.api.v1.* modules #283

Conversation

Cristi1324 commented Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dany9966 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add unit test for `coriolis.api.v1.*` modules #283

Add unit test for `coriolis.api.v1.*` modules #283

Cristi1324 commented Nov 24, 2023 •

edited

Loading