From da6bdff8934280d5bdec7042e90becb58697545a Mon Sep 17 00:00:00 2001
From: Alexander Bayandin <alexander@neon.tech>
Date: Mon, 29 Jul 2024 11:00:33 +0100
Subject: [PATCH] test_runner: fix user_property usage in benchmarks (#8531)

## Problem

After https://github.com/neondatabase/neon/pull/7990 `regress_test` job
started to fail with an error:
```
...
File "/__w/neon/neon/test_runner/fixtures/benchmark_fixture.py", line 485, in pytest_terminal_summary
  terminalreporter.write(f"{test_report.head_line}.{recorded_property['name']}: ")
TypeError: 'bool' object is not subscriptable
```

https://github.com/neondatabase/neon/actions/runs/10125750938/job/28002582582

It happens because the current implementation doesn't expect pytest's
`user_properties` can be used for anything else but benchmarks (and
https://github.com/neondatabase/neon/pull/7990 started to use it for
tracking `preserve_database_files` parameter)

## Summary of changes
- Make NeonBenchmarker use only records with`neon_benchmarker_` prefix
---
 test_runner/fixtures/benchmark_fixture.py | 23 +++++++++++++++++++----
 test_runner/fixtures/neon_fixtures.py     |  5 ++---
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/test_runner/fixtures/benchmark_fixture.py b/test_runner/fixtures/benchmark_fixture.py
index 038f557cc8..0c36cd6ef7 100644
--- a/test_runner/fixtures/benchmark_fixture.py
+++ b/test_runner/fixtures/benchmark_fixture.py
@@ -222,6 +222,8 @@ class NeonBenchmarker:
     function by the zenbenchmark fixture
     """
 
+    PROPERTY_PREFIX = "neon_benchmarker_"
+
     def __init__(self, property_recorder: Callable[[str, object], None]):
         # property recorder here is a pytest fixture provided by junitxml module
         # https://docs.pytest.org/en/6.2.x/reference.html#pytest.junitxml.record_property
@@ -238,7 +240,7 @@ class NeonBenchmarker:
         Record a benchmark result.
         """
         # just to namespace the value
-        name = f"neon_benchmarker_{metric_name}"
+        name = f"{self.PROPERTY_PREFIX}_{metric_name}"
         self.property_recorder(
             name,
             {
@@ -249,6 +251,18 @@ class NeonBenchmarker:
             },
         )
 
+    @classmethod
+    def records(
+        cls, user_properties: list[tuple[str, object]]
+    ) -> Iterator[tuple[str, dict[str, object]]]:
+        """
+        Yield all records related to benchmarks
+        """
+        for property_name, recorded_property in user_properties:
+            if property_name.startswith(cls.PROPERTY_PREFIX):
+                assert isinstance(recorded_property, dict)
+                yield recorded_property["name"], recorded_property
+
     @contextmanager
     def record_duration(self, metric_name: str) -> Iterator[None]:
         """
@@ -425,10 +439,11 @@ def zenbenchmark(
     yield benchmarker
 
     results = {}
-    for _, recorded_property in request.node.user_properties:
+    for _, recorded_property in NeonBenchmarker.records(request.node.user_properties):
         name = recorded_property["name"]
         value = str(recorded_property["value"])
-        if (unit := recorded_property["unit"].strip()) != "":
+        unit = str(recorded_property["unit"]).strip()
+        if unit != "":
             value += f" {unit}"
         results[name] = value
 
@@ -477,7 +492,7 @@ def pytest_terminal_summary(
     for test_report in terminalreporter.stats.get("passed", []):
         result_entry = []
 
-        for _, recorded_property in test_report.user_properties:
+        for _, recorded_property in NeonBenchmarker.records(test_report.user_properties):
             if not is_header_printed:
                 terminalreporter.section("Benchmark results", "-")
                 is_header_printed = True
diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py
index d98b2564df..c5fffc2af6 100644
--- a/test_runner/fixtures/neon_fixtures.py
+++ b/test_runner/fixtures/neon_fixtures.py
@@ -1441,6 +1441,7 @@ def neon_env_builder(
     pageserver_virtual_file_io_engine: str,
     pageserver_default_tenant_config_compaction_algorithm: Optional[Dict[str, Any]],
     pageserver_aux_file_policy: Optional[AuxFileStore],
+    record_property: Callable[[str, object], None],
 ) -> Iterator[NeonEnvBuilder]:
     """
     Fixture to create a Neon environment for test.
@@ -1480,9 +1481,7 @@ def neon_env_builder(
         yield builder
         # Propogate `preserve_database_files` to make it possible to use in other fixtures,
         # like `test_output_dir` fixture for attaching all database files to Allure report.
-        request.node.user_properties.append(
-            ("preserve_database_files", builder.preserve_database_files)
-        )
+        record_property("preserve_database_files", builder.preserve_database_files)
 
 
 @dataclass