Report each failure against master and against the branch point

mod_ci/comparison.py

@@ -0,0 +1,143 @@
+"""Compare one test run's regression results against another run's.
+
+Pass and fail are decided elsewhere, and against one thing only: the approved
+output. That answers "is this correct?", which is the question a baseline is for
+-- but it is not the question a reviewer is asking. A reviewer wants to know
+what *this change* did, and a test that has been failing since last month tells
+them nothing while burying the one that started failing today.
+
+So the verdict stays absolute and the report is relative. The same failure is
+described against several references at once: the approved output, the tip of
+master, and the closest ancestor commit we still hold results for. A test
+failing identically on all of them is drift someone needs to approve; a test
+failing only here is the change under review.
+
+The functions below take plain values rather than models so the classification
+can be tested without a database, a GitHub client, or a CI run.
+"""
+
+from typing import Any, Dict, Iterable, List, NamedTuple, Optional, Tuple
+
+#: The test matched the approved output on both sides.
+UNCHANGED_PASS = 'unchanged_pass'
+#: Fails here, matched the approved output in the reference run.
+BROKEN_HERE = 'broken_here'
+#: Matches the approved output here, failed in the reference run.
+FIXED_HERE = 'fixed_here'
+#: Fails on both sides and produces the *same* bytes -- unchanged behaviour
+#: measured against a baseline that no longer describes it.
+FAILING_IDENTICALLY = 'failing_identically'
+#: Fails on both sides but the output differs, so something moved even though
+#: the verdict did not.
+FAILING_DIFFERENTLY = 'failing_differently'
+#: The reference run holds no record of this test, so nothing can be said.
+NO_REFERENCE = 'no_reference'
+
+#: Every verdict, in the order a reader should be shown them: what this change
+#: broke first, what it fixed next, then the pre-existing noise.
+VERDICTS = (BROKEN_HERE, FIXED_HERE, FAILING_DIFFERENTLY, FAILING_IDENTICALLY,
+            UNCHANGED_PASS, NO_REFERENCE)
+
+
+class TestState(NamedTuple):
+    """How one regression test behaved in one run.
+
+    ``signature`` identifies *how* a failing test differed, so two runs failing
+    the same test can be told apart by whether they produced the same bytes.
+    """
+
+    #: True when the exit code matched and every output file matched the approved one.
+    passed: bool
+    #: (output_id, produced hash) for each output that did not match, sorted.
+    signature: Tuple[Tuple[int, Optional[str]], ...]
+
+
+def build_state(test_results: Iterable[Any]) -> Dict[int, TestState]:
+    """
+    Summarise a run as one state per regression test.
+
+    Whether a test passed is taken from ``get_test_results``, which is the
+    platform's own verdict and already accounts for exit codes, outputs that are
+    absent when they should not be, and the alternative hashes an output may
+    legitimately produce. Re-deriving any of that here would mean two
+    definitions of "passed" that could drift apart.
+
+    The signature is built from the recorded hashes, which is the part
+    ``get_test_results`` does not express: it lets two runs failing the same
+    test be told apart by whether they produced the same bytes.
+
+    :param test_results: The structure returned by ``get_test_results``.
+    :type test_results: Iterable[Any]
+    :return: Regression test id mapped to how that test behaved.
+    :rtype: Dict[int, TestState]
+    """
+    states: Dict[int, TestState] = {}
+    for category in test_results:
+        for entry in category['tests']:
+            # A caller that reports no files leaves the failure unexplained rather
+            # than unnoticed: the verdict still counts, only the signature is empty.
+            failed_outputs: List[Tuple[int, Optional[str]]] = sorted(
+                (result_file.regression_test_output_id, result_file.got)
+                for result_file in (entry.get('files') or ()) if result_file.got is not None)
+            states[entry['test'].id] = TestState(passed=not entry['error'],
+                                                 signature=tuple(failed_outputs))
+    return states
+
+
+def classify(current: TestState, reference: Optional[TestState]) -> str:
+    """
+    Describe one test's behaviour here relative to a reference run.
+
+    :param current: How the test behaved in the run being reported on.
+    :type current: TestState
+    :param reference: How it behaved in the reference run, if that run has a record.
+    :type reference: Optional[TestState]
+    :return: One of the module's verdict constants.
+    :rtype: str
+    """
+    if reference is None:
+        return NO_REFERENCE
+    if current.passed and reference.passed:
+        return UNCHANGED_PASS
+    if current.passed:
+        return FIXED_HERE
+    if reference.passed:
+        return BROKEN_HERE
+    if current.signature == reference.signature:
+        return FAILING_IDENTICALLY
+    return FAILING_DIFFERENTLY
+
+
+def compare(current: Dict[int, TestState],
+            reference: Optional[Dict[int, TestState]]) -> Dict[str, List[int]]:
+    """
+    Bucket every regression test in a run by how it compares to a reference run.
+
+    A missing reference run is not the same as a reference run that passed
+    everything: it is reported as ``no_reference`` so the comment can say "we
+    have no records to compare against" instead of implying good news.
+
+    :param current: States for the run being reported on.
+    :type current: Dict[int, TestState]
+    :param reference: States for the reference run, or None when there is no such run.
+    :type reference: Optional[Dict[int, TestState]]
+    :return: Verdict mapped to the regression test ids in it.
+    :rtype: Dict[str, List[int]]
+    """
+    buckets: Dict[str, List[int]] = {verdict: [] for verdict in VERDICTS}
+    for rt_id in sorted(current):
+        reference_state = None if reference is None else reference.get(rt_id)
+        buckets[classify(current[rt_id], reference_state)].append(rt_id)
+    return buckets
+
+
+def summarise(buckets: Dict[str, List[int]]) -> Dict[str, int]:
+    """
+    Count each verdict, for a table that has to stay short.
+
+    :param buckets: Output of :func:`compare`.
+    :type buckets: Dict[str, List[int]]
+    :return: Verdict mapped to how many tests fell in it.
+    :rtype: Dict[str, int]
+    """
+    return {verdict: len(ids) for verdict, ids in buckets.items()}

