#145 Adjust "no test results found" text.
Closed: Fixed Opened by ralph.

As in #141, it is a bit misleading.

A patch like this (WiP) could do it:

diff --git a/greenwave/policies.py b/greenwave/policies.py
index af8de2d..cdd0903 100644
--- a/greenwave/policies.py
+++ b/greenwave/policies.py
@@ -95,16 +95,23 @@ def summarize_answers(answers):
     """
     if len(answers) == 0:
     return 'no tests are required'
+
     if all(answer.is_satisfied for answer in answers):
     return 'all required tests passed'
+
     failure_count = len([answer for answer in answers if isinstance(answer, TestResultFailed)])
-    if failure_count:
-        return ('{} of {} required tests failed'.format(failure_count, len(answers)))
     missing_count = len([answer for answer in answers if isinstance(answer, TestResultMissing)])
-    if missing_count == len(answers):
-        return 'no test results found'
+
+    if failure_count and missing_count:
+        return ('Of {} required tests, {} failed and {} are missing'.format(
+            len(answers), failure_count, missing_count))
+    elif failure_count:
+        return ('Of {} required tests, {} failed'.format(
+            len(answers), failure_count))
     elif missing_count:
-        return '{} of {} required tests not found'.format(missing_count, len(answers))
+        return ('Of {} required tests, {} are missing'.format(
+            len(answers), missing_count))
+
     return 'inexplicable result'
diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py
index cbe5493..4b68581 100644
--- a/greenwave/tests/test_policies.py
+++ b/greenwave/tests/test_policies.py
@@ -19,14 +19,14 @@ def test_summarize_answers():
     assert summarize_answers([RuleSatisfied()]) == \
     'all required tests passed'
     assert summarize_answers([TestResultFailed('item', 'test', None, 'id'), RuleSatisfied()]) == \
-        '1 of 2 required tests failed'
+        'Of 2 required tests, 1 failed'
     assert summarize_answers([TestResultMissing('item', 'test', None)]) == \
-        'no test results found'
+        'Of 1 required tests, 1 are missing'
     assert summarize_answers([TestResultMissing('item', 'test', None),
                   TestResultFailed('item', 'test', None, 'id')]) == \
-        '1 of 2 required tests failed'
+        'Of 2 required tests, 1 failed and 1 are missing'
     assert summarize_answers([TestResultMissing('item', 'test', None), RuleSatisfied()]) == \
-        '1 of 2 required tests not found'
+        'Of 2 required tests, 1 are missing'
 def test_waive_absence_of_result(tmpdir):

Yeah this looks like a nice improvement.

If you'll permit some bikeshedding though...

The phrase starting with "Of" looks a bit odd to me though. Also, the other summaries start with a lowercase letter (I guess since they are not quite complete sentences right now, lacking a verb) so for consistency these should continue to use a lowercase letter at the start too. Although that makes it look even stranger.

I think I kind of prefer the key fact being at the start too (which is, how many are missing or failed, rather than the total). What about:

"1 of 5 required tests failed"

"1 of 5 required test results missing"

"1 of 5 required tests failed, 2 results missing"

some bikeshedding

Oh yeah, paint that shed!

Your proposal looks great. Let's go with that.

I got stuck submitting a real PR here because the functional test suite is broken for me and I didn't have cycles to dig in atm.

Oh hm... I keep forgetting the Greenwave Jenkins job doesn't run the functional tests :grimacing:

Ahh okay. I think I got it... The Greenwave functional tests have been broken since we stopped using SQLite in Waiverdb (specifically the waiverdb.app.init_db function is gone)... Yikes. PR coming up.

I think we really need to stop with this idea of running all the servers as separate threads hosted inside the pytest process. They really should be subprocesses (or, in Jenkins, pointed at a real Openshift deployment the way we do for Waiverdb).

I have a patch for that, but it breaks the fedmsg consumer tests which want to interact with the same cache backend as the application is using. We might need a different approach for those tests, I'm not sure... or reclassify them as unit tests.

I'm working on it.

See PR#149 for fixing the functional tests.

Metadata Update from @dcallagh:
- Issue assigned to dcallagh

See PR#151 for the wording changes discussed here.

Metadata Update from @dcallagh:
- Issue set to the milestone: 0.7

Metadata Update from @lholecek:
- Issue close_status updated to: Fixed
- Issue status updated to: Closed (was: Open)
- Issue tagged with: enhancement

Metadata