Gating: assume 'waiting' for two hours on missing results (#4219)

This is part of addressing #4219. Currently we always count the
gating status as failed if requirements are unsatisfied. However,
it's usual for test results to not be present yet for a little
while after the update is created or edited. Ideally we would
have a mechanism for knowing for sure whether the tests in
question are pending, but we don't, so let's use a dumb rule
instead: if there are no failures (only missing results), and
it's less than two hours since the update was last modified, we'll
return 'waiting' as the status. After two hours, maybe the tests
actually didn't get scheduled or the policy is wrong, so we'll
return 'failed'.

This also substantially simplifies the logic used to decide the
status, and reduces code duplication. We can determine the status
using only the number of satisfied and unsatisfied requirements:
if there are none of either it is ignored, if there are satisfied
requirements and no unsatisfied requirements it is passed,
otherwise it is failed or waiting.

As part of this we change the mock greenwave responses used in
a lot of other tests to use test-result-failed not -missing,
because they currently use -missing but expect the gating status
to be 'failed', which it no longer always will be. We also have
to make the mock greenwave responses used in tests that hit this
code a bit more realistic - they have to always include the
satisfied and unsatisfied requirement lists. Note I've checked
greenwave code and it does always include these in a successful
query response.

Signed-off-by: Adam Williamson <awilliam@redhat.com>

bodhi/server/models.py

@@ -19,7 +19,7 @@
 
 from collections import defaultdict
 from copy import copy
-from datetime import datetime
+from datetime import datetime, timedelta
 from textwrap import wrap
 import hashlib
 import json
@@ -2213,42 +2213,61 @@ def get_test_gating_info(self):
         }
         return util.greenwave_api_post(self._greenwave_api_url, data)
 
+    @property
+    def _greenwave_requirements(self):
+        """
+        Query Greenwave about this update and return satisfied and unsatisfied
+        requirements.
+
+        Returns:
+            tuple: A 2-tuple of lists: first item satisfied requirements, second
+            item unsatisfied requirements. Either list or both may be empty.
+
+        Raises:
+            BodhiException: When the ``greenwave_api_url`` is undefined in configuration.
+            RuntimeError: If Greenwave did not give us a 200 code, including if no applicable
+            policies were found.
+        """
+        satisfied_requirements = []
+        unsatisfied_requirements = []
+        for data in self.greenwave_request_batches(verbose=False):
+            response = util.greenwave_api_post(self._greenwave_api_url, data)
+            satisfied_requirements.extend(response.get('satisfied_requirements', []))
+            unsatisfied_requirements.extend(response.get('unsatisfied_requirements', []))
+
+        return (satisfied_requirements, unsatisfied_requirements)
+
     def _get_test_gating_status(self):
         """
         Query Greenwave about this update and return the information retrieved.
 
         Returns:
             TestGatingStatus:
                 - TestGatingStatus.ignored if no tests are required
-                - TestGatingStatus.failed if policies are not satisfied
-                - TestGatingStatus.passed if policies are satisfied, and there
-                  are required tests
+                - TestGatingStatus.passed if there are required tests and policies are satisfied
+                - TestGatingStatus.waiting if there are required tests that have not yet completed,
+                  no required test has failed, and update was last modified less than 2 hours ago
+                - TestGatingStatus.failed otherwise (failed required tests, missing required
+                  test results and last modified more than 2 hours ago)
 
         Raises:
             BodhiException: When the ``greenwave_api_url`` is undefined in configuration.
-            RuntimeError: If Greenwave did not give us a 200 code.
+            RuntimeError: If Greenwave did not give us a 200 code, including if no applicable
+            policies were found.
         """
-        # If an unrestricted policy is applied and no tests are required
-        # on this update, let's set the test gating as ignored in Bodhi.
-        status = TestGatingStatus.ignored
-        for data in self.greenwave_request_batches(verbose=False):
-            response = util.greenwave_api_post(self._greenwave_api_url, data)
-            if not response['policies_satisfied']:
-                return TestGatingStatus.failed
+        (satisfied, unsatisfied) = self._greenwave_requirements
 
-            if status != TestGatingStatus.ignored or response['summary'] != 'no tests are required':
-                status = TestGatingStatus.passed
+        if not satisfied and not unsatisfied:
+            return TestGatingStatus.ignored
 
-        return status
+        if satisfied and not unsatisfied:
+            return TestGatingStatus.passed
 
