Rewrite summarize_answers to be more accurate

The summaries are getting pretty inaccurate in certain cases.
Incomplete tests aren't really "missing", but they're treated
as such in the summary. Problems with gating.yaml files are
counted as failed tests, when they really aren't. The special
handling of errors (we kinda count them as both failures and
errors) doesn't really make much sense.

This rewrites summarize_answers entirely to give more accurate
summaries, with logic that's hopefully clearer, and with only
one iteration over the answers. It's fairly similar to the
javascript code in Bodhi which does much the same work (we have
to duplicate this in Bodhi so it can create a single combined
summary for the results of multiple greenwave queries, when it
has to run more than one query).

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

greenwave/policies.py

@@ -331,6 +331,7 @@ class InvalidRemoteRuleYaml(RuleNotSatisfied):
     """
 
     scenario = None
+    is_test_result = False
 
     def __init__(self, subject, test_case_name, details, source):
         self.subject = subject
@@ -360,6 +361,7 @@ class MissingRemoteRuleYaml(RuleNotSatisfied):
 
     test_case_name = 'missing-gating-yaml'
     scenario = None
+    is_test_result = False
 
     def __init__(self, subject, sources):
         self.subject = subject
@@ -384,9 +386,9 @@ class FailedFetchRemoteRuleYaml(RuleNotSatisfied):
     Error while fetching remote policy.
     """
 
-    scenario = None
-
     test_case_name = 'failed-fetch-gating-yaml'
+    scenario = None
+    is_test_result = False
 
     def __init__(self, subject, sources, error):
         self.subject = subject
@@ -458,6 +460,9 @@ class ExcludedInPolicy(RuleSatisfied):
     """
     Package was excluded in policy.
     """
+
+    is_test_result = False
+
     def __init__(self, subject_identifier, policy):
         self.subject_identifier = subject_identifier
         self.policy = policy
@@ -471,37 +476,6 @@ def to_json(self):
         }
 
 
-def _summarize_answers_without_errored(answers):
-    failure_count = sum(
-        1 for answer in answers
-        if isinstance(answer, RuleNotSatisfied)
-    )
-    missing_count = sum(
-        1 for answer in answers
-        if isinstance(answer, (TestResultIncomplete, TestResultMissing))
-    )
-
-    # Missing results are also failures but we will distinguish between those
-    # two in summary message.
-    failure_count -= missing_count
-
-    if failure_count and missing_count:
-        return '{} of {} required tests failed, {} result{} missing'.format(
-            failure_count, len(answers), missing_count, 's' if missing_count > 1 else '')
-
-    if failure_count > 0:
-        return '{} of {} required tests failed'.format(failure_count, len(answers))
-
-    if missing_count > 0:
-        return '{} of {} required test results missing'.format(missing_count, len(answers))
-
-    if all(answer.is_satisfied for answer in answers):
-        return 'All required tests passed'
-
-    logging.error('Unexpected unsatisfied result')
-    return 'inexplicable result'
-
-
 def summarize_answers(answers):
     """
     Produces a one-sentence human-readable summary of the result of evaluating a policy.
@@ -512,23 +486,61 @@ def summarize_answers(answers):
     Returns:
         str: Human-readable summary.
     """
