[IMPROVEMENT] Fix working of draft PRs (#868)

Author: Tarun-Arora

mod_ci/controllers.py

@@ -211,7 +211,7 @@ def gcp_instance(app, db, platform, repository, delay) -> None:
     :param app: The Flask app
     :type app: Flask
     :param db: database connection
-    :type db: sqlalchemy.orm.scoped_session
+    :type db: sqlalchemy.orm.scoping.scoped_session
     :param platform: operating system
     :type platform: str
     :param repository: repository to run tests on
@@ -248,11 +248,20 @@ def gcp_instance(app, db, platform, repository, delay) -> None:
     compute = get_compute_service_object()
 
     for test in pending_tests:
-        if test.test_type == TestType.pull_request and test.pr_nr == 0:
-            log.warn(f'[{platform}] Test {test.id} is invalid, deleting')
-            db.delete(test)
-            db.commit()
-            continue
+        if test.test_type == TestType.pull_request:
+            gh_commit = repository.get_commit(test.commit)
+            if test.pr_nr == 0:
+                log.warn(f'[{platform}] Test {test.id} is invalid')
+                deschedule_test(gh_commit, message="Invalid PR number", test=test, db=db)
+                continue
+            test_pr = repository.get_pull(test.pr_nr)
+            if test.commit != test_pr.head.sha:
+                log.warn(f'[{platform}] Test {test.id} is invalid')
+                deschedule_test(gh_commit, message="PR closed or updated", test=test, db=db)
+                continue
+            if test_pr.state != 'open':
+                log.debug(f"PR {test.pr_nr} not in open state, skipping test {test.id}")
+                continue
         start_test(compute, app, db, repository, test, github_config['bot_token'])
 
 
@@ -281,7 +290,7 @@ def start_test(compute, app, db, repository: Repository.Repository, test, bot_to
     :param app: The Flask app
     :type app: Flask
     :param db: database connection
-    :type db: sqlalchemy.orm.scoped_session
+    :type db: sqlalchemy.orm.scoping.scoped_session
     :param platform: operating system
     :type platform: str
     :param repository: repository to run tests on
@@ -616,7 +625,7 @@ def add_test_entry(db, commit, test_type, branch="master", pr_nr=0) -> None:
     Add test details entry into Test model for each platform.
 
     :param db: Database connection.
-    :type db: sqlalchemy.orm.scoped_session
+    :type db: sqlalchemy.orm.scoping.scoped_session
     :param gh_commit: The GitHub API call for the commit. Can be None
     :type gh_commit: Any
     :param commit: The commit hash.
@@ -690,8 +699,8 @@ def update_status_on_github(gh_commit: Commit.Commit, state, description, contex
         log.critical(f'Could not post to GitHub! Response: {a.data}')
 
 
-def deschedule_test(gh_commit: Commit.Commit, commit, test_type, platform, branch="master",
-                    message="Tests have been cancelled", state=Status.FAILURE) -> None:
+def deschedule_test(gh_commit: Commit.Commit, commit=None, test_type=None, platform=None, branch="master",
+                    message="Tests have been cancelled", state=Status.FAILURE, test=None, db=None) -> None:
     """
     Post status to GitHub (default: as failure due to GitHub Actions incompletion).
 
@@ -709,32 +718,37 @@ def deschedule_test(gh_commit: Commit.Commit, commit, test_type, platform, branc
     :type message: str
     :param state: The status badge of the test
     :type state: Status
+    :param test: The test which is to be canceled (optional)
+    :type state: Test
+    :param db: db session
+    :type db: sqlalchemy.orm.scoping.scoped_session
     :return: Nothing
     :rtype: None
     """
     from run import log
 
-    fork_url = f"%/{g.github['repository_owner']}/{g.github['repository']}.git"
-    fork = Fork.query.filter(Fork.github.like(fork_url)).first()
-
     if test_type == TestType.pull_request:
         log.debug('pull request test type detected')
         branch = "pull_request"
 
-    platform_test = Test.query.filter(and_(Test.platform == platform,
-                                           Test.commit == commit,
-                                           Test.fork_id == fork.id,
-                                           Test.test_type == test_type,
-                                           Test.branch == branch,
-                                           )).first()
-
-    if platform_test is not None:
-        progress = TestProgress(platform_test.id, TestStatus.canceled, message, datetime.datetime.now())
-        g.db.add(progress)
-        g.db.commit()
+    if test is None:
+        fork_url = f"%/{g.github['repository_owner']}/{g.github['repository']}.git"
+        fork = Fork.query.filter(Fork.github.like(fork_url)).first()
+        test = Test.query.filter(and_(Test.platform == platform,
+                                      Test.commit == commit,
+                                      Test.fork_id == fork.id,
+                                      Test.test_type == test_type,
+                                      Test.branch == branch,
+                                      )).first()
+
+    if test is not None:
+        progress = TestProgress(test.id, TestStatus.canceled, message, datetime.datetime.now())
+        db = db or g.db
+        db.add(progress)
+        db.commit()
 
-    if gh_commit is not None:
-        update_status_on_github(gh_commit, state, message, f"CI - {platform.value}")
+        if gh_commit is not None:
+            update_status_on_github(gh_commit, state, message, f"CI - {test.platform.value}")
 
 
 def queue_test(gh_commit: Commit.Commit, commit, test_type, platform, branch="master", pr_nr=0) -> None:
@@ -900,12 +914,11 @@ def start_ci():
             # If it's a valid PR, run the tests
             pr_nr = payload['pull_request']['number']
 
-            is_draft = payload['pull_request']['draft']
             action = payload['action']
-            is_active = action in ['opened', 'synchronize', 'reopened', 'ready_for_review']
-            is_inactive = action in ['closed', 'converted_to_draft']
+            is_active = action in ['opened', 'synchronize', 'reopened']
+            is_inactive = action in ['closed']
 
-            if not is_draft and is_active:
+            if is_active:
                 try:
                     commit_hash = payload['pull_request']['head']['sha']
                 except KeyError:
@@ -935,9 +948,10 @@ def start_ci():
                     g.db.add(progress)
                     g.db.commit()
                     # If test run status exists, mark them as cancelled
-                    for status in repository.get_commit(test.commit).get_statuses():
+                    gh_commit = repository.get_commit(test.commit)
+                    for status in gh_commit.get_statuses():
                         if status["context"] == f"CI - {test.platform.value}":
-                            repository.get_commit(test.commit).create_status(
+                            gh_commit.create_status(
                                 state=Status.FAILURE,
                                 description="Tests cancelled",
                                 context=f"CI - {test.platform.value}",

tests/test_ci/test_controllers.py

@@ -256,20 +256,38 @@ def extractall(*args, **kwargs):
         mock_create_instance.assert_called_once()
         mock_wait_for_operation.assert_called_once()
 
+    @mock.patch('github.Github.get_repo')
     @mock.patch('mod_ci.controllers.start_test')
     @mock.patch('mod_ci.controllers.get_compute_service_object')
     @mock.patch('mod_ci.controllers.g')
-    def test_gcp_instance(self, mock_g, mock_get_compute_service_object, mock_start_test):
+    def test_gcp_instance(self, mock_g, mock_get_compute_service_object, mock_start_test, mock_repo):
         """Test gcp_instance function."""
         from mod_ci.controllers import gcp_instance
 
-        # Making a sample test invalid
-        test = Test.query.get(1)
-        test.pr_nr = 0
+        repo = mock_repo()
+
+        # Making test with id 1 invalid with pr_nr = 0
+        test_1 = Test.query.get(1)
+        test_1.pr_nr = 0
+
+        # Making pr of test with id 2 already updated
+        test_2 = Test.query.get(2)
+        pr_head_sha = test_2.commit + 'f'
+        repo.get_pull.return_value.head.sha = pr_head_sha
+        repo.get_pull.return_value.state = 'open'
+
+        # Creating a test with pull_request type that is valid
+        test_3 = Test(TestPlatform.linux, TestType.pull_request, 1, "pull_request", pr_head_sha, 2)
+        g.db.add(test_3)
+
+        # Creating a test with commit type that is valid
+        test_4 = Test(TestPlatform.linux, TestType.commit, 1, "master", pr_head_sha)
+        g.db.add(test_4)
         g.db.commit()
-        gcp_instance(self.app, mock_g.db, TestPlatform.linux, mock.ANY, None)
 
-        mock_start_test.assert_called_once()
+        gcp_instance(self.app, mock_g.db, TestPlatform.linux, repo, None)
+
+        self.assertEqual(mock_start_test.call_count, 2)
         mock_get_compute_service_object.assert_called_once()
 
     def test_get_compute_service_object(self):
@@ -459,9 +477,10 @@ def test_customizedtest_added_to_queue(self):
         self.assertIn(2, customized_test)
         self.assertNotIn(1, customized_test)
 
+    @mock.patch('flask.g.log.error')
     @mock.patch('mailer.Mailer')
     @mock.patch('mod_ci.controllers.get_html_issue_body')
-    def test_inform_mailing_list(self, mock_get_html_issue_body, mock_email):
+    def test_inform_mailing_list(self, mock_get_html_issue_body, mock_email, mock_log_error):
         """Test the inform_mailing_list function."""
         from mod_ci.controllers import inform_mailing_list
 
@@ -482,6 +501,11 @@ def test_inform_mailing_list(self, mock_get_html_issue_body, mock_email):
             }
         )
         mock_get_html_issue_body.assert_called_once()
+        mock_log_error.assert_not_called()
+
+        mock_email.send_simple_message.return_value = False
+        inform_mailing_list(mock_email, "matejmecka", "2430", "Some random string", "Lorem Ipsum sit dolor amet...")
+        mock_log_error.assert_called_once()
 
     @staticmethod
     @mock.patch('mod_ci.controllers.markdown')
@@ -741,34 +765,6 @@ def __init__(self):
 
         mock_test.query.filter.assert_called_once()
 
-    @mock.patch('github.Github.get_repo')
-    @mock.patch('mod_ci.controllers.Test')
-    @mock.patch('requests.get', side_effect=mock_api_request_github)
-    def test_webhook_pr_converted_to_draft(self, mock_requests, mock_test, mock_repo):
-        """Test webhook triggered with pull_request event with converted_to_draft action."""
-        platform_name = "platform"
-
-        class MockTest:
-            def __init__(self):
-                self.id = 1
-                self.progress = []
-                self.platform = MockPlatform(platform_name)
-                self.commit = "test"
-
-        mock_test.query.filter.return_value.all.return_value = [MockTest()]
-        mock_repo.return_value.get_commit.return_value.get_statuses.return_value = [
-            {"context": f"CI - {platform_name}"}]
-
-        data = {'action': 'converted_to_draft',
-                'pull_request': {'number': 1234, 'draft': False}}
-        # one of ip address from GitHub web hook
-        with self.app.test_client() as c:
-            response = c.post(
-                '/start-ci', environ_overrides=WSGI_ENVIRONMENT,
-                data=json.dumps(data), headers=self.generate_header(data, 'pull_request'))
-
-        mock_test.query.filter.assert_called_once()
-
     @mock.patch('mod_ci.controllers.BlockedUsers')
     @mock.patch('github.Github.get_repo')
     @mock.patch('requests.get', side_effect=mock_api_request_github)
@@ -803,25 +799,6 @@ def test_webhook_pr_opened(self, mock_request, mock_add_test_entry, mock_repo, m
         mock_blocked.query.filter.assert_called_once_with(mock_blocked.user_id == 'test')
         mock_add_test_entry.assert_called_once()
 
-    @mock.patch('mod_ci.controllers.BlockedUsers')
-    @mock.patch('github.Github.get_repo')
-    @mock.patch('mod_ci.controllers.add_test_entry')
-    @mock.patch('requests.get', side_effect=mock_api_request_github)
-    def test_webhook_pr_ready_for_review(self, mock_request, mock_add_test_entry, mock_repo, mock_blocked):
-        """Test webhook triggered with pull_request event with ready_for_review action."""
-        mock_blocked.query.filter.return_value.first.return_value = None
-
-        data = {'action': 'ready_for_review',
-                'pull_request': {'number': 1234, 'head': {'sha': 'abcd1234'}, 'user': {'id': 'test'}, 'draft': False}}
-        with self.app.test_client() as c:
-            response = c.post(
-                '/start-ci', environ_overrides=WSGI_ENVIRONMENT,
-                data=json.dumps(data), headers=self.generate_header(data, 'pull_request'))
-
-        self.assertEqual(response.data, b'{"msg": "EOL"}')
-        mock_blocked.query.filter.assert_called_once_with(mock_blocked.user_id == 'test')
-        mock_add_test_entry.assert_called_once()
-
     @mock.patch('mod_ci.controllers.BlockedUsers')
     @mock.patch('github.Github.get_repo')
     @mock.patch('requests.get', side_effect=mock_api_request_github)
@@ -1292,10 +1269,8 @@ def test_github_api_error(self, mock_critical, mock_github):
         schedule_test(github_status)
         mock_critical.assert_called()
         mock_critical.reset_mock()
-        deschedule_test(github_status, 1, TestType.commit, TestPlatform.linux)
-        mock_critical.assert_called()
-        mock_critical.reset_mock()
-        deschedule_test(github_status, 1, TestType.commit, TestPlatform.windows)
+        test = Test.query.first()
+        deschedule_test(github_status, test=test, db=g.db)
         mock_critical.assert_called()
 
     @mock.patch('mod_ci.controllers.is_main_repo')

tests/test_sample/test_controllers.py

@@ -200,3 +200,13 @@ def test_edit_sample(self):
             sample = Sample.query.filter(Sample.id == 1).first()
             self.assertEqual(sample.upload.notes, 'mp4')
             self.assertEqual(sample.upload.parameters, '-latin1')
+
+    def test_edit_sample_get_request(self):
+        """Check the edit sample get form request."""
+        self.create_user_with_role(self.user.name, self.user.email, self.user.password, Role.admin)
+
+        with self.app.test_client() as c:
+            c.post('/account/login', data=self.create_login_form_data(self.user.email, self.user.password))
+            response = c.get('/sample/edit/1')
+            self.assertIn("Editing sample with id 1", str(response.data))
+            self.assertEqual(response.status_code, 200)