-    @property
-    def _unsatisfied_requirements(self):
-        unsatisfied_requirements = []
-        for data in self.greenwave_request_batches(verbose=False):
-            response = util.greenwave_api_post(self._greenwave_api_url, data)
-            unsatisfied_requirements.extend(response['unsatisfied_requirements'])
+        if datetime.utcnow() - self.last_modified < timedelta(hours=2):
+            if all(req.get('type', '') == 'test-result-missing' for req in unsatisfied):
+                return TestGatingStatus.waiting
 
-        return unsatisfied_requirements
+        return TestGatingStatus.failed
 
     @property
     def install_command(self) -> str:
@@ -3056,7 +3075,8 @@ def waive_test_results(self, username, comment=None, tests=None):
         # Ensure we can always iterate over tests
         tests = tests or []
 
-        for requirement in self._unsatisfied_requirements:
+        # these are the unsatisfied requirements
+        for requirement in self._greenwave_requirements[1]:
 
             if tests and requirement['testcase'] not in tests:
                 continue

bodhi/tests/server/consumers/test_greenwave.py

@@ -40,6 +40,21 @@ def setup_method(self, method):
                 "subject_type": "koji_build",
                 'policies_satisfied': True,
                 'summary': "all tests have passed",
+                'applicable_policies': [
+                    'kojibuild_bodhipush_no_requirements',
+                    'kojibuild_bodhipush_remoterule',
+                    'bodhiupdate_bodhipush_no_requirements',
+                    'bodhiupdate_bodhipush_openqa'
+                ],
+                'satisfied_requirements': [
+                    {
+                        'result_id': 39603316,
+                        'subject_type': 'bodhi_update',
+                        'testcase': 'update.install_default_update_netinst',
+                        'type': 'test-result-passed'
+                    },
+                ],
+                'unsatisfied_requirements': []
             },
         )
         self.handler = greenwave.GreenwaveHandler()
@@ -72,6 +87,19 @@ def test_single_build_failed(self):
         update = self.single_build_update
 
         self.sample_message.body["policies_satisfied"] = False
+        self.sample_message.body["summary"] = "1 of 1 required tests failed"
+        self.sample_message.body["satisfied_requirements"] = []
+        self.sample_message.body["unsatisfied_requirements"] = [
+            {
+                'item': {
+                    'type': 'bodhi_update'
+                },
+                'scenario': 'fedora.updates-everything-boot-iso.x86_64.64bit',
+                'subject_type': 'bodhi_update',
+                'testcase': 'update.install_default_update_netinst',
+                'type': 'test-result-failed'
+            }
+        ]
         self.handler(self.sample_message)
         assert update.test_gating_status == models.TestGatingStatus.failed
 
@@ -84,6 +112,8 @@ def test_single_build_ignored(self):
 
         self.sample_message.body["policies_satisfied"] = True
         self.sample_message.body["summary"] = "no tests are required"
+        self.sample_message.body["satisfied_requirements"] = []
+        self.sample_message.body["unsatisfied_requirements"] = []
         self.handler(self.sample_message)
         assert update.test_gating_status == models.TestGatingStatus.ignored
 
@@ -114,7 +144,22 @@ def test_multiple_builds(self):
         with mock.patch('bodhi.server.models.util.greenwave_api_post') as mock_greenwave:
             greenwave_response = {
                 'policies_satisfied': True,
-                'summary': "all tests have passed"
+                'summary': "All required tests passed",
+                'applicable_policies': [
+                    'kojibuild_bodhipush_no_requirements',
+                    'kojibuild_bodhipush_remoterule',
+                    'bodhiupdate_bodhipush_no_requirements',
+                    'bodhiupdate_bodhipush_openqa'
+                ],
+                'satisfied_requirements': [
+                    {
+                        'result_id': 39603316,
+                        'subject_type': 'bodhi_update',
+                        'testcase': 'update.install_default_update_netinst',
+                        'type': 'test-result-passed'
+                    },
+                ],
+                'unsatisfied_requirements': []
             }
             mock_greenwave.return_value = greenwave_response
             self.handler(self.sample_message)

bodhi/tests/server/consumers/test_signed.py

