[FIX] runbot_merge: avoid repeatedly warning about the same failures

The mergebot has a feature to ping users when an approved PR or
forward-port suffers from a CI failure, as those PRs might be somewhat
unattended (so the author needs to be warned explicitly).

Because the runbot can send the same failure information multiple
times, the mergebot also has a *deduplication* feature, however this
deduplication feature was too weak to handle the case where the PR has
2+ failures e.g. ci and linting as it only stores the last-seen
failure, and there would be two different failures here.

Worse, because the validation step looks at all required statuses, in
that case it would send a failure ping message for each failed
status *on each inbound status*: first it'd notify about the ci
failure and store that, then it'd see the linting failure, check
against the previous (ci), consider it a new failure, notify, and
store that. Rinse and repeat every time runbot sends a ci *or* lint
failure, leading to a lot of dumb and useless spam.

Fix by storing the entire current failure state (a map of context:
status) instead of just the last-seen status data.

Note: includes a backwards-compatibility shim where we just convert a
stored status into a full `{context: status}` map. This uses the
"current context" because we don't have the original, but if it was a
different context it's not going to match anyway (the target_url
should be different) and if it was the same context then there's a
chance we skip sending a redundant notification.

Fixes #435

runbot_merge/models/pull_requests.py

@@ -1113,11 +1113,12 @@ class PullRequests(models.Model):
         return failed
 
     def _notify_ci_new_failure(self, ci, st):
-        # only sending notification if the newly failed status is different than
-        # the old one
         prev = json.loads(self.previous_failure)
-        if not self._statuses_equivalent(st, prev):
-            self.previous_failure = json.dumps(st)
+        if prev.get('state'): # old-style previous-failure
+            prev = {ci: prev}
+        if not any(self._statuses_equivalent(st, v) for v in prev.values()):
+            prev[ci] = st
+            self.previous_failure = json.dumps(prev)
             self._notify_ci_failed(ci)
 
     def _statuses_equivalent(self, a, b):

runbot_merge/tests/test_basic.py

@@ -940,17 +940,24 @@ def test_ci_failure_after_review(env, repo, users, config):
         prx.post_comment('hansen r+', config['role_reviewer']['token'])
     env.run_crons()
 
-    with repo:
-        repo.post_status(prx.head, 'failure', 'ci/runbot', target_url="https://a")
-        repo.post_status(prx.head, 'failure', 'legal/cla', target_url="https://b")
-        repo.post_status(prx.head, 'failure', 'foo/bar', target_url="https://c")
-    env.run_crons()
+    for ctx, url in [
+        ('ci/runbot', 'https://a'),
+        ('ci/runbot', 'https://a'),
+        ('legal/cla', 'https://b'),
+        ('foo/bar', 'https://c'),
+        ('ci/runbot', 'https://a'),
+        ('legal/cla', 'https://d'), # url changes so different from the previous
+    ]:
+        with repo:
+            repo.post_status(prx.head, 'failure', ctx, target_url=url)
+        env.run_crons()
 
     assert prx.comments == [
         (users['reviewer'], 'hansen r+'),
         seen(env, prx, users),
-        (users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)),
         (users['user'], "'ci/runbot' failed on this reviewed PR.".format_map(users)),
+        (users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)),
+        (users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)),
     ]
 
 def test_reopen_merged_pr(env, repo, config, users):