From 7fcb343b3c9cd5ce6b398d7f6affec1b5bd74e93 Mon Sep 17 00:00:00 2001 From: pranavrajpal <78008260+pranavrajpal@users.noreply.github.com> Date: Mon, 17 May 2021 05:35:13 -0700 Subject: [PATCH] [FIX] Fix incorrect PR comment generation (#540) * Add test for variant output files in PR comment Add a test that the info used to generate a PR comment handles variant output files correctly. Regression tests that output one of the allowed variants of the output file should be treated as correct in addition to tests that output the original version of the output file. * Refactor getting PR comment info into new function Refactor the code for getting the info about a test run for use in a PR comment into a separate function. This makes this function easier to test instead of testing the entire process of creating a PR comment at the same time. * Fix variant output file handling in PR comments Currently, when generating stats and a list of failed tests for PR comments, tests are only marked as passed if they exactly matched the original output file. This commit changes that by also treating a regression test as passed if it is an acceptable variant of the output file. * Add test for invalid variant hash Add another test to make sure that the change in 4e78b55aff5a6a3591059be2e64f91dedb978ef9 didn't accidentally cause the PR comment handling to mark all tests as passing even if they result in an invalid variant file. * Rename get_info_about_test_for_pr_comment nosetests appears to be treating get_info_about_test_for_pr_comment as a test to run because it has 'test' in the name. That causes the test suite to fail because it doesn't pass in the required argument. This renames the function so that nosetests realizes that it shouldn't treat it as a test. * Move dataclasses to models.py Move dataclasses used for representing info about PR comments to models.py so that all classes for storing data are placed together. Co-authored-by: Willem Co-authored-by: Shivam Kumar Jha --- mod_ci/controllers.py | 49 +++++++++++++++---------- mod_ci/models.py | 26 +++++++++++++- tests/test_ci/TestControllers.py | 61 ++++++++++++++++++++++++++++++-- 3 files changed, 113 insertions(+), 23 deletions(-) diff --git a/mod_ci/controllers.py b/mod_ci/controllers.py index 430f0bf6..e163aec5 100755 --- a/mod_ci/controllers.py +++ b/mod_ci/controllers.py @@ -28,12 +28,13 @@ from mod_auth.controllers import check_access_rights, login_required from mod_auth.models import Role from mod_ci.forms import AddUsersToBlacklist, DeleteUserForm -from mod_ci.models import BlockedUsers, Kvm, MaintenanceMode +from mod_ci.models import BlockedUsers, Kvm, MaintenanceMode, PrCommentInfo from mod_customized.models import CustomizedTest from mod_deploy.controllers import is_valid_signature, request_from_github from mod_home.models import CCExtractorVersion, GeneralData from mod_regression.models import (Category, RegressionTest, RegressionTestOutput, + RegressionTestOutputFiles, regressionTestLinkTable) from mod_sample.models import Issue from mod_test.models import (Fork, Test, TestPlatform, TestProgress, @@ -1145,20 +1146,8 @@ def set_avg_time(platform, process_type: str, time_taken: int) -> None: g.db.commit() -def comment_pr(test_id, state, pr_nr, platform) -> None: - """ - Upload the test report to the GitHub PR as comment. - - :param test_id: The identity of Test whose report will be uploaded - :type test_id: str - :param state: The state of the PR. - :type state: Status - :param pr_nr: PR number to which test commit is related and comment will be uploaded - :type: str - :param platform - :type: str - """ - from run import app, log +def get_info_for_pr_comment(test_id: int) -> PrCommentInfo: + """Return info about the given test id for use in a PR comment.""" regression_testid_passed = g.db.query(TestResult.regression_test_id).outerjoin( TestResultFile, TestResult.test_id == TestResultFile.test_id).filter( TestResult.test_id == test_id, @@ -1167,8 +1156,11 @@ def comment_pr(test_id, state, pr_nr, platform) -> None: TestResult.exit_code != 0, and_(TestResult.exit_code == 0, TestResult.regression_test_id == TestResultFile.regression_test_id, - TestResultFile.got.is_(None) - ), + or_(TestResultFile.got.is_(None), + and_( + RegressionTestOutputFiles.regression_test_output_id == TestResultFile.regression_test_output_id, + TestResultFile.got == RegressionTestOutputFiles.file_hashes + ))), and_( RegressionTestOutput.regression_id == TestResult.regression_test_id, RegressionTestOutput.ignore.is_(True), @@ -1184,9 +1176,28 @@ def comment_pr(test_id, state, pr_nr, platform) -> None: Category.id == regressionTestLinkTable.c.category_id).group_by( regressionTestLinkTable.c.category_id).all() regression_testid_failed = RegressionTest.query.filter(RegressionTest.id.notin_(regression_testid_passed)).all() + return PrCommentInfo(tot, regression_testid_failed) + + +def comment_pr(test_id, state, pr_nr, platform) -> None: + """ + Upload the test report to the GitHub PR as comment. + + :param test_id: The identity of Test whose report will be uploaded + :type test_id: str + :param state: The state of the PR. + :type state: Status + :param pr_nr: PR number to which test commit is related and comment will be uploaded + :type: str + :param platform + :type: str + """ + from run import app, log + + comment_info = get_info_for_pr_comment(test_id) template = app.jinja_env.get_or_select_template('ci/pr_comment.txt') - message = template.render(tests=tot, failed_tests=regression_testid_failed, test_id=test_id, - state=state, platform=platform) + message = template.render(tests=comment_info.category_stats, failed_tests=comment_info.failed_tests, + test_id=test_id, state=state, platform=platform) log.debug(f"GitHub PR Comment Message Created for Test_id: {test_id}") try: gh = GitHub(access_token=g.github['bot_token']) diff --git a/mod_ci/models.py b/mod_ci/models.py index 188c97d8..ffd5ae82 100644 --- a/mod_ci/models.py +++ b/mod_ci/models.py @@ -9,7 +9,8 @@ """ import datetime -from typing import Any, Dict, Type +from dataclasses import dataclass +from typing import Any, Dict, List, Optional, Type from sqlalchemy import (Boolean, Column, DateTime, ForeignKey, Integer, String, Text) @@ -17,6 +18,7 @@ import mod_test.models from database import Base +from mod_regression.models import RegressionTest from mod_test.models import Test, TestPlatform @@ -109,3 +111,25 @@ def __repr__(self) -> str: :rtype str(platform, status): str """ return f"" + + +@dataclass +class CategoryTestInfo: + """Contains information about the number of successful tests for a specific category during a specific test run.""" + + # the test category being referred to + category: str + # the total number of tests in this category + total: int + # the number of successful tests - None if no tests were successful + success: Optional[int] + + +@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] + # list of regression tests that failed + failed_tests: List[RegressionTest] diff --git a/tests/test_ci/TestControllers.py b/tests/test_ci/TestControllers.py index cd053949..fe0ac21b 100644 --- a/tests/test_ci/TestControllers.py +++ b/tests/test_ci/TestControllers.py @@ -7,12 +7,13 @@ from werkzeug.datastructures import Headers from mod_auth.models import Role -from mod_ci.controllers import start_platforms +from mod_ci.controllers import get_info_for_pr_comment, start_platforms from mod_ci.models import BlockedUsers from mod_customized.models import CustomizedTest from mod_home.models import CCExtractorVersion, GeneralData -from mod_regression.models import RegressionTest -from mod_test.models import Test, TestPlatform, TestType +from mod_regression.models import (RegressionTest, RegressionTestOutput, + RegressionTestOutputFiles) +from mod_test.models import Test, TestPlatform, TestResultFile, TestType from tests.base import (BaseTestCase, generate_git_api_header, generate_signature, mock_api_request_github) @@ -55,6 +56,60 @@ def __init__(self): class TestControllers(BaseTestCase): """Test CI-related controllers.""" + def test_comment_info_handles_variant_files_correctly(self): + """Test that allowed variants of output files are handled correctly in PR comments. + + Make sure that the info used for the PR comment treats tests as successes if they got one of the allowed + variants of the output file instead of the original version + """ + VARIANT_HASH = 'abcdefgh_this_is_a_hash' + TEST_RUN_ID = 1 + # Setting up the database + # create regression test with an output file that has an allowed variant + regression_test: RegressionTest = RegressionTest.query.filter(RegressionTest.id == 1).first() + output_file: RegressionTestOutput = regression_test.output_files[0] + variant_output_file = RegressionTestOutputFiles(VARIANT_HASH, output_file.id) + output_file.multiple_files.append(variant_output_file) + g.db.add(output_file) + + test_run_output_file: TestResultFile = TestResultFile.query.filter( + TestResultFile.regression_test_output_id == output_file.id, + TestResultFile.test_id == TEST_RUN_ID + ).first() + # mark the test as getting the variant as output, not the expected file + test_run_output_file.got = VARIANT_HASH + g.db.add(test_run_output_file) + g.db.commit() + + # The actual test + test: Test = Test.query.filter(Test.id == TEST_RUN_ID).first() + comment_info = get_info_for_pr_comment(test.id) + # we got a valid variant, so should still pass + 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) + + def test_comment_info_handles_invalid_variants_correctly(self): + """Test that invalid variants of output files are handled correctly in PR comments. + + Make sure that regression tests are correctly marked as not passing when an invalid file hash is found + """ + INVALID_VARIANT_HASH = 'this_is_an_invalid_hash' + TEST_RUN_ID = 1 + test_result_file: TestResultFile = TestResultFile.query.filter(TestResultFile.test_id == TEST_RUN_ID).first() + test_result_file.got = INVALID_VARIANT_HASH + g.db.add(test_result_file) + g.db.commit() + + test: Test = Test.query.filter(Test.id == TEST_RUN_ID).first() + comment_info = get_info_for_pr_comment(test.id) + # all categories that this regression test applies to should fail because of the invalid hash + for category in test_result_file.regression_test.categories: + stats = [stat for stat in comment_info.category_stats if stat.category == category.name] + for stat in stats: + self.assertEqual(stat.success, None) + @mock.patch('mod_ci.controllers.Process') @mock.patch('run.log') def test_start_platform_none_specified(self, mock_log, mock_process):