Rewrite summarize_answers to be more accurate #169

AdamWill · 2023-06-06T00:31:38Z

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).

AdamWill · 2023-06-06T00:32:26Z

Note: I think there may be a concern that users may be parsing the summary and might be thrown off by changes in it. I know Bodhi used to do this, but it no longer does and hasn't for a while (I changed that behaviour myself). I'm not sure if any RH-internal users do it, though.

hluk

The summaries in docs need to be updated.

I hope this won't break any usage - "summary" is meant to be read by users or displayed in UIs but not parsed. (Actually, it breaks our integration tests for gating, but that can be easily fixed.)

Well, the exact format of the summary is not documented anyway. 🤷

(BTW, the CI here is currently failing because of some recent breaking changes in waiverdb. We are working on a fix.)

greenwave/policies.py

AdamWill · 2023-06-06T18:15:47Z

Updated the summaries in docs and implemented the other suggestion.

AdamWill · 2023-06-06T18:18:59Z

btw, is greenwave's minimum supported python version high enough that we can switch to f"" format strings? I've seen enough curly braces for this lifetime...:D

hluk · 2023-06-07T13:33:14Z

btw, is greenwave's minimum supported python version high enough that we can switch to f"" format strings? I've seen enough curly braces for this lifetime...:D

Yes, definitely! Python 3.9 is what the container image uses.

hluk · 2023-06-20T09:28:57Z

There are two failed tests currently:

  ____________________ test_make_a_decision_on_queued_result _____________________
  
  requests_session = <requests.sessions.Session object at 0x7f30a5bf2220>
  greenwave_server = 'http://localhost:8080/'
  testdatabuilder = <conftest.TestDataBuilder object at 0x7f30a5c6edc0>
  
      def test_make_a_decision_on_queued_result(requests_session, greenwave_server, testdatabuilder):
          nvr = testdatabuilder.unique_nvr()
          result = testdatabuilder.create_result(item=nvr,
                                                 testcase_name=TASKTRON_RELEASE_CRITICAL_TASKS[0],
                                                 outcome='QUEUED')
          data = {
              'decision_context': 'bodhi_update_push_stable',
              'product_version': 'fedora-26',
              'subject_type': 'koji_build',
              'subject_identifier': nvr,
          }
          r = requests_session.post(greenwave_server + 'api/v1.0/decision', json=data)
          assert r.status_code == 200
          res_data = r.json()
          assert res_data['policies_satisfied'] is False
          assert 'taskotron_release_critical_tasks' in res_data['applicable_policies']
          assert 'taskotron_release_critical_tasks_with_blocklist' in res_data['applicable_policies']
          expected_summary = 'Of 3 required test(s), 3 result(s) missing'
  >       assert res_data['summary'] == expected_summary
  E       AssertionError: assert 'Of 3 require...lt(s) missing' == 'Of 3 require...lt(s) missing'
  E         - Of 3 required test(s), 3 result(s) missing
  E         ?                        ^
  E         + Of 3 required test(s), 1 test(s) incomplete, 2 result(s) missing
  E         ?                        ^^^^^^^^^^^^^^^^^^^^^^^
  
  functional-tests/test_api_v1.py:441: AssertionError
  ____________________ test_make_a_decision_on_running_result ____________________
  
  requests_session = <requests.sessions.Session object at 0x7f30a5bf2220>
  greenwave_server = 'http://localhost:8080/'
  testdatabuilder = <conftest.TestDataBuilder object at 0x7f30a5c6edc0>
  
      def test_make_a_decision_on_running_result(requests_session, greenwave_server, testdatabuilder):
          nvr = testdatabuilder.unique_nvr()
          result = testdatabuilder.create_result(item=nvr,
                                                 testcase_name=TASKTRON_RELEASE_CRITICAL_TASKS[0],
                                                 outcome='RUNNING')
          data = {
              'decision_context': 'bodhi_update_push_stable',
              'product_version': 'fedora-26',
              'subject_type': 'koji_build',
              'subject_identifier': nvr,
          }
          r = requests_session.post(greenwave_server + 'api/v1.0/decision', json=data)
          assert r.status_code == 200
          res_data = r.json()
          assert res_data['policies_satisfied'] is False
          assert 'taskotron_release_critical_tasks' in res_data['applicable_policies']
          assert 'taskotron_release_critical_tasks_with_blocklist' in res_data['applicable_policies']
          expected_summary = 'Of 3 required test(s), 3 result(s) missing'
  >       assert res_data['summary'] == expected_summary
  E       AssertionError: assert 'Of 3 require...lt(s) missing' == 'Of 3 require...lt(s) missing'
  E         - Of 3 required test(s), 3 result(s) missing
  E         ?                        ^
  E         + Of 3 required test(s), 1 test(s) incomplete, 2 result(s) missing

AdamWill · 2023-06-20T09:50:54Z

