Merge pull request #45 from buildkite/pie-3532-failed-test-reported-as-passed

Fix the hook handling order that caused `failed` test to be reported as `passed`

Author: nprizal

src/buildkite_test_collector/collector/payload.py

@@ -141,7 +141,9 @@ def tag_execution(self, key: str, val: str) -> 'TestData':
         if not isinstance(key, str) or not isinstance(val, str):
             raise TypeError("Expected string for key and value")
 
-        self.tags[key] = val
+        new_tags = self.tags.copy()
+        new_tags[key] = val
+        return replace(self, tags=new_tags)
 
     def finish(self) -> 'TestData':
         """Set the end_at and duration on this test"""

src/buildkite_test_collector/pytest_plugin/buildkite_plugin.py

@@ -1,6 +1,5 @@
 """Buildkite test collector plugin for Pytest"""
 import json
-
 from uuid import uuid4
 
 from ..collector.payload import TestData
@@ -29,26 +28,21 @@ def pytest_runtest_logstart(self, nodeid, location):
         )
         self.in_flight[nodeid] = test_data
 
-    def pytest_runtest_teardown(self, item):
-        """pytest_runtest_hook hook callback to collect execution_tag"""
-        test_data = self.in_flight.get(item.nodeid)
-
-        if test_data:
-            tags = item.iter_markers("execution_tag")
-            for tag in tags:
-                test_data.tag_execution(tag.args[0], tag.args[1])
-
     def pytest_runtest_logreport(self, report):
-        """pytest_runtest_logreport hook callback"""
-        if report.when != 'teardown':
+        """pytest_runtest_logreport hook callback to get test outcome after test call"""
+
+        # This hook is called three times during the lifecycle of a test:
+        # after the setup phase, the call phase, and the teardown phase.
+        # Since we want to capture the outcome from the call phase,
+        # we only proceed when this hook is triggered following the call phase.
+        # See: https://github.com/buildkite/test-collector-python/pull/45
+        if report.when != 'call':
             return
 
         nodeid = report.nodeid
         test_data = self.in_flight.get(nodeid)
 
         if test_data:
-            test_data = test_data.finish()
-
             if report.passed:
                 test_data = test_data.passed()
 
@@ -58,7 +52,23 @@ def pytest_runtest_logreport(self, report):
             if report.skipped:
                 test_data = test_data.skipped()
 
-            del self.in_flight[nodeid]
+            # TestData is immutable.
+            # We need to replace the test_data in `in_flight` with updated test_data,
+            # so we can get the correct result when we process it during the teardown hook.
+            self.in_flight[nodeid] = test_data
+
+    def pytest_runtest_teardown(self, item):
+        """pytest_runtest_hook hook callback to mark test as finished and add it to the payload"""
+        test_data = self.in_flight.get(item.nodeid)
+
+        if test_data:
+            test_data = test_data.finish()
+
+            tags = item.iter_markers("execution_tag")
+            for tag in tags:
+                test_data = test_data.tag_execution(tag.args[0], tag.args[1])
+
+            del self.in_flight[item.nodeid]
             self.payload = self.payload.push_test_data(test_data)
 
     def save_payload_as_json(self, path):

tests/buildkite_test_collector/collector/test_payload.py

@@ -171,14 +171,14 @@ def test_test_data_as_json_when_skipped(skipped_test):
 
 class TestTestDataTagExecution:
     def test_test_data_tag_execution(self, successful_test):
-        successful_test.tag_execution("owner", "test-engine")
-        successful_test.tag_execution("python.version", "3.12.3")
+        test_data = successful_test.tag_execution("owner", "test-engine")
+        test_data = test_data.tag_execution("python.version", "3.12.3")
 
         expected_tags = {"owner": "test-engine", "python.version": "3.12.3"}
 
-        assert successful_test.tags == expected_tags
+        assert test_data.tags == expected_tags
 
-        json = successful_test.as_json(Instant.now())
+        json = test_data.as_json(Instant.now())
         assert json["tags"] == {"owner": "test-engine", "python.version": "3.12.3"}
 
     def test_test_data_tag_execution_non_string(self, successful_test):