mod_ci/controllers.py

@@ -4,6 +4,7 @@
 import datetime
 import fnmatch
 import hashlib
+import itertools
 import json
 import os
 import re
@@ -14,7 +15,7 @@
 from collections import defaultdict
 from functools import wraps
 from pathlib import Path
-from typing import Any, Callable, Dict, Optional, TypeVar
+from typing import Any, Callable, Dict, List, Optional, TypeVar
 
 import googleapiclient.discovery
 import requests
@@ -35,10 +36,11 @@
 from decorators import get_menu_entries, template_renderer
 from mod_auth.controllers import check_access_rights, login_required
 from mod_auth.models import Role
+from mod_ci import comparison
 from mod_ci.forms import AddUsersToBlacklist, DeleteUserForm
 from mod_ci.models import (BlockedUsers, CategoryTestInfo, GcpInstance,
                            MaintenanceMode, PendingDeletion, PrCommentInfo,
-                           Status)
+                           ReferenceComparison, Status)
 from mod_customized.models import CustomizedTest
 from mod_home.models import CCExtractorVersion, GeneralData
 from mod_regression.models import (Category, RegressionTest,
@@ -53,6 +55,10 @@
 GITHUB_API_TIMEOUT = 30  # Timeout for GitHub API calls
 GCP_API_TIMEOUT = 60  # Timeout for GCP API calls
 ARTIFACT_DOWNLOAD_TIMEOUT = 300  # 5 minutes for artifact downloads
+
+#: How far back to walk a branch's history looking for a run to compare against.
+#: Deep enough to clear a stale branch, short enough to stay one API page.
+ANCESTOR_SEARCH_DEPTH = 50
 GCP_OPERATION_MAX_WAIT = 1800  # 30 minutes max wait for GCP operations
 GCP_VM_CREATE_VERIFY_TIMEOUT = 60  # 60 seconds to verify VM creation started
 
@@ -2835,43 +2841,141 @@
     safe_db_commit(g.db, f"updating average {process_type} time for {platform.value}")
 
 
-def get_info_for_pr_comment(test: Test) -> PrCommentInfo:
+def find_ancestor_run(repository, test: Test) -> Optional[Test]:
     """
-    Return info about the given test for use in a PR comment.
+    Find the newest completed run for a commit this one descends from.
 
-    :param test: The test whose report will be returned
+    The tip of master is not always what a branch was cut from, so a comparison
+    against it charges the branch for whatever master did in between. Walking
+    back from the branch's own base answers the narrower question a reviewer is
+    asking: what changed *here*.
+
+    Any GitHub failure resolves to None rather than raising -- a comment missing
+    one of its comparisons is worth more than no comment at all.
+
+    :param repository: GitHub repository handle used to walk the commit history.
+    :type repository: Repository.Repository
+    :param test: The run whose ancestry should be searched.
     :type test: Test
+    :return: The closest ancestor's completed run on the same platform, if any.
+    :rtype: Optional[Test]
     """
-    last_test_master = g.db.query(Test).filter(Test.branch == "master", Test.test_type == TestType.commit,
-                                               Test.platform == test.platform).join(
+    from run import log
+
+    if repository is None:
+        return None
+    try:
+        if test.pr_nr:
+            start = repository.get_pull(number=test.pr_nr).base.sha
+        else:
+            parents = repository.get_commit(test.commit).parents
+            if not parents:
+                return None
+            start = parents[0].sha
+        ancestry = [commit.sha for commit in
+                    itertools.islice(repository.get_commits(sha=start), ANCESTOR_SEARCH_DEPTH)]
+    except Exception as error:
+        log.warning(f"Could not resolve ancestry for test {test.id}: {type(error).__name__}: {error}")
+        return None
+
+    if not ancestry:
+        return None
+
+    runs = g.db.query(Test).filter(and_(Test.commit.in_(ancestry),
+                                        Test.platform == test.platform,
+                                        Test.id != test.id)).join(
         TestProgress, Test.id == TestProgress.test_id).filter(
-            TestProgress.status == TestStatus.completed).order_by(TestProgress.id.desc()).first()
+            TestProgress.status == TestStatus.completed).order_by(TestProgress.id.desc()).all()
+
+    newest_per_commit: Dict[str, Test] = {}
+    for run in runs:
+        newest_per_commit.setdefault(run.commit, run)
+    # Nearest ancestor first: ancestry is already in walk order.
+    for sha in ancestry:
+        if sha in newest_per_commit:
+            return newest_per_commit[sha]
+    return None
 
-    extra_failed_tests = []
-    common_failed_tests = []
-    fixed_tests = []
-    category_stats = []
 
+def _compare_against(label: str, reference: Optional[Test], current: Dict[int, comparison.TestState],
+                     regression_tests: Dict[int, RegressionTest],
+                     already_used: Dict[Any, str]) -> ReferenceComparison:
+    """
+    Describe this run's results against one reference run.
+
+    :param label: How the reference should be named to a reader.
+    :type label: str
+    :param reference: The run to compare against, or None when there is none.
+    :type reference: Optional[Test]
+    :param current: States for the run being reported on.
+    :type current: Dict[int, comparison.TestState]
+    :param regression_tests: Regression tests by id, for rendering the buckets.
+    :type regression_tests: Dict[int, RegressionTest]
+    :param already_used: Run ids already compared against, mapped to their label.
+    :type already_used: Dict[Any, str]
+    :return: The comparison, empty when there was nothing to compare against.
+    :rtype: ReferenceComparison
+    """
+    if reference is None:
+        empty: Dict[str, List[RegressionTest]] = {verdict: [] for verdict in comparison.VERDICTS}
+        return ReferenceComparison(label, None, empty, {verdict: 0 for verdict in comparison.VERDICTS})
+
+    duplicate_of = already_used.get(reference.id)
+    if duplicate_of is None:
+        already_used[reference.id] = label
+
+    buckets = comparison.compare(current, comparison.build_state(get_test_results(reference)))
+    tests = {verdict: [regression_tests[rt_id] for rt_id in ids if rt_id in regression_tests]
+             for verdict, ids in buckets.items()}
+    return ReferenceComparison(label, reference, tests, comparison.summarise(buckets), duplicate_of)
+
+
+def get_info_for_pr_comment(test: Test, repository=None) -> PrCommentInfo:
+    """
+    Return info about the given test for use in a PR comment.
+
+    Pass and fail are decided against the approved output and nothing else. The
+    comparisons that follow do not change any verdict; they say what each
+    failure means relative to master and to the commit the branch was cut from,
+    which is what separates "this change broke it" from "it has been failing for
+    a month".
+
+    :param test: The test whose report will be returned
+    :type test: Test
+    :param repository: GitHub repository handle, needed to resolve the ancestor.
+    :type repository: Optional[Repository.Repository]
+    """
     test_results = get_test_results(test)
-    platform_column = f"last_passed_on_{test.platform.value}"
+    current = comparison.build_state(test_results)
+
+    category_stats = []
+    failed_tests = []
+    regression_tests: Dict[int, RegressionTest] = {}
     for category_results in test_results:
-        category_name = category_results['category'].name
-
-        category_test_pass_count = 0
-        for test in category_results['tests']:
-            if not test['error']:
-                category_test_pass_count += 1
-                if last_test_master and getattr(test['test'], platform_column) != last_test_master.id:
-                    fixed_tests.append(test['test'])
+        passed_in_category = 0
+        for entry in category_results['tests']:
+            regression_tests[entry['test'].id] = entry['test']
+            if entry['error']:
+                failed_tests.append(entry['test'])
             else:
-                if last_test_master and getattr(test['test'], platform_column) != last_test_master.id:
-                    common_failed_tests.append(test['test'])
-                else:
-                    extra_failed_tests.append(test['test'])
+                passed_in_category += 1
+        category_stats.append(CategoryTestInfo(category_results['category'].name,
+                                               len(category_results['tests']), passed_in_category))
+
+    last_test_master = g.db.query(Test).filter(Test.branch == "master", Test.test_type == TestType.commit,
+                                               Test.platform == test.platform).join(
+        TestProgress, Test.id == TestProgress.test_id).filter(
+            TestProgress.status == TestStatus.completed).order_by(TestProgress.id.desc()).first()
 
-        category_stats.append(CategoryTestInfo(category_name, len(category_results['tests']), category_test_pass_count))
+    already_used: Dict[Any, str] = {}
+    comparisons = [
+        _compare_against('the tip of master', last_test_master, current, regression_tests, already_used),
+        _compare_against('the commit this branch was cut from', find_ancestor_run(repository, test),
+                         current, regression_tests, already_used),
+    ]
 
-    return PrCommentInfo(category_stats, extra_failed_tests, fixed_tests, common_failed_tests, last_test_master)
+    return PrCommentInfo(category_stats, failed_tests, len(current) - len(failed_tests),
+                         len(current), comparisons, last_test_master)
 
 
 def comment_pr(test: Test) -> str:
@@ -2885,16 +2989,28 @@
 
     test_id = test.id
     platform = test.platform.name
-    comment_info = get_info_for_pr_comment(test)
-    template = app.jinja_env.get_or_select_template('ci/pr_comment.txt')
-    message = template.render(comment_info=comment_info, test_id=test_id, platform=platform)
-    log.debug(f"GitHub PR Comment Message Created for Test_id: {test_id}")
     if not g.github['bot_token']:
         log.error(f"GitHub token not configured, cannot post PR comment for Test_id: {test_id}")
         return Status.FAILURE
+
+    # Resolved before the report is built, because working out which commit this
+    # branch was cut from needs the repository. A failure here costs that one
+    # comparison; the comment is still worth posting without it.
+    gh = None
+    repository = None
     try:
         gh = Github(auth=Auth.Token(g.github['bot_token']))
         repository = gh.get_repo(f"{g.github['repository_owner']}/{g.github['repository']}")
+    except Exception as e:
+        log.error(f"Could not reach GitHub for Test_id: {test_id} with Exception {e}")
+
+    comment_info = get_info_for_pr_comment(test, repository)
+    template = app.jinja_env.get_or_select_template('ci/pr_comment.txt')
+    message = template.render(comment_info=comment_info, test_id=test_id, platform=platform)
+    log.debug(f"GitHub PR Comment Message Created for Test_id: {test_id}")
+    try:
+        if repository is None or gh is None:
+            raise RuntimeError('no GitHub repository handle')
         # Pull requests are just issues with code, so GitHub considers PR comments in issues
         pull_request = repository.get_pull(number=test.pr_nr)
         comments = pull_request.get_issue_comments()
@@ -2907,7 +3023,11 @@
         log.debug(f"GitHub PR Comment ID {comment.id} Uploaded for Test_id: {test_id}")
     except Exception as e:
         log.error(f"GitHub PR Comment Failed for Test_id: {test_id} with Exception {e}")
-    return Status.SUCCESS if len(comment_info.extra_failed_tests) == 0 else Status.FAILURE
+    # The verdict is whether the output matched what was approved, and nothing
+    # else. The comparisons in the comment explain a failure; they never excuse
+    # one, because a baseline that no longer matches reality is a thing to fix
+    # rather than a thing to pass.
+    return Status.SUCCESS if len(comment_info.failed_tests) == 0 else Status.FAILURE
 
 
 @mod_ci.route('/show_maintenance')