[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
4e78b55 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 <[email protected]>
Co-authored-by: Shivam Kumar Jha <[email protected]>

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'])

mod_ci/models.py

@@ -9,14 +9,16 @@
 """
 
 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)
 from sqlalchemy.orm import relationship
 
 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"<Platform {self.platform.description}, maintenance {self.disabled}>"
+
+
+@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]

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