From f92f77f683656f93fbb71057089c20326e729263 Mon Sep 17 00:00:00 2001 From: Zhenyu Zheng Date: Fri, 17 May 2019 15:01:37 +0800 Subject: [PATCH] Fix typo in ceilometer datasource Change I25b4cb0e1b85379ff0c4da9d0c1474380d75ce3a in Queens refactored the statistic_aggregation method and renamed the "aggregate" kwarg to "aggregation", presumably to match the signature of the GnocchiHelper statistic_aggregation method (the commit message does not give details) so a base method could be added to the parent class for all datasource helpers. As a result, the CeilometerHelper calls to its statistic_aggregation started passing the new "granularity" kwarg but failed to match the rename to the "aggregation" kwarg, which breaks the CeilometerHelper. This was missed by the unit tests because the tests were just asserting the erroneous call that the runtime code made. This change fixes the kwarg typo and makes the tests more robust by using the mock spec kwarg to define a spec for the statistic_aggregation mock so that it must be called with the correct parameters defined in the method signature. The test is refactored to reduce duplicate mocking. The same test hardening can and should be done in the gnocchi and monasca helper tests but that should be done in a separate change. Co-Authored-By: Matt Riedemann Closes-Bug: #1829542 Change-Id: Idfd099f718873d9056fdc35a97954771c9ae5762 --- watcher/datasources/ceilometer.py | 22 +-- .../datasources/test_ceilometer_helper.py | 159 +++++++----------- 2 files changed, 76 insertions(+), 105 deletions(-) diff --git a/watcher/datasources/ceilometer.py b/watcher/datasources/ceilometer.py index 28e3eeee7..215ad01de 100644 --- a/watcher/datasources/ceilometer.py +++ b/watcher/datasources/ceilometer.py @@ -232,64 +232,64 @@ class CeilometerHelper(base.DataSourceBase): granularity=None): meter_name = self.METRIC_MAP.get('host_cpu_usage') return self.statistic_aggregation(resource_id, meter_name, period, - granularity, aggregate=aggregate) + granularity, aggregation=aggregate) def get_instance_cpu_usage(self, resource_id, period, aggregate, granularity=None): meter_name = self.METRIC_MAP.get('instance_cpu_usage') return self.statistic_aggregation(resource_id, meter_name, period, - granularity, aggregate=aggregate) + granularity, aggregation=aggregate) def get_host_memory_usage(self, resource_id, period, aggregate, granularity=None): meter_name = self.METRIC_MAP.get('host_memory_usage') return self.statistic_aggregation(resource_id, meter_name, period, - granularity, aggregate=aggregate) + granularity, aggregation=aggregate) def get_instance_ram_usage(self, resource_id, period, aggregate, granularity=None): meter_name = self.METRIC_MAP.get('instance_ram_usage') return self.statistic_aggregation(resource_id, meter_name, period, - granularity, aggregate=aggregate) + granularity, aggregation=aggregate) def get_instance_l3_cache_usage(self, resource_id, period, aggregate, granularity=None): meter_name = self.METRIC_MAP.get('instance_l3_cache_usage') return self.statistic_aggregation(resource_id, meter_name, period, - granularity, aggregate=aggregate) + granularity, aggregation=aggregate) def get_instance_ram_allocated(self, resource_id, period, aggregate, granularity=None): meter_name = self.METRIC_MAP.get('instance_ram_allocated') return self.statistic_aggregation(resource_id, meter_name, period, - granularity, aggregate=aggregate) + granularity, aggregation=aggregate) def get_instance_root_disk_size(self, resource_id, period, aggregate, granularity=None): meter_name = self.METRIC_MAP.get('instance_root_disk_size') return self.statistic_aggregation(resource_id, meter_name, period, - granularity, aggregate=aggregate) + granularity, aggregation=aggregate) def get_host_outlet_temp(self, resource_id, period, aggregate, granularity=None): meter_name = self.METRIC_MAP.get('host_outlet_temp') return self.statistic_aggregation(resource_id, meter_name, period, - granularity, aggregate=aggregate) + granularity, aggregation=aggregate) def get_host_inlet_temp(self, resource_id, period, aggregate, granularity=None): meter_name = self.METRIC_MAP.get('host_inlet_temp') return self.statistic_aggregation(resource_id, meter_name, period, - granularity, aggregate=aggregate) + granularity, aggregation=aggregate) def get_host_airflow(self, resource_id, period, aggregate, granularity=None): meter_name = self.METRIC_MAP.get('host_airflow') return self.statistic_aggregation(resource_id, meter_name, period, - granularity, aggregate=aggregate) + granularity, aggregation=aggregate) def get_host_power(self, resource_id, period, aggregate, granularity=None): meter_name = self.METRIC_MAP.get('host_power') return self.statistic_aggregation(resource_id, meter_name, period, - granularity, aggregate=aggregate) + granularity, aggregation=aggregate) diff --git a/watcher/tests/datasources/test_ceilometer_helper.py b/watcher/tests/datasources/test_ceilometer_helper.py index f07321804..7251c1f0c 100644 --- a/watcher/tests/datasources/test_ceilometer_helper.py +++ b/watcher/tests/datasources/test_ceilometer_helper.py @@ -27,6 +27,16 @@ from watcher.tests import base @mock.patch.object(clients.OpenStackClients, 'ceilometer') class TestCeilometerHelper(base.BaseTestCase): + def setUp(self): + super(TestCeilometerHelper, self).setUp() + self.osc_mock = mock.Mock() + self.helper = ceilometer_helper.CeilometerHelper(osc=self.osc_mock) + stat_agg_patcher = mock.patch.object( + self.helper, 'statistic_aggregation', + spec=ceilometer_helper.CeilometerHelper.statistic_aggregation) + self.mock_aggregation = stat_agg_patcher.start() + self.addCleanup(stat_agg_patcher.stop) + def test_build_query(self, mock_ceilometer): mock_ceilometer.return_value = mock.MagicMock() cm = ceilometer_helper.CeilometerHelper() @@ -95,110 +105,71 @@ class TestCeilometerHelper(base.BaseTestCase): val = cm.statistic_list(meter_name="cpu_util") self.assertEqual(expected_value, val) - @mock.patch.object(ceilometer_helper.CeilometerHelper, - 'statistic_aggregation') - def test_get_host_cpu_usage(self, mock_aggregation, mock_ceilometer): - helper = ceilometer_helper.CeilometerHelper() - helper.get_host_cpu_usage('compute1', 600, 'mean') - mock_aggregation.assert_called_once_with( - 'compute1', helper.METRIC_MAP['host_cpu_usage'], 600, None, - aggregate='mean') + def test_get_host_cpu_usage(self, mock_ceilometer): + self.helper.get_host_cpu_usage('compute1', 600, 'mean') + self.mock_aggregation.assert_called_once_with( + 'compute1', self.helper.METRIC_MAP['host_cpu_usage'], 600, None, + aggregation='mean') - @mock.patch.object(ceilometer_helper.CeilometerHelper, - 'statistic_aggregation') - def test_get_instance_cpu_usage(self, mock_aggregation, mock_ceilometer): - helper = ceilometer_helper.CeilometerHelper() - helper.get_instance_cpu_usage('compute1', 600, 'mean') - mock_aggregation.assert_called_once_with( - 'compute1', helper.METRIC_MAP['instance_cpu_usage'], 600, None, - aggregate='mean') + def test_get_instance_cpu_usage(self, mock_ceilometer): + self.helper.get_instance_cpu_usage('compute1', 600, 'mean') + self.mock_aggregation.assert_called_once_with( + 'compute1', self.helper.METRIC_MAP['instance_cpu_usage'], 600, + None, aggregation='mean') - @mock.patch.object(ceilometer_helper.CeilometerHelper, - 'statistic_aggregation') - def test_get_host_memory_usage(self, mock_aggregation, mock_ceilometer): - helper = ceilometer_helper.CeilometerHelper() - helper.get_host_memory_usage('compute1', 600, 'mean') - mock_aggregation.assert_called_once_with( - 'compute1', helper.METRIC_MAP['host_memory_usage'], 600, None, - aggregate='mean') + def test_get_host_memory_usage(self, mock_ceilometer): + self.helper.get_host_memory_usage('compute1', 600, 'mean') + self.mock_aggregation.assert_called_once_with( + 'compute1', self.helper.METRIC_MAP['host_memory_usage'], 600, None, + aggregation='mean') - @mock.patch.object(ceilometer_helper.CeilometerHelper, - 'statistic_aggregation') - def test_get_instance_memory_usage(self, mock_aggregation, - mock_ceilometer): - helper = ceilometer_helper.CeilometerHelper() - helper.get_instance_ram_usage('compute1', 600, 'mean') - mock_aggregation.assert_called_once_with( - 'compute1', helper.METRIC_MAP['instance_ram_usage'], 600, None, - aggregate='mean') + def test_get_instance_memory_usage(self, mock_ceilometer): + self.helper.get_instance_ram_usage('compute1', 600, 'mean') + self.mock_aggregation.assert_called_once_with( + 'compute1', self.helper.METRIC_MAP['instance_ram_usage'], 600, + None, aggregation='mean') - @mock.patch.object(ceilometer_helper.CeilometerHelper, - 'statistic_aggregation') - def test_get_instance_l3_cache_usage(self, mock_aggregation, - mock_ceilometer): - helper = ceilometer_helper.CeilometerHelper() - helper.get_instance_l3_cache_usage('compute1', 600, 'mean') - mock_aggregation.assert_called_once_with( - 'compute1', helper.METRIC_MAP['instance_l3_cache_usage'], 600, - None, aggregate='mean') + def test_get_instance_l3_cache_usage(self, mock_ceilometer): + self.helper.get_instance_l3_cache_usage('compute1', 600, 'mean') + self.mock_aggregation.assert_called_once_with( + 'compute1', self.helper.METRIC_MAP['instance_l3_cache_usage'], 600, + None, aggregation='mean') - @mock.patch.object(ceilometer_helper.CeilometerHelper, - 'statistic_aggregation') - def test_get_instance_ram_allocated(self, mock_aggregation, - mock_ceilometer): - helper = ceilometer_helper.CeilometerHelper() - helper.get_instance_ram_allocated('compute1', 600, 'mean') - mock_aggregation.assert_called_once_with( - 'compute1', helper.METRIC_MAP['instance_ram_allocated'], 600, None, - aggregate='mean') + def test_get_instance_ram_allocated(self, mock_ceilometer): + self.helper.get_instance_ram_allocated('compute1', 600, 'mean') + self.mock_aggregation.assert_called_once_with( + 'compute1', self.helper.METRIC_MAP['instance_ram_allocated'], 600, + None, aggregation='mean') - @mock.patch.object(ceilometer_helper.CeilometerHelper, - 'statistic_aggregation') - def test_get_instance_root_disk_allocated(self, mock_aggregation, - mock_ceilometer): - helper = ceilometer_helper.CeilometerHelper() - helper.get_instance_root_disk_size('compute1', 600, 'mean') - mock_aggregation.assert_called_once_with( - 'compute1', helper.METRIC_MAP['instance_root_disk_size'], 600, - None, aggregate='mean') + def test_get_instance_root_disk_allocated(self, mock_ceilometer): + self.helper.get_instance_root_disk_size('compute1', 600, 'mean') + self.mock_aggregation.assert_called_once_with( + 'compute1', self.helper.METRIC_MAP['instance_root_disk_size'], 600, + None, aggregation='mean') - @mock.patch.object(ceilometer_helper.CeilometerHelper, - 'statistic_aggregation') - def test_get_host_outlet_temperature(self, mock_aggregation, - mock_ceilometer): - helper = ceilometer_helper.CeilometerHelper() - helper.get_host_outlet_temp('compute1', 600, 'mean') - mock_aggregation.assert_called_once_with( - 'compute1', helper.METRIC_MAP['host_outlet_temp'], 600, None, - aggregate='mean') + def test_get_host_outlet_temperature(self, mock_ceilometer): + self.helper.get_host_outlet_temp('compute1', 600, 'mean') + self.mock_aggregation.assert_called_once_with( + 'compute1', self.helper.METRIC_MAP['host_outlet_temp'], 600, None, + aggregation='mean') - @mock.patch.object(ceilometer_helper.CeilometerHelper, - 'statistic_aggregation') - def test_get_host_inlet_temperature(self, mock_aggregation, - mock_ceilometer): - helper = ceilometer_helper.CeilometerHelper() - helper.get_host_inlet_temp('compute1', 600, 'mean') - mock_aggregation.assert_called_once_with( - 'compute1', helper.METRIC_MAP['host_inlet_temp'], 600, None, - aggregate='mean') + def test_get_host_inlet_temperature(self, mock_ceilometer): + self.helper.get_host_inlet_temp('compute1', 600, 'mean') + self.mock_aggregation.assert_called_once_with( + 'compute1', self.helper.METRIC_MAP['host_inlet_temp'], 600, None, + aggregation='mean') - @mock.patch.object(ceilometer_helper.CeilometerHelper, - 'statistic_aggregation') - def test_get_host_airflow(self, mock_aggregation, mock_ceilometer): - helper = ceilometer_helper.CeilometerHelper() - helper.get_host_airflow('compute1', 600, 'mean') - mock_aggregation.assert_called_once_with( - 'compute1', helper.METRIC_MAP['host_airflow'], 600, None, - aggregate='mean') + def test_get_host_airflow(self, mock_ceilometer): + self.helper.get_host_airflow('compute1', 600, 'mean') + self.mock_aggregation.assert_called_once_with( + 'compute1', self.helper.METRIC_MAP['host_airflow'], 600, None, + aggregation='mean') - @mock.patch.object(ceilometer_helper.CeilometerHelper, - 'statistic_aggregation') - def test_get_host_power(self, mock_aggregation, mock_ceilometer): - helper = ceilometer_helper.CeilometerHelper() - helper.get_host_power('compute1', 600, 'mean') - mock_aggregation.assert_called_once_with( - 'compute1', helper.METRIC_MAP['host_power'], 600, None, - aggregate='mean') + def test_get_host_power(self, mock_ceilometer): + self.helper.get_host_power('compute1', 600, 'mean') + self.mock_aggregation.assert_called_once_with( + 'compute1', self.helper.METRIC_MAP['host_power'], 600, None, + aggregation='mean') def test_check_availability(self, mock_ceilometer): ceilometer = mock.MagicMock()