From 2e7ddbb315acfbeccef81fe67061fe901f746e71 Mon Sep 17 00:00:00 2001 From: Carlos Fernandez Date: Sat, 15 Aug 2026 18:32:38 -0700 Subject: [PATCH] feat(ci): report each failure against master and against the branch point A verdict says a test does not match the approved output. It does not say who made that true, and for a reviewer that is the only interesting part. The comment answered it by asking whether the test last passed in the newest master run, which charges a branch for everything master did while the branch was open -- a three-line change to file_functions.c was reported as breaking 45 tests it never touched. Each failure is now described against two references as well as the approved output: the tip of master, and the newest completed run for the closest ancestor commit the branch actually descends from. Where both sides fail, the recorded hashes separate "fails identically" -- unchanged behaviour measured against a baseline that has gone stale -- from "fails differently", where something moved even though the verdict did not. Pass and fail are untouched and still decided against the approved output alone. A comparison explains a failure; it never excuses one, because a baseline that stopped describing reality is a thing to fix rather than a thing to pass. A reference we have no run for is reported as such rather than as agreement. The verdict per test is taken from get_test_results, which already accounts for exit codes, absent outputs, and the alternative hashes an output may legitimately produce. Deciding that again here would have created a second definition of "passed", free to drift from the first. --- mod_ci/comparison.py | 143 +++++++++++++++++++++++ mod_ci/controllers.py | 184 ++++++++++++++++++++++++------ mod_ci/models.py | 33 +++++- templates/ci/pr_comment.txt | 90 +++++++++------ tests/test_ci/test_comparison.py | 161 ++++++++++++++++++++++++++ tests/test_ci/test_controllers.py | 103 ++++++++++++++++- 6 files changed, 639 insertions(+), 75 deletions(-) create mode 100644 mod_ci/comparison.py create mode 100644 tests/test_ci/test_comparison.py diff --git a/mod_ci/comparison.py b/mod_ci/comparison.py new file mode 100644 index 000000000..37620cb65 --- /dev/null +++ b/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()} diff --git a/mod_ci/controllers.py b/mod_ci/controllers.py index 0bce26af6..647c78147 100755 --- a/mod_ci/controllers.py +++ b/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 @@ def set_avg_time(platform, process_type: str, time_taken: int) -> None: 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 @@ def comment_pr(test: Test) -> str: 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 @@ def comment_pr(test: Test) -> str: 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') diff --git a/mod_ci/models.py b/mod_ci/models.py index 1b5c9ebfb..20da01112 100644 --- a/mod_ci/models.py +++ b/mod_ci/models.py @@ -169,13 +169,38 @@ class Status: FAILURE = "failure" +@dataclass +class ReferenceComparison: + """How a run's results read against one other run. + + The verdict a test carries is absolute -- it matched the approved output or + it did not. This says what that verdict means *relative to somewhere else*, + which is what tells a reviewer whether a failure is theirs. + """ + + # what this reference is, for the reader: "master", "closest ancestor" + label: str + # the run being compared against, or None when there is nothing to compare to + run: Optional[Test] + # regression tests per verdict, keyed by the constants in mod_ci.comparison + tests: Dict[str, List[RegressionTest]] + # how many tests fell in each verdict + counts: Dict[str, int] + # set when this reference resolved to the same run as an earlier one + duplicate_of: Optional[str] = None + + @dataclass class PrCommentInfo: """Contains info about a test run that is useful for displaying a PR comment.""" # info about successes and failures for each category category_stats: List[CategoryTestInfo] - extra_failed_tests: List[RegressionTest] - fixed_tests: List[RegressionTest] - common_failed_tests: List[RegressionTest] - last_test_master: Test + # the verdict, and the only thing pass/fail is decided on: tests whose + # output did not match the approved file + failed_tests: List[RegressionTest] + passed_count: int + total_count: int + # the same failures described against other runs, in reading order + comparisons: List[ReferenceComparison] + last_test_master: Optional[Test] diff --git a/templates/ci/pr_comment.txt b/templates/ci/pr_comment.txt index 56ae2936c..773d9675f 100644 --- a/templates/ci/pr_comment.txt +++ b/templates/ci/pr_comment.txt @@ -1,5 +1,19 @@ +{% macro test_list(tests) -%} + +{%- endmacro %} +{% set changed = namespace(here=[], reference=none) %} +{% for comparison in comment_info.comparisons %} +{% if comparison.run and not comparison.duplicate_of and changed.reference is none %} +{% set changed.here = comparison.tests.broken_here + comparison.tests.failing_differently %} +{% set changed.reference = comparison %} +{% endif %} +{% endfor %}
-CCExtractor CI platform finished running the test files on {{platform}}. Below is a summary of the test results{% if comment_info.last_test_master %}, when compared to test for commit {{ comment_info.last_test_master.commit[:7] }}...{% endif %}: +CCExtractor CI platform finished running the test files on {{platform}}. {{ comment_info.passed_count }}/{{ comment_info.total_count }} tests matched the approved output: @@ -18,46 +32,56 @@ {% endfor %}
Report Name
-{% if comment_info.extra_failed_tests | length %} -Your PR breaks these cases: - + +{% if comment_info.failed_tests | length %} +{{ comment_info.failed_tests | length }} tests do not match the approved output. That is the pass/fail verdict. Whether this branch caused it is a separate question, answered below. +{{ test_list(comment_info.failed_tests) }} {% endif %} -{% if comment_info.common_failed_tests | length %} -NOTE: The following tests have been failing on the master branch as well as the PR: -
-{% if comment_info.extra_failed_tests | length %} -It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). -{% elif comment_info.common_failed_tests | length %} -This PR does not introduce any new test failures. However, some tests are failing on both master and this PR (see above). +{% if changed.reference is none %} +{% if comment_info.failed_tests | length %} +{{ comment_info.failed_tests | length }} tests do not match the approved output. There is no completed run to compare against, so this report cannot say whether this branch caused them — treat the list above as unattributed. {% else %} -All tests passed completely. +All tests passed: every output matched the approved file. +{% endif %} +{% elif changed.here | length %} +This branch changes the behaviour of {{ changed.here | length }} test(s) relative to {{ changed.reference.label }}. Those are the ones worth looking at; anything else in the list fails the same way on both sides. +{% elif comment_info.failed_tests | length %} +No test changes behaviour relative to {{ changed.reference.label }}: every failure above fails there too, byte for byte. The approved output for those tests is out of date, which is a baseline to review rather than a regression in this branch. +{% else %} +All tests passed: every output matched the approved file. {% endif %} - -Check the result page for more info. diff --git a/tests/test_ci/test_comparison.py b/tests/test_ci/test_comparison.py new file mode 100644 index 000000000..4e92f7ce0 --- /dev/null +++ b/tests/test_ci/test_comparison.py @@ -0,0 +1,161 @@ +"""Tests for the run-to-run comparison behind the PR comment.""" + +import unittest +from types import SimpleNamespace + +from mod_ci.comparison import (BROKEN_HERE, FAILING_DIFFERENTLY, + FAILING_IDENTICALLY, FIXED_HERE, NO_REFERENCE, + UNCHANGED_PASS, TestState, build_state, + classify, compare, summarise) + + +def entry(rt_id, error=False, files=()): + """ + Build one ``get_test_results`` test entry. + + :param rt_id: Regression test id. + :type rt_id: int + :param error: The platform's verdict: True when the test failed. + :type error: bool + :param files: (output_id, got) pairs recorded for the test. + :type files: Iterable[Tuple[int, Optional[str]]] + :return: Entry shaped like the one get_test_results produces. + :rtype: Dict[str, Any] + """ + return { + 'test': SimpleNamespace(id=rt_id), + 'error': error, + 'files': [SimpleNamespace(regression_test_output_id=output_id, got=got) + for output_id, got in files], + } + + +def category(entries): + """ + Wrap test entries in a category, as get_test_results returns them. + + :param entries: The test entries in the category. + :type entries: List[Dict[str, Any]] + :return: Category structure. + :rtype: Dict[str, Any] + """ + return {'category': SimpleNamespace(name='Category'), 'tests': entries} + + +class BuildStateTests(unittest.TestCase): + """A run summarised into one state per regression test.""" + + def test_the_platform_verdict_is_taken_as_given(self): + """passed mirrors get_test_results' own error flag, not a re-derivation. + + That flag already accounts for exit codes, missing outputs, and the + alternative hashes an output may legitimately produce -- deciding it + again here would create a second definition free to drift. + """ + states = build_state([category([entry(1, error=False), + entry(2, error=True, files=[(10, 'abc')])])]) + + self.assertTrue(states[1].passed) + self.assertFalse(states[2].passed) + + def test_a_variant_hash_that_the_platform_accepted_still_passes(self): + """A recorded hash does not imply failure: outputs may have variants.""" + states = build_state([category([entry(1, error=False, files=[(10, 'a-known-variant')])])]) + + self.assertTrue(states[1].passed) + + def test_signature_records_which_outputs_differed(self): + """The signature is what distinguishes two failures of the same test.""" + states = build_state([category([entry(1, error=True, files=[(10, 'abc')])])]) + + self.assertEqual(states[1].signature, ((10, 'abc'),)) + + def test_signature_order_does_not_depend_on_row_order(self): + """Two runs failing the same way must produce equal signatures.""" + one = build_state([category([entry(1, error=True, files=[(11, 'b'), (10, 'a')])])]) + other = build_state([category([entry(1, error=True, files=[(10, 'a'), (11, 'b')])])]) + + self.assertEqual(one[1].signature, other[1].signature) + + def test_matching_outputs_leave_no_signature(self): + """Rows without a recorded hash say nothing about how a test differed.""" + states = build_state([category([entry(1, error=False, files=[(10, None)])])]) + + self.assertEqual(states[1].signature, ()) + + +class ClassifyTests(unittest.TestCase): + """How one test's behaviour reads against a reference run.""" + + def setUp(self): + """Name the two states every case is built from.""" + self.passing = TestState(passed=True, signature=()) + self.failing = TestState(passed=False, signature=((10, 'abc'),)) + self.failing_otherwise = TestState(passed=False, signature=((10, 'def'),)) + + def test_broken_here(self): + """Passing there and failing here is the finding worth surfacing.""" + self.assertEqual(classify(self.failing, self.passing), BROKEN_HERE) + + def test_fixed_here(self): + """Failing there and passing here is the good news.""" + self.assertEqual(classify(self.passing, self.failing), FIXED_HERE) + + def test_identical_failure_is_not_this_change(self): + """Same bytes on both sides: behaviour did not move, the baseline is stale.""" + self.assertEqual(classify(self.failing, self.failing), FAILING_IDENTICALLY) + + def test_different_failure_is_worth_a_look(self): + """Both fail, but not the same way, so something did change.""" + self.assertEqual(classify(self.failing, self.failing_otherwise), FAILING_DIFFERENTLY) + + def test_unchanged_pass(self): + """Passing on both sides.""" + self.assertEqual(classify(self.passing, self.passing), UNCHANGED_PASS) + + def test_absent_reference_is_not_good_news(self): + """No record must never be reported as agreement.""" + self.assertEqual(classify(self.failing, None), NO_REFERENCE) + self.assertEqual(classify(self.passing, None), NO_REFERENCE) + + +class CompareTests(unittest.TestCase): + """Bucketing a whole run against a reference.""" + + def test_buckets_every_test_exactly_once(self): + """Nothing is dropped and nothing is double counted.""" + passing = TestState(passed=True, signature=()) + failing = TestState(passed=False, signature=((10, 'abc'),)) + current = {1: passing, 2: failing, 3: failing} + reference = {1: passing, 2: passing, 3: failing} + + buckets = compare(current, reference) + + self.assertEqual(buckets[UNCHANGED_PASS], [1]) + self.assertEqual(buckets[BROKEN_HERE], [2]) + self.assertEqual(buckets[FAILING_IDENTICALLY], [3]) + self.assertEqual(sum(summarise(buckets).values()), len(current)) + + def test_a_missing_reference_run_reports_no_reference(self): + """A reference run we do not have is distinct from one that agreed.""" + current = {1: TestState(passed=False, signature=((10, 'abc'),))} + + buckets = compare(current, None) + + self.assertEqual(buckets[NO_REFERENCE], [1]) + self.assertEqual(buckets[FAILING_IDENTICALLY], []) + + def test_the_stale_baseline_case_reads_as_not_this_change(self): + """The case this exists for: 45 tests failing identically to master. + + A branch that changed nothing relevant must not be described as + breaking them, which is what the platform used to report. + """ + drifted = TestState(passed=False, signature=((10, 'same-bytes'),)) + current = {rt: drifted for rt in range(1, 46)} + reference = dict(current) + + buckets = compare(current, reference) + + self.assertEqual(len(buckets[FAILING_IDENTICALLY]), 45) + self.assertEqual(buckets[BROKEN_HERE], []) diff --git a/tests/test_ci/test_controllers.py b/tests/test_ci/test_controllers.py index 500249711..47a46144c 100644 --- a/tests/test_ci/test_controllers.py +++ b/tests/test_ci/test_controllers.py @@ -98,9 +98,7 @@ def test_comment_info_handles_variant_files_correctly(self): test: Test = Test.query.get(TEST_RUN_ID) comment_info = get_info_for_pr_comment(test) # we got a valid variant, so should still pass - self.assertEqual(comment_info.common_failed_tests, []) - self.assertEqual(comment_info.extra_failed_tests, []) - self.assertEqual(comment_info.fixed_tests, []) + self.assertEqual(comment_info.failed_tests, []) for stats in comment_info.category_stats: # make sure the stats for the category confirm that everything passed too self.assertEqual(stats.success, stats.total) @@ -486,12 +484,19 @@ def test_comments_successfully_in_passed_pr_test(self, mock_github): # Comment on test that fails some/all regression tests test = Test.query.get(2) - comment_pr(test) + status = comment_pr(test) pull_request.get_issue_comments.assert_called_with() args, kwargs = pull_request.create_issue_comment.call_args message = kwargs['body'] - if "passed" not in message: - assert False, "Message not Correct" + + # Test 2's fixtures record a mismatch for one regression test, so the + # comment has to report that verdict. The previous assertion only looked + # for the word "passed", which the failure copy also contained -- it + # would have held whatever the comment said. + self.assertIn('matched the approved output', message) + self.assertIn('do not match the approved output', message) + self.assertNotIn('All tests passed', message) + self.assertEqual(status, Status.FAILURE) @mock.patch('mod_test.controllers.get_test_results') @mock.patch('github.Github') @@ -518,6 +523,92 @@ def test_comments_successfuly_in_failed_pr_test(self, mock_github, mock_get_test if regression_test.command not in message: assert False, "Message not Correct" + @mock.patch('mod_ci.controllers.get_info_for_pr_comment') + @mock.patch('mod_ci.controllers.Github') + def test_comment_pr_gives_the_report_a_repository_to_walk(self, mock_github, mock_info): + """The ancestor comparison needs GitHub, so the handle must reach the report. + + Building the report before opening the client silently drops that + comparison in production while every test still passes, because the + fixtures have no ancestor run either way. + """ + from mod_ci.controllers import comment_pr + from mod_test.models import Test + + mock_info.return_value = MagicMock(failed_tests=[]) + comment_pr(Test.query.get(2)) + + self.assertTrue(mock_info.called) + args, kwargs = mock_info.call_args + repository = kwargs.get('repository', args[1] if len(args) > 1 else None) + self.assertIsNotNone(repository, "the report was built without a repository") + + def test_find_ancestor_run_prefers_the_nearest_ancestor_with_records(self): + """The comparison point is where the branch was cut from, not the tip of master. + + Master moves while a branch is open. Comparing against its tip charges + the branch for everything that landed meanwhile, which is what made a + three-line PR look like it broke 45 tests. + """ + from mod_ci.controllers import find_ancestor_run + from mod_test.models import (Test, TestPlatform, TestProgress, + TestStatus, TestType) + + near = Test(TestPlatform.linux, TestType.commit, 1, 'master', 'sha_near') + far = Test(TestPlatform.linux, TestType.commit, 1, 'master', 'sha_far') + g.db.add_all([near, far]) + g.db.commit() + g.db.add_all([TestProgress(near.id, TestStatus.completed, 'done'), + TestProgress(far.id, TestStatus.completed, 'done')]) + g.db.commit() + + repository = MagicMock() + repository.get_pull.return_value.base.sha = 'sha_tip' + repository.get_commits.return_value = [MagicMock(sha=sha) for sha in + ('sha_tip', 'sha_near', 'sha_far')] + + subject = Test.query.get(1) + self.assertEqual(find_ancestor_run(repository, subject).id, near.id) + + def test_find_ancestor_run_ignores_runs_that_never_completed(self): + """A run that never finished has nothing to compare against.""" + from mod_ci.controllers import find_ancestor_run + from mod_test.models import (Test, TestPlatform, TestProgress, + TestStatus, TestType) + + unfinished = Test(TestPlatform.linux, TestType.commit, 1, 'master', 'sha_unfinished') + completed = Test(TestPlatform.linux, TestType.commit, 1, 'master', 'sha_completed') + g.db.add_all([unfinished, completed]) + g.db.commit() + g.db.add_all([TestProgress(unfinished.id, TestStatus.testing, 'still going'), + TestProgress(completed.id, TestStatus.completed, 'done')]) + g.db.commit() + + repository = MagicMock() + repository.get_pull.return_value.base.sha = 'sha_tip' + repository.get_commits.return_value = [MagicMock(sha=sha) for sha in + ('sha_unfinished', 'sha_completed')] + + subject = Test.query.get(1) + self.assertEqual(find_ancestor_run(repository, subject).id, completed.id) + + def test_find_ancestor_run_survives_github_being_unavailable(self): + """A missing comparison must not cost the whole comment.""" + from mod_ci.controllers import find_ancestor_run + from mod_test.models import Test + + repository = MagicMock() + repository.get_pull.side_effect = Exception('GitHub is having a moment') + + self.assertIsNone(find_ancestor_run(repository, Test.query.get(1))) + + def test_find_ancestor_run_without_a_repository_is_none(self): + """Callers that have no GitHub handle get no ancestor, not a crash.""" + from mod_ci.controllers import find_ancestor_run + from mod_test.models import Test + + self.assertIsNone(find_ancestor_run(None, Test.query.get(1))) + def test_get_running_instances(self): """Test get_running_instances function.""" from mod_ci.controllers import get_running_instances