-    test_answers = [
-        answer for answer in answers
-        if answer.is_test_result
-    ]
-    if not test_answers:
-        return 'no tests are required'
-
-    summary = _summarize_answers_without_errored(test_answers)
-
-    errored_count = len([
-        answer for answer in test_answers
-        if isinstance(answer, TestResultErrored)
-    ])
-    if errored_count:
-        summary += f' ({errored_count} {"error" if errored_count == 1 else "errors"})'
-
-    return summary
+    # let's count! counting is fun!
+    test_count = 0
+    failed_nontest_count = 0
+    failed_test_count = 0
+    errored_test_count = 0
+    incomplete_test_count = 0
+    missing_test_count = 0
+    for answer in answers:
+        # handle non test results first, makes logic easier
+        if not answer.is_test_result:
+            if isinstance(answer, RuleNotSatisfied):
+                failed_nontest_count += 1
+            continue
+        # now we're only dealing with test results
+        test_count += 1
+        if isinstance(answer, TestResultIncomplete):
+            incomplete_test_count += 1
+        elif isinstance(answer, TestResultMissing):
+            missing_test_count += 1
+        elif isinstance(answer, TestResultErrored):
+            errored_test_count += 1
+        elif isinstance(answer, RuleNotSatisfied):
+            failed_test_count += 1
+
+    # now generate the text
+    # first handle cases where there are unsatisfied results
+    msgstr = ""
+    if failed_nontest_count:
+        msgstr = "{} non-test-result unsatisfied requirement(s) (gating.yaml issues)"
+        msgstr = msgstr.format(failed_nontest_count)
+
+    testmsgs = ["Of {} required test(s)".format(test_count)]
+    if failed_test_count:
+        testmsgs.append("{} test(s) failed".format(failed_test_count))
+    if errored_test_count:
+        testmsgs.append("{} test(s) errored".format(errored_test_count))
+    if incomplete_test_count:
+        testmsgs.append("{} test(s) incomplete".format(incomplete_test_count))
+    if missing_test_count:
+        testmsgs.append("{} result(s) missing".format(missing_test_count))
+    if len(testmsgs) > 1:
+        if msgstr:
+            msgstr += ". "
+        msgstr += ", ".join(testmsgs)
+
+    if msgstr:
+        return msgstr
+
+    # if we got here, there should be no unsatisfied results
+    if test_count:
+        # this means we had some passed/waived tests
+        return 'All required tests passed or waived'
+
+    # otherwise, should mean we had no required tests
+    return 'No tests are required'
 
 
 class Rule(SafeYAMLObject):

greenwave/tests/test_api_v1.py

