[IMPROVEMENT] Compare tests with master branch for PRs (#869)

Author: Tarun-Arora

mod_ci/controllers.py

@@ -33,7 +33,7 @@
 from mod_auth.models import Role
 from mod_ci.forms import AddUsersToBlacklist, DeleteUserForm
 from mod_ci.models import (BlockedUsers, GcpInstance, MaintenanceMode,
-                           PrCommentInfo)
+                           PrCommentInfo, Status)
 from mod_customized.models import CustomizedTest
 from mod_home.models import CCExtractorVersion, GeneralData
 from mod_regression.models import (Category, RegressionTest,
@@ -48,15 +48,6 @@
 mod_ci = Blueprint('ci', __name__)
 
 
-class Status:
-    """Define different states for the tests."""
-
-    PENDING = "pending"
-    SUCCESS = "success"
-    ERROR = "error"
-    FAILURE = "failure"
-
-
 class Workflow_builds(DeclEnum):
     """Define GitHub Action workflow build names."""
 
@@ -1304,7 +1295,7 @@ def progress_type_request(log, test, test_id, request) -> bool:
             state = Status.SUCCESS
             message = 'Tests completed'
         if test.test_type == TestType.pull_request:
-            comment_pr(test.id, state, test.pr_nr, test.platform.name)
+            state = comment_pr(test.id, test.pr_nr, test.platform.name)
         update_build_badge(state, test)
 
     else:
@@ -1604,23 +1595,32 @@ def get_query_regression_testid_passed(test_id: int) -> Query:
 
 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 = get_query_regression_testid_passed(test_id)
+    last_test_master = g.db.query(Test).filter(Test.branch == "master", Test.test_type == TestType.commit).join(
+        TestProgress, Test.id == TestProgress.test_id).filter(
+            TestProgress.status == TestStatus.completed).order_by(TestProgress.id.desc()).first()
+    regression_test_passed = get_query_regression_testid_passed(test_id)
 
     passed = g.db.query(label('category_id', Category.id), label(
         'success', count(regressionTestLinkTable.c.regression_id))).filter(
-            regressionTestLinkTable.c.regression_id.in_(regression_testid_passed),
+            regressionTestLinkTable.c.regression_id.in_(regression_test_passed),
             Category.id == regressionTestLinkTable.c.category_id).group_by(
             regressionTestLinkTable.c.category_id).subquery()
     tot = g.db.query(label('category', Category.name), label('total', count(regressionTestLinkTable.c.regression_id)),
                      label('success', passed.c.success)).outerjoin(
         passed, passed.c.category_id == Category.id).filter(
         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)
+    regression_test_failed = RegressionTest.query.filter(RegressionTest.id.notin_(regression_test_passed)).all()
+
+    last_test_master_id = last_test_master.id if last_test_master else 0
+    extra_failed_tests = [test for test in regression_test_failed if test.last_passed_on == last_test_master]
+    fixed_tests = RegressionTest.query.filter(RegressionTest.id.in_(regression_test_passed),
+                                              RegressionTest.last_passed_on != last_test_master_id).all()
+    common_failed_tests = [test for test in regression_test_failed if test.last_passed_on != last_test_master]
+    return PrCommentInfo(tot, extra_failed_tests, fixed_tests, common_failed_tests, last_test_master)
 
 
-def comment_pr(test_id, state, pr_nr, platform) -> None:
+def comment_pr(test_id, pr_nr, platform) -> str:
     """
     Upload the test report to the GitHub PR as comment.
 
@@ -1637,8 +1637,7 @@ def comment_pr(test_id, state, pr_nr, platform) -> None:
 
     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=comment_info.category_stats, failed_tests=comment_info.failed_tests,
-                              test_id=test_id, state=state, platform=platform)
+    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:
         gh = Github(g.github['bot_token'])
@@ -1655,6 +1654,7 @@ def comment_pr(test_id, state, pr_nr, platform) -> None:
         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
 
 
 @mod_ci.route('/show_maintenance')

mod_ci/models.py

@@ -16,7 +16,6 @@
                         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
@@ -124,11 +123,22 @@ class CategoryTestInfo:
     success: Optional[int]
 
 
+class Status:
+    """Define different states for the tests."""
+
+    PENDING = "pending"
+    SUCCESS = "success"
+    ERROR = "error"
+    FAILURE = "failure"
+
+
 @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]
+    extra_failed_tests: List[RegressionTest]
+    fixed_tests: List[RegressionTest]
+    common_failed_tests: List[RegressionTest]
+    last_test_master: Test

templates/ci/pr_comment.txt

@@ -1,10 +1,10 @@
-<b>CCExtractor CI platform</b> finished running the test files on <b>{{platform}}</b>. Below is a summary of the test results:
+<b>CCExtractor CI platform</b> finished running the test files on <b>{{platform}}</b>. Below is a summary of the test results{% if comment_info.last_test_master %}, when compared to <a href="{{ url_for('test.by_id', test_id=comment_info.last_test_master.id, _external=True) }}">test for commit  {{ comment_info.last_test_master.commit[:7] }}...</a>{% endif %}:
 <table>
 <thead>
 <td> Report Name </td>
 <td> Tests Passed </td>
 </thead>
-{% for test in tests%}
+{% for test in comment_info.category_stats %}
 <tr>
 <td> {{test.category}} </td>
 {% if test.success==None %}
@@ -17,17 +17,33 @@
 </tr>
 {% endfor %}
 </table>
-{% if state == "failure" %}
+{% 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). 
 
 Your PR breaks these cases:
 <ul>
-{% for test in failed_tests %}
-<li> ccextractor {{ test.command }} <a href="{{ url_for('sample.sample_by_id', sample_id=test.sample.id, _external=True) }}">{{ test.sample.sha[:10] }}...</a>, Last passed: {% if test.last_passed_on %}<a href="{{ url_for('test.by_id', test_id=test.last_passed_on, _external=True) }}">Test {{ test.last_passed_on }}</a>{% else %}<span>Never</span>{% endif %}</li>
+{% for test in comment_info.extra_failed_tests %}
+<li> ccextractor {{ test.command }} <a href="{{ url_for('sample.sample_by_id', sample_id=test.sample.id, _external=True) }}">{{ test.sample.sha[:10] }}...</a></li>
 {% endfor %}
 </ul>
 {% else %}
-All tests were passed completely.
+All tests passing on the master branch were passed completely.
+{% endif %}
+{% if comment_info.common_failed_tests | length %}
+NOTE: The following tests have been failing on the master branch as well as the PR:
+<ul>
+{% for test in comment_info.common_failed_tests %}
+<li> ccextractor {{ test.command }} <a href="{{ url_for('sample.sample_by_id', sample_id=test.sample.id, _external=True) }}">{{ test.sample.sha[:10] }}...</a>, Last passed: {% if test.last_passed_on %}<a href="{{ url_for('test.by_id', test_id=test.last_passed_on, _external=True) }}">Test {{ test.last_passed_on }}</a>{% else %}<span>Never</span>{% endif %}</li>
+{% endfor %}
+</ul>
+{% endif %}
+{% if comment_info.fixed_tests | length %}
+Congratulations: Merging this PR would fix the following tests:
+<ul>
+{% for test in comment_info.fixed_tests %}
+<li> ccextractor {{ test.command }} <a href="{{ url_for('sample.sample_by_id', sample_id=test.sample.id, _external=True) }}">{{ test.sample.sha[:10] }}...</a>, Last passed: {% if test.last_passed_on %}<a href="{{ url_for('test.by_id', test_id=test.last_passed_on, _external=True) }}">Test {{ test.last_passed_on }}</a>{% else %}<span>Never</span>{% endif %}</li>
+{% endfor %}
+</ul>
 {% endif %}
 <br>
 Check the <a href="{{ url_for('test.by_id', test_id=test_id, _external=True) }}">result</a> page for more info.

tests/test_ci/test_controllers.py

@@ -85,7 +85,9 @@ def test_comment_info_handles_variant_files_correctly(self):
         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, [])
+        self.assertEqual(comment_info.common_failed_tests, [])
+        self.assertEqual(comment_info.extra_failed_tests, [])
+        self.assertEqual(comment_info.fixed_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)
@@ -377,7 +379,7 @@ def test_comments_successfully_in_passed_pr_test(self, mock_github):
 
         # Delete old bot comments and create a new comment
         pull_request.get_issue_comments.return_value = [comment1, comment2]
-        comment_pr(1, Status.SUCCESS, 1, 'linux')
+        comment_pr(1, 1, 'linux')
         mock_github.assert_called_with(g.github['bot_token'])
         mock_github(g.github['bot_token']).get_repo.assert_called_with(
             f"{g.github['repository_owner']}/{g.github['repository']}")
@@ -405,7 +407,7 @@ def test_comments_successfuly_in_failed_pr_test(self, mock_github):
                    "test files on <b>linux</b>. Below is a summary of the test results")
         pull_request.get_issue_comments.return_value = [MagicMock(IssueComment)]
         # Comment on test that fails some/all regression tests
-        comment_pr(2, Status.FAILURE, 1, 'linux')
+        comment_pr(2, 1, 'linux')
         pull_request.get_issue_comments.assert_called_with()
         args, kwargs = pull_request.create_issue_comment.call_args
         message = kwargs['body']