[fix] Fixed issues in metric collection (mock requests + empty upgrad…

…es) #360

- Fixed mocking of request library in MockRequestPostRunner 
- Do not send metrics on post_migrate if no module is upgraded

Related to #360

---------

Co-authored-by: Federico Capoano <[email protected]>

openwisp_utils/measurements/tasks.py

@@ -50,6 +50,8 @@ def send_usage_metrics(upgrade_only=False):
         OpenwispVersion.objects.create(module_version=current_versions)
     else:
         upgraded_modules = OpenwispVersion.get_upgraded_modules(current_versions)
+        if upgrade_only and not upgraded_modules:
+            return
         metrics.extend(_get_events('Upgrade', upgraded_modules))
         if not upgrade_only:
             metrics.extend(get_openwisp_module_metrics(current_versions))

openwisp_utils/measurements/tests/runner.py

@@ -1,19 +1,30 @@
-from unittest.mock import MagicMock
+from unittest.mock import patch
 
 import requests
-from django.test.runner import DiscoverRunner
 from openwisp_utils import utils
+from openwisp_utils.tests import TimeLoggingTestRunner
 
 success_response = requests.Response()
 success_response.status_code = 204
 
 
-class MockRequestPostRunner(DiscoverRunner):
+class MockRequestPostRunner(TimeLoggingTestRunner):
     """
     This runner ensures that usage metrics are
     not sent in development when running tests.
     """
 
+    pass
+
     def setup_databases(self, **kwargs):
-        utils.requests.post = MagicMock(return_value=success_response)
-        return super().setup_databases(**kwargs)
+        utils.requests.Session._original_post = utils.requests.Session.post
+        with patch.object(
+            utils.requests.Session, 'post', return_value=success_response
+        ):
+            return super().setup_databases(**kwargs)
+
+    def run_suite(self, suite, **kwargs):
+        with patch.object(
+            utils.requests.Session, 'post', return_value=success_response
+        ):
+            return super().run_suite(suite)

openwisp_utils/measurements/tests/test_models.py

@@ -6,6 +6,7 @@
 from django.db import migrations
 from django.test import TestCase, override_settings
 from freezegun import freeze_time
+from openwisp_utils import utils
 from urllib3.response import HTTPResponse
 
 from .. import tasks
@@ -167,24 +168,54 @@ def test_send_usage_metrics_upgrade_only_flag(
         }
         self.assertEqual(version.module_version, expected_module_version)
 
+    @freeze_time('2023-12-01 00:00:00')
+    @patch.object(tasks, 'get_openwisp_version', return_value='23.0.0a')
+    @patch.object(
+        tasks,
+        'get_enabled_openwisp_modules',
+        return_value=_ENABLED_OPENWISP_MODULES_RETURN_VALUE,
+    )
+    @patch.object(
+        tasks,
+        'get_os_details',
+        return_value=_OS_DETAILS_RETURN_VALUE,
+    )
+    @patch.object(tasks, 'post_usage_metrics')
+    @patch.object(tasks, 'get_openwisp_module_metrics')
+    def test_send_usage_metrics_upgrade_only_without_upgrades(
+        self, mocked_get_openwisp_module_metrics, mocked_post_usage_metrics, *args
+    ):
+        """
+        This test checks that no usage metrics are sent when
+        there are no OpenWISP modules upgrades and upgrade_only
+        is set to True.
+        """
+        self.assertEqual(OpenwispVersion.objects.count(), 1)
+        tasks.send_usage_metrics.delay(upgrade_only=True)
+        mocked_get_openwisp_module_metrics.assert_not_called()
+        mocked_post_usage_metrics.assert_not_called()
+
     @patch('time.sleep')
     @patch('logging.Logger.warning')
     @patch('logging.Logger.error')
     def test_post_usage_metrics_400_response(self, mocked_error, mocked_warning, *args):
         bad_response = requests.Response()
         bad_response.status_code = 400
         with patch.object(
-            requests.Session, 'post', return_value=bad_response
-        ) as mocked_post:
+            tasks, 'retryable_request', return_value=bad_response
+        ) as mocked_retryable_request:
             tasks.send_usage_metrics.delay()
-        mocked_post.assert_called_once()
+        mocked_retryable_request.assert_called_once()
         mocked_warning.assert_not_called()
         mocked_error.assert_called_with(
             'Collection of usage metrics failed, max retries exceeded.'
             ' Error: HTTP 400 Response'
         )
 
     @patch('urllib3.util.retry.Retry.sleep')
+    @patch(
+        'urllib3.connectionpool.HTTPConnection.request',
+    )
     @patch(
         'urllib3.connectionpool.HTTPConnection.getresponse',
         return_value=HTTPResponse(status=500, version='1.1'),
@@ -193,7 +224,11 @@ def test_post_usage_metrics_400_response(self, mocked_error, mocked_warning, *ar
     def test_post_usage_metrics_500_response(
         self, mocked_error, mocked_getResponse, *args
     ):
-        tasks.send_usage_metrics.delay()
+        # Unmock post request from MockedRequestPostRunner
+        with patch.object(
+            utils.requests.Session, 'post', new=utils.requests.Session._original_post
+        ):
+            tasks.send_usage_metrics.delay()
         self.assertEqual(len(mocked_getResponse.mock_calls), 11)
         mocked_error.assert_called_with(
             'Collection of usage metrics failed, max retries exceeded.'
@@ -206,17 +241,20 @@ def test_post_usage_metrics_500_response(
     @patch('logging.Logger.warning')
     @patch('logging.Logger.error')
     def test_post_usage_metrics_204_response(self, mocked_error, mocked_warning, *args):
-        bad_response = requests.Response()
-        bad_response.status_code = 204
+        success_response = requests.Response()
+        success_response.status_code = 204
         with patch.object(
-            requests.Session, 'post', return_value=bad_response
-        ) as mocked_post:
+            tasks, 'retryable_request', return_value=success_response
+        ) as mocked_retryable_request:
             tasks.send_usage_metrics.delay()
-        self.assertEqual(len(mocked_post.mock_calls), 1)
+        self.assertEqual(len(mocked_retryable_request.mock_calls), 1)
         mocked_warning.assert_not_called()
         mocked_error.assert_not_called()
 
     @patch('urllib3.util.retry.Retry.sleep')
+    @patch(
+        'urllib3.connectionpool.HTTPConnection.request',
+    )
     @patch(
         'urllib3.connectionpool.HTTPConnectionPool._get_conn',
         side_effect=OSError,
@@ -225,7 +263,11 @@ def test_post_usage_metrics_204_response(self, mocked_error, mocked_warning, *ar
     def test_post_usage_metrics_connection_error(
         self, mocked_error, mocked_get_conn, *args
     ):
-        tasks.send_usage_metrics.delay()
+        # Unmock post request from MockedRequestPostRunner
+        with patch.object(
+            utils.requests.Session, 'post', new=utils.requests.Session._original_post
+        ):
+            tasks.send_usage_metrics.delay()
         mocked_error.assert_called_with(
             'Collection of usage metrics failed, max retries exceeded.'
             ' Error: HTTPSConnectionPool(host=\'analytics.openwisp.io\', port=443):'