Oh, sorry, I thought I'd caught all of those. Will fix.

AdamWill · 2023-06-20T10:11:47Z

OK, in both cases there the behaviour is correct and I got the test expectation wrong, so I just fixed the test expectations (and rebased). Should be good now.

mvalik · 2023-06-26T22:45:24Z

greenwave/policies.py

+        if self.non_test_msgs:
+            msgstr = ", ".join("{} {}".format(v, k) for (k, v) in self.non_test_msgs.items())
+        if self.test_msgs:
+            addmsg = "Of {} required test(s), ".format(test_count)


"Of {} required test{}, ".format(test_count, "s" * (test_count != 1))

mvalik · 2023-06-26T22:53:37Z

greenwave/policies.py


 class TestResultMissing(RuleNotSatisfied):
    """
    A required test case is missing (that is, we did not find any result in
    ResultsDB with a matching item and test case name).
    """

+    summary_text = "result(s) missing"


{num} result{s} missing

mvalik · 2023-06-26T22:54:12Z

greenwave/policies.py

@@ -186,6 +196,8 @@ class TestResultIncomplete(RuleNotSatisfied):
    result outcomes in ResultsDB with a matching item and test case name).
    """

+    summary_text = "test(s) incomplete"


{num} test{s} incomplete

mvalik · 2023-06-26T22:54:36Z

greenwave/policies.py

@@ -245,6 +257,8 @@ class TestResultFailed(RuleNotSatisfied):
    not passing).
    """

+    summary_text = "test(s) failed"


{num} test{s} failed

mvalik · 2023-06-26T22:55:06Z

greenwave/policies.py

@@ -284,6 +298,8 @@ class TestResultErrored(RuleNotSatisfied):
    was an error).
    """

+    summary_text = "test(s) errored"


{num} test{s} errored

mvalik · 2023-06-26T22:55:45Z

greenwave/policies.py

@@ -331,6 +347,8 @@ class InvalidRemoteRuleYaml(RuleNotSatisfied):
    """

    scenario = None
+    is_test_result = False
+    summary_text = "non-test-result unsatisfied requirement(s) (gating.yaml issues)"


{num} non-test-result unsatisfied requirement{s} (invalid gating.yaml)

mvalik · 2023-06-26T22:58:40Z

greenwave/policies.py

    test_case_name = 'failed-fetch-gating-yaml'
+    scenario = None
+    is_test_result = False
+    summary_text = "non-test-result unsatisfied requirement(s) (gating.yaml issues)"


{num} non-test-result unsatisfied requirement{s} (failed to fetch the gating.yaml)

mvalik · 2023-06-26T22:59:07Z

greenwave/policies.py