@@ -72,7 +72,7 @@ def test_make_decision_retrieves_waivers_on_missing(mock_results, mock_waivers,
     mock_waivers.return_value = []
     response = make_decision()
     assert 200 == response.status_code
-    assert '1 of 1 required test results missing' == response.json['summary']
+    assert 'Of 1 required test(s), 1 result(s) missing' == response.json['summary']
     mock_waivers.assert_called_once()
 
 
@@ -81,7 +81,7 @@ def test_make_decision_retrieves_waivers_on_failed(mock_results, mock_waivers, m
     mock_waivers.return_value = []
     response = make_decision()
     assert 200 == response.status_code
-    assert '1 of 1 required tests failed' == response.json['summary']
+    assert 'Of 1 required test(s), 1 test(s) failed' == response.json['summary']
     mock_waivers.assert_called_once()
 
 
@@ -91,7 +91,7 @@ def test_make_decision_retrieves_waivers_omitted_on_passed(
     mock_waivers.return_value = []
     response = make_decision()
     assert 200 == response.status_code
-    assert 'All required tests passed' == response.json['summary']
+    assert 'All required tests passed or waived' == response.json['summary']
     mock_waivers.assert_not_called()
 
 
@@ -100,7 +100,7 @@ def test_make_decision_retrieves_waivers_on_errored(mock_results, mock_waivers,
     mock_waivers.return_value = []
     response = make_decision()
     assert 200 == response.status_code
-    assert '1 of 1 required tests failed (1 error)' == response.json['summary']
+    assert 'Of 1 required test(s), 1 test(s) errored' == response.json['summary']
     mock_waivers.assert_called_once()
 
 
@@ -110,7 +110,7 @@ def test_make_decision_retrieves_waivers_once_on_verbose_and_missing(
     mock_waivers.return_value = []
     response = make_decision(verbose=True)
     assert 200 == response.status_code
-    assert '1 of 1 required test results missing' == response.json['summary']
+    assert 'Of 1 required test(s), 1 result(s) missing' == response.json['summary']
     mock_waivers.assert_called_once()
 
 
@@ -128,7 +128,7 @@ def test_make_decision_with_no_tests_required(mock_results, mock_waivers, make_d
     """
     response = make_decision(policies=policies)
     assert 200 == response.status_code
-    assert 'no tests are required' == response.json['summary']
+    assert 'No tests are required' == response.json['summary']
     mock_waivers.assert_not_called()
 
 
@@ -152,7 +152,7 @@ def test_make_decision_with_no_tests_required_and_missing_gating_yaml(
             f.return_value = None
             response = make_decision(policies=policies)
             assert 200 == response.status_code
-            assert 'no tests are required' == response.json['summary']
+            assert 'No tests are required' == response.json['summary']
             mock_waivers.assert_not_called()
 
 
@@ -195,7 +195,7 @@ def test_make_decision_with_no_tests_required_and_empty_remote_rules(
             f.return_value = remote_fragment1
             response = make_decision(policies=policies)
             assert 200 == response.status_code
-            assert 'no tests are required' == response.json['summary']
+            assert 'No tests are required' == response.json['summary']
             mock_waivers.assert_not_called()
 
         with mock.patch('greenwave.resources.retrieve_yaml_remote_rule') as f:
@@ -227,7 +227,8 @@ def test_make_decision_with_missing_required_gating_yaml(mock_results, mock_waiv
             response = make_decision(policies=policies)
             assert 200 == response.status_code
             assert not response.json['policies_satisfied']
-            assert '1 of 1 required tests failed' == response.json['summary']
+            exp = '1 non-test-result unsatisfied requirement(s) (gating.yaml issues)'
+            assert exp == response.json['summary']
             mock_waivers.assert_called_once()
 
 
@@ -264,7 +265,7 @@ def test_make_decision_multiple_contexts(mock_results, mock_waivers, make_decisi
     """
     response = make_decision(policies=policies, decision_context=["test_policies", "test_2"])
     assert 200 == response.status_code
-    assert '2 of 2 required tests failed' == response.json['summary']
+    assert 'Of 2 required test(s), 2 test(s) failed' == response.json['summary']
     assert ['test_policy', 'test_policy_2'] == response.json['applicable_policies']
     mock_waivers.assert_called_once()
 

greenwave/tests/test_policies.py

@@ -83,17 +83,17 @@ def test_summarize_answers():
     testResultMissing = TestResultMissing(testSubject, 'test', None, None)
 
     assert summarize_answers([testResultPassed]) == \
-        'All required tests passed'
+        'All required tests passed or waived'
     assert summarize_answers([testResultFailed, testResultPassed]) == \
-        '1 of 2 required tests failed'
+        'Of 2 required test(s), 1 test(s) failed'
     assert summarize_answers([testResultMissing]) == \
-        '1 of 1 required test results missing'
+        'Of 1 required test(s), 1 result(s) missing'
     assert summarize_answers([testResultMissing, testResultFailed]) == \
-        '1 of 2 required tests failed, 1 result missing'
+        'Of 2 required test(s), 1 test(s) failed, 1 result(s) missing'
     assert summarize_answers([testResultMissing, testResultMissing, testResultFailed]) == \
-        '1 of 3 required tests failed, 2 results missing'
+        'Of 3 required test(s), 1 test(s) failed, 2 result(s) missing'
     assert summarize_answers([testResultMissing, testResultPassed]) == \
-        '1 of 2 required test results missing'
+        'Of 2 required test(s), 1 result(s) missing'
 
 
 def test_decision_with_missing_result(tmpdir):

greenwave/tests/test_summary.py

@@ -32,79 +32,80 @@ def test_summary_passed():
     answers = [
         testResultPassed,
     ]
-    assert summarize_answers(answers) == 'All required tests passed'
+    assert summarize_answers(answers) == 'All required tests passed or waived'
 
 
 def test_summary_empty():
     answers = []
-    assert summarize_answers(answers) == 'no tests are required'
+    assert summarize_answers(answers) == 'No tests are required'
 
 
 def test_summary_failed():
     answers = [
         testResultFailed,
     ]
-    assert summarize_answers(answers) == '1 of 1 required tests failed'
+    assert summarize_answers(answers) == 'Of 1 required test(s), 1 test(s) failed'
 
 
 def test_summary_incomplete():
     answers = [
         testResultIncomplete,
     ]
-    assert summarize_answers(answers) == '1 of 1 required test results missing'
+    assert summarize_answers(answers) == 'Of 1 required test(s), 1 test(s) incomplete'
 
 
 def test_summary_missing():
     answers = [
         testResultMissing,
     ]
-    assert summarize_answers(answers) == '1 of 1 required test results missing'
+    assert summarize_answers(answers) == 'Of 1 required test(s), 1 result(s) missing'
 
 
 def test_summary_missing_waived():
     answers = [
         TestResultWaived(testResultMissing, 123),
     ]
-    assert summarize_answers(answers) == 'All required tests passed'
+    assert summarize_answers(answers) == 'All required tests passed or waived'
 
 
 def test_summary_errored():
     answers = [
         testResultErrored,
     ]
-    assert summarize_answers(answers) == '1 of 1 required tests failed (1 error)'
+    assert summarize_answers(answers) == 'Of 1 required test(s), 1 test(s) errored'
 
 
 def test_summary_one_passed_one_failed():
     answers = [
         testResultPassed,
         testResultFailed,
     ]
-    assert summarize_answers(answers) == '1 of 2 required tests failed'
+    assert summarize_answers(answers) == 'Of 2 required test(s), 1 test(s) failed'
 
 
 def test_summary_one_passed_one_missing():
     answers = [
         testResultPassed,
         testResultMissing,
     ]
-    assert summarize_answers(answers) == '1 of 2 required test results missing'
+    assert summarize_answers(answers) == 'Of 2 required test(s), 1 result(s) missing'
 
 
 def test_summary_one_passed_one_missing_waived():
     answers = [
         testResultPassed,
         TestResultWaived(testResultMissing, 123),
     ]
-    assert summarize_answers(answers) == 'All required tests passed'
+    assert summarize_answers(answers) == 'All required tests passed or waived'
 
 
 def test_summary_one_failed_one_missing():
     answers = [
         testResultFailed,
         testResultMissing,
     ]
-    assert summarize_answers(answers) == '1 of 2 required tests failed, 1 result missing'
+    exp = 'Of 2 required test(s), 1 test(s) failed, 1 result(s) missing'
+    assert summarize_answers(answers) == exp
 
 
 def test_summary_one_passed_one_failed_one_missing():
@@ -113,7 +114,8 @@ def test_summary_one_passed_one_failed_one_missing():
         testResultFailed,
         testResultMissing,
     ]
-    assert summarize_answers(answers) == '1 of 3 required tests failed, 1 result missing'
+    exp = 'Of 3 required test(s), 1 test(s) failed, 1 result(s) missing'
+    assert summarize_answers(answers) == exp
 
 
 def test_summary_one_passed_one_failed_one_missing_two_errored():
@@ -124,14 +126,16 @@ def test_summary_one_passed_one_failed_one_missing_two_errored():
         testResultMissing,
         testResultErrored,
     ]
-    assert summarize_answers(answers) == '3 of 5 required tests failed, 1 result missing (2 errors)'
+    exp = 'Of 5 required test(s), 1 test(s) failed, 2 test(s) errored, 1 result(s) missing'
+    assert summarize_answers(answers) == exp
 
 
 def test_summary_invalid_gating_yaml():
     answers = [
         testInvalidGatingYaml,
     ]
-    assert summarize_answers(answers) == '1 of 1 required tests failed'
+    exp = '1 non-test-result unsatisfied requirement(s) (gating.yaml issues)'
+    assert summarize_answers(answers) == exp
 
 
 def test_summary_one_passed_one_invalid_gating_yaml_one_missing():
@@ -140,4 +144,6 @@ def test_summary_one_passed_one_invalid_gating_yaml_one_missing():
         testResultMissing,
         testInvalidGatingYaml,
     ]
-    assert summarize_answers(answers) == '1 of 3 required tests failed, 1 result missing'
+    exp = '1 non-test-result unsatisfied requirement(s) (gating.yaml issues). '
+    exp += 'Of 2 required test(s), 1 result(s) missing'
+    assert summarize_answers(answers) == exp