Compare against the approved baseline, not the previous run's output

mod_ci/controllers.py

@@ -1001,12 +1001,6 @@ def start_test(compute, app, db, repository: Repository.Repository, test, bot_to
     Path(base_folder).mkdir(parents=True, exist_ok=True)
 
     categories = Category.query.order_by(Category.id.desc()).all()
-    commit_name = 'fetch_commit_' + test.platform.value
-    commit_hash = GeneralData.query.filter(GeneralData.key == commit_name).first().value
-    last_commit = Test.query.filter(and_(Test.commit == commit_hash, Test.platform == test.platform)).first()
-
-    if last_commit is not None:
-        log.debug(f"[{gcp_instance_name}] We will compare against the results of test {last_commit.id}")
 
     regression_ids = test.get_customized_regressiontests()
 
@@ -1039,25 +1033,18 @@ def start_test(compute, app, db, repository: Repository.Repository, test, bot_to
             output_node = etree.SubElement(entry, 'output')
             output_node.text = regression_test.output_type.value
             compare = etree.SubElement(entry, 'compare')
-            last_files = TestResultFile.query.filter(and_(
-                TestResultFile.test_id == last_commit.id,
-                TestResultFile.regression_test_id == regression_test.id
-            )).subquery()
 
             for output_file in regression_test.output_files:
                 ignore_file = str(output_file.ignore).lower()
                 file_node = etree.SubElement(compare, 'file', ignore=ignore_file, id=str(output_file.id))
-                last_commit_files = db.query(last_files.c.got).filter(and_(
-                    last_files.c.regression_test_output_id == output_file.id,
-                    last_files.c.got.isnot(None)
-                )).first()
                 correct = etree.SubElement(file_node, 'correct')
                 # Need a path that is relative to the folder we provide inside the CI environment.
-                if last_commit_files is None:
-                    log.debug(f"Selecting original file for RT #{regression_test.id} ({category.name})")
-                    correct.text = output_file.filename_correct
-                else:
-                    correct.text = output_file.create_correct_filename(last_commit_files[0])
+                # Always the approved baseline. This used to fall back to the previous run's own
+                # output whenever that run had recorded a mismatch, which made the reference roll
+                # forward on its own: a behavioural change was flagged once and then silently
+                # adopted, and because a passing run records no output, the run after that fell
+                # back here again and the same test failed anew. See issue #1173.
+                correct.text = output_file.filename_correct
 
                 expected = etree.SubElement(file_node, 'expected')
                 expected.text = output_file.filename_expected(regression_test.sample.sha)

tests/test_ci/test_controllers.py

@@ -291,6 +291,83 @@ def extractall(*args, **kwargs):
         mock_g.db.commit.assert_called_once()
         mock_create_instance.assert_called_once()
 
+    @mock.patch('mod_ci.controllers.wait_for_operation')
+    @mock.patch('mod_ci.controllers.create_instance')
+    @mock.patch('mod_ci.controllers.save_xml_to_file')
+    @mock.patch('builtins.open', new_callable=mock.mock_open())
+    @mock.patch('mod_ci.controllers.g')
+    @mock.patch('mod_ci.controllers.TestProgress')
+    @mock.patch('mod_ci.controllers.GcpInstance')
+    def test_start_test_compares_against_approved_baseline(self, mock_gcp_instance, mock_test_progress,
+                                                           mock_g, mock_open_file, mock_save_xml,
+                                                           mock_create_instance, mock_wait_for_operation):
+        """The file a test is compared against must be the approved baseline (issue #1173).
+
+        It used to be the previous run's own output whenever that run had recorded a
+        mismatch, which let a behavioural change promote itself to the reference.
+        Fixture ``TestResultFile(2, 2, 2, "sample_out2", "out2")`` is exactly that case:
+        a recorded ``got`` of ``out2`` for the output whose approved baseline is
+        ``sample_out2``.
+        """
+        import zipfile
+
+        import requests
+        from github.Artifact import Artifact
+
+        from mod_ci.controllers import Artifact_names, start_test
+
+        mock_gcp_instance.query.filter.return_value.first.return_value = None
+        mock_test_progress.query.filter.return_value.first.return_value = None
+
+        test = Test.query.first()
+        repository = MagicMock()
+
+        artifact = MagicMock(Artifact)
+        artifact.name = Artifact_names.linux if test.platform == TestPlatform.linux else Artifact_names.windows
+        artifact.workflow_run.head_sha = test.commit
+
+        class mock_zip:
+            def __enter__(self):
+                return self
+
+            def __exit__(self, *args):
+                return False
+
+            def extractall(*args, **kwargs):
+                return None
+
+        repository.get_artifacts.return_value = [artifact]
+        response = requests.models.Response()
+        response.status_code = 200
+        requests.get = MagicMock(return_value=response)
+        zipfile.ZipFile = MagicMock(return_value=mock_zip())
+
+        create_mock_db_query(mock_g)
+        mock_create_instance.return_value = {'name': 'op-1', 'status': 'RUNNING'}
+        mock_wait_for_operation.return_value = {'status': 'DONE'}
+
+        start_test(mock.ANY, self.app, mock_g.db, repository, test, mock.ANY)
+
+        self.assertTrue(mock_save_xml.called, "start_test wrote no test definition XML")
+
+        correct_texts = []
+        for call in mock_save_xml.call_args_list:
+            for correct in call.args[0].iter('correct'):
+                correct_texts.append(correct.text)
+
+        self.assertTrue(correct_texts, "no <correct> elements were emitted")
+
+        # The approved baselines, and nothing derived from a previous run's output.
+        expected = {rto.filename_correct for rto in RegressionTestOutput.query.all()}
+        for text in correct_texts:
+            self.assertIn(text, expected,
+                          f"compared against {text!r}, which is not an approved baseline")
+
+        stored_got = 'out2' + RegressionTestOutput.query.filter(
+            RegressionTestOutput.id == 2).first().correct_extension
+        self.assertNotIn(stored_got, correct_texts,
+                         "a previous run's recorded output was used as the comparison target")
+
     @mock.patch('github.Github.get_repo')
     @mock.patch('mod_ci.controllers.start_test')
     @mock.patch('mod_ci.controllers.get_compute_service_object')