@@ -140,6 +141,7 @@ class RuleNotSatisfied(Answer):
    """

    is_satisfied = False
+    summary_text = "unexpected unsatisfied requirement(s)"


{num} unexpected unsatisfied requirement{s}

mvalik · 2023-06-26T23:01:19Z

greenwave/policies.py

+            msgstr = ", ".join("{} {}".format(v, k) for (k, v) in self.non_test_msgs.items())
+        if self.test_msgs:
+            addmsg = "Of {} required test(s), ".format(test_count)
+            addmsg += ", ".join("{} {}".format(v, k) for (k, v) in self.test_msgs.items())


addmsg += ", ".join(k.format(num=v, s="s" * (v > 1)) for (k, v) in self.test_msgs.items())

mvalik · 2023-06-26T23:04:17Z

greenwave/policies.py

+    def to_text(self, test_count):
+        msgstr = ""
+        if self.non_test_msgs:
+            msgstr = ", ".join("{} {}".format(v, k) for (k, v) in self.non_test_msgs.items())


msgstr = ", ".join(k.format(num=v, s="s" * (v > 1)) for (k, v) in self.non_test_msgs.items())

mvalik · 2023-06-26T23:09:43Z

greenwave/policies.py

+        if self.test_msgs:
+            addmsg = "Of {} required test(s), ".format(test_count)
+            addmsg += ", ".join("{} {}".format(v, k) for (k, v) in self.test_msgs.items())
+            if msgstr:


msgstr = (msgstr + ". ") * bool(msgstr) + addmsg

hluk · 2023-07-24T10:22:17Z

Any update? I'm OK with keeping the (s) in the messages for any count (though without it the messages may be a bit clearer).

AdamWill · 2023-07-24T15:02:47Z

oh, sorry, this fell out of my list of things to do...will try and get to it soon.

AdamWill · 2023-07-24T15:03:39Z

@hluk what do you think of the cuteness required to detect whether to add the s in some cases? It's clever, but I don't like being overly clever when it's not really necessary...

hluk · 2023-07-25T14:39:26Z

@hluk what do you think of the cuteness required to detect whether to add the s in some cases? It's clever, but I don't like being overly clever when it's not really necessary...

I'm OK with keeping the (s) in messages to indicate both single and multiple items.

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>

AdamWill · 2023-08-11T20:48:35Z

OK, well, then, there's not really a lot to change, then. The only other thing @mvalik suggests is to rejig slightly how to handle the number of each message (put it in the message template), but...that doesn't really seem like an obvious improvement to me.

I tweaked it to use f-strings instead of .format, and use clearer variable names in the to_text() method. I don't really see anything else to do, if we're not going to do the cleverness about adding the "s" or not.

AdamWill · 2023-08-11T20:49:27Z

Of course, we could merge this and @mvalik could send their proposed tweaks as a follow-up PR, if they want?

hluk · 2023-08-14T09:56:13Z

@mvalik Any thoughts?

BTW, I would like to avoid changing this too often since various tests in our internal infrastructure verify the summary and I'm not sure if anyone else depends on the old format.

mvalik · 2023-08-14T12:02:06Z

I think adding the -s is quite easy, but improves the user experience. If you don't like the way "s" * (num != 1), maybe a better solution will be to define a simple function:

def s(num: int) -> str:
    return "s" if num != 1 else ""

so you can use it as f"{num} test{s(num)} failed"

mvalik · 2023-08-14T12:07:25Z

Also I mentioned, that I don't like an abstract message "gating.yaml issues". IMO it is much better to mention a particular problem, so the user could know what went wrong and how to deal with it.

hluk · 2023-08-30T07:23:11Z

I have marked this as Draft so it does not pop up in our open pull request notifications all the time.

I will add a task in out backlog to see if we can improve the messages further and fix our tests (hopefully in the next sprint).

mvalik · 2024-03-07T15:35:59Z

Closing this PR, since changes was merged in the another one.

AdamWill force-pushed the fix-summarize branch from 7071fac to 37be41b Compare June 6, 2023 06:03

hluk reviewed Jun 6, 2023

View reviewed changes

greenwave/policies.py Show resolved Hide resolved

AdamWill force-pushed the fix-summarize branch from 37be41b to 114d099 Compare June 6, 2023 18:14

AdamWill force-pushed the fix-summarize branch from 114d099 to d0274a8 Compare June 20, 2023 10:11

mvalik reviewed Jun 26, 2023

View reviewed changes

AdamWill force-pushed the fix-summarize branch from d0274a8 to f01b55f Compare August 11, 2023 20:46

hluk marked this pull request as draft August 30, 2023 07:20

mvalik closed this Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite summarize_answers to be more accurate #169

Rewrite summarize_answers to be more accurate #169

AdamWill commented Jun 6, 2023

AdamWill commented Jun 6, 2023

hluk left a comment

AdamWill commented Jun 6, 2023

AdamWill commented Jun 6, 2023

hluk commented Jun 7, 2023

hluk commented Jun 20, 2023

AdamWill commented Jun 20, 2023

AdamWill commented Jun 20, 2023

mvalik Jun 26, 2023

mvalik Jun 26, 2023

mvalik Jun 26, 2023

mvalik Jun 26, 2023

mvalik Jun 26, 2023

mvalik Jun 26, 2023

mvalik Jun 26, 2023

mvalik Jun 26, 2023

mvalik Jun 26, 2023 •

edited

Loading

mvalik Jun 26, 2023 •

edited

Loading

mvalik Jun 26, 2023

hluk commented Jul 24, 2023

AdamWill commented Jul 24, 2023

AdamWill commented Jul 24, 2023

hluk commented Jul 25, 2023

AdamWill commented Aug 11, 2023

AdamWill commented Aug 11, 2023

hluk commented Aug 14, 2023

mvalik commented Aug 14, 2023

mvalik commented Aug 14, 2023

hluk commented Aug 30, 2023

mvalik commented Mar 7, 2024

Rewrite summarize_answers to be more accurate #169

Rewrite summarize_answers to be more accurate #169

Conversation

AdamWill commented Jun 6, 2023

AdamWill commented Jun 6, 2023

hluk left a comment

Choose a reason for hiding this comment

AdamWill commented Jun 6, 2023

AdamWill commented Jun 6, 2023

hluk commented Jun 7, 2023

hluk commented Jun 20, 2023

AdamWill commented Jun 20, 2023

AdamWill commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvalik Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

mvalik Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hluk commented Jul 24, 2023

AdamWill commented Jul 24, 2023

AdamWill commented Jul 24, 2023

hluk commented Jul 25, 2023

AdamWill commented Aug 11, 2023

AdamWill commented Aug 11, 2023

hluk commented Aug 14, 2023

mvalik commented Aug 14, 2023

mvalik commented Aug 14, 2023

hluk commented Aug 30, 2023

mvalik commented Mar 7, 2024

mvalik Jun 26, 2023 •

edited

Loading

mvalik Jun 26, 2023 •

edited

Loading