@@ -239,7 +239,22 @@ def test_consume_from_tag(self):
         with mock.patch('bodhi.server.models.util.greenwave_api_post') as mock_greenwave:
             greenwave_response = {
                 'policies_satisfied': True,
-                'summary': "all tests have passed"
+                'summary': "All required tests passed",
+                'applicable_policies': [
+                    'kojibuild_bodhipush_no_requirements',
+                    'kojibuild_bodhipush_remoterule',
+                    'bodhiupdate_bodhipush_no_requirements',
+                    'bodhiupdate_bodhipush_openqa'
+                ],
+                'satisfied_requirements': [
+                    {
+                        'result_id': 39603316,
+                        'subject_type': 'bodhi_update',
+                        'testcase': 'update.install_default_update_netinst',
+                        'type': 'test-result-passed'
+                    },
+                ],
+                'unsatisfied_requirements': []
             }
             mock_greenwave.return_value = greenwave_response
             with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV1):
@@ -276,7 +291,22 @@ def test_consume_from_tag_composed_by_bodhi(self, add_tag):
         with mock.patch('bodhi.server.models.util.greenwave_api_post') as mock_greenwave:
             greenwave_response = {
                 'policies_satisfied': True,
-                'summary': "all tests have passed"
+                'summary': "All required tests passed",
+                'applicable_policies': [
+                    'kojibuild_bodhipush_no_requirements',
+                    'kojibuild_bodhipush_remoterule',
+                    'bodhiupdate_bodhipush_no_requirements',
+                    'bodhiupdate_bodhipush_openqa'
+                ],
+                'satisfied_requirements': [
+                    {
+                        'result_id': 39603316,
+                        'subject_type': 'bodhi_update',
+                        'testcase': 'update.install_default_update_netinst',
+                        'type': 'test-result-passed'
+                    },
+                ],
+                'unsatisfied_requirements': []
             }
             mock_greenwave.return_value = greenwave_response
             with fml_testing.mock_sends(update_schemas.UpdateRequestTestingV1):

bodhi/tests/server/services/test_updates.py

@@ -5843,7 +5843,7 @@ def test_waive_test_results_1_unsatisfied_requirement(
                     },
                     'scenario': None,
                     'testcase': 'dist.rpmdeplint',
-                    'type': 'test-result-missing'
+                    'type': 'test-result-failed'
                 }
             ],
         }
@@ -5912,7 +5912,7 @@ def test_waive_test_results_2_unsatisfied_requirements(
                     },
                     'scenario': None,
                     'testcase': 'dist.rpmdeplint',
-                    'type': 'test-result-missing'
+                    'type': 'test-result-failed'
                 },
                 {
                     'item': {
@@ -5921,7 +5921,7 @@ def test_waive_test_results_2_unsatisfied_requirements(
                     },
                     'scenario': None,
                     'testcase': 'atomic_ci_pipeline_results',
-                    'type': 'test-result-missing'
+                    'type': 'test-result-failed'
                 }
             ],
         }
@@ -6006,7 +6006,7 @@ def test_waive_test_results_1_of_2_unsatisfied_requirements(
                     },
                     'scenario': None,
                     'testcase': 'dist.rpmdeplint',
-                    'type': 'test-result-missing'
+                    'type': 'test-result-failed'
                 },
                 {
                     'item': {
@@ -6015,7 +6015,7 @@ def test_waive_test_results_1_of_2_unsatisfied_requirements(
                     },
                     'scenario': None,
                     'testcase': 'atomic_ci_pipeline_results',
-                    'type': 'test-result-missing'
+                    'type': 'test-result-failed'
                 }
             ],
         }
@@ -6088,7 +6088,7 @@ def test_waive_test_results_2_of_2_unsatisfied_requirements(
                     },
                     'scenario': None,
                     'testcase': 'dist.rpmdeplint',
-                    'type': 'test-result-missing'
+                    'type': 'test-result-failed'
                 },
                 {
                     'item': {
@@ -6097,7 +6097,7 @@ def test_waive_test_results_2_of_2_unsatisfied_requirements(
                     },
                     'scenario': None,
                     'testcase': 'atomic_ci_pipeline_results',
-                    'type': 'test-result-missing'
+                    'type': 'test-result-failed'
                 }
             ],
         }
@@ -6186,7 +6186,7 @@ def test_waive_test_results_unfailing_tests(
                     },
                     'scenario': None,
                     'testcase': 'dist.rpmdeplint',
-                    'type': 'test-result-missing'
+                    'type': 'test-result-failed'
                 },
                 {
                     'item': {
@@ -6195,7 +6195,7 @@ def test_waive_test_results_unfailing_tests(
                     },
                     'scenario': None,
                     'testcase': 'atomic_ci_pipeline_results',
-                    'type': 'test-result-missing'
+                    'type': 'test-result-failed'
                 }
             ],
         }