From de1564b1dadd336800a6dc8a29038270d8990d74 Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Wed, 24 Jul 2024 13:25:57 +0200
Subject: [PATCH 01/19] Benchmarks: re-add dummy test module (#5)

But skipped by default
---
 qa/benchmarks/tests/conftest.py   | 32 ++++++++++++++++++++++----
 qa/benchmarks/tests/test_dummy.py | 38 +++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 4 deletions(-)
 create mode 100644 qa/benchmarks/tests/test_dummy.py

diff --git a/qa/benchmarks/tests/conftest.py b/qa/benchmarks/tests/conftest.py
index 271ffee..179e9a3 100644
--- a/qa/benchmarks/tests/conftest.py
+++ b/qa/benchmarks/tests/conftest.py
@@ -24,16 +24,40 @@ def pytest_addoption(parser):
         type=int,
         help="Only run random selected subset benchmarks.",
     )
+    parser.addoption(
+        "--dummy",
+        action="store_true",
+        help="Toggle to only run dummy benchmarks/tests (instead of skipping them)",
+    )
 
 
-def pytest_collection_modifyitems(session, config, items):
+def pytest_ignore_collect(collection_path, config):
     """
-    Pytest plugin to select a random subset of benchmarks to run.
+    Pytest hook to ignore certain directories/files during test collection.
+    """
+    # Note: there as some subtleties about the return values of this hook,
+    # which makes the logic slightly more complex than a naive approach would suggest:
+    # - `True` means to ignore the path,
+    # - `False` means to forcefully include it regardless of other plugins,
+    # - `None` means to keep it for now, but allow other plugins to still ignore.
+    dummy_mode = bool(config.getoption("--dummy"))
+    is_dummy_path = bool("dummy" in collection_path.name)
+    if dummy_mode and not is_dummy_path:
+        return True
+    elif not dummy_mode and is_dummy_path:
+        return True
+    else:
+        return None
 
-    based on https://alexwlchan.net/til/2024/run-random-subset-of-tests-in-pytest/
+
+def pytest_collection_modifyitems(session, config, items):
+    """
+    Pytest hook to filter/reorder collected test items.
     """
-    subset_size = config.getoption("--random-subset")
 
+    # Optionally, select a random subset of benchmarks to run.
+    # based on https://alexwlchan.net/til/2024/run-random-subset-of-tests-in-pytest/
+    subset_size = config.getoption("--random-subset")
     if subset_size >= 0:
         _log.warning(
             f"Selecting random subset of {subset_size} from {len(items)} benchmarks."
diff --git a/qa/benchmarks/tests/test_dummy.py b/qa/benchmarks/tests/test_dummy.py
new file mode 100644
index 0000000..41fb043
--- /dev/null
+++ b/qa/benchmarks/tests/test_dummy.py
@@ -0,0 +1,38 @@
+"""
+Dummy tests to to help with tooling development.
+
+Note that this test module will be skipped by default.
+Use the `--dummy` runtime option to run these tests
+and skip all other non-dummy tests.
+"""
+
+import pytest
+
+
+@pytest.mark.parametrize("y", [4, 5, 6])
+def test_tracking(track_metric, y):
+    x = 3
+    track_metric("x squared", x * x)
+    track_metric("y", y)
+    assert x + y == 8
+
+
+def test_simple_success():
+    x = 3
+    assert x + 5 == 8
+
+
+def test_simple_fail():
+    x = 3
+    assert x + 5 == "eight"
+
+
+def test_produce_files_success(tmp_path):
+    path = tmp_path / "hello.txt"
+    path.write_text("Hello, world.\n")
+
+
+def test_produce_files_fail(tmp_path):
+    path = tmp_path / "hello.txt"
+    path.write_text("Hello, world.\n")
+    assert 1 == 2

From 2d39be70c69362fb099cdf4a913c6f1813dd35fe Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Wed, 24 Jul 2024 13:27:25 +0200
Subject: [PATCH 02/19] Benchmarks: develop in dummy mode (to be reverted) #5

---
 .github/workflows/benchmarks.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.github/workflows/benchmarks.yaml b/.github/workflows/benchmarks.yaml
index 1a2d7b9..5095b23 100644
--- a/.github/workflows/benchmarks.yaml
+++ b/.github/workflows/benchmarks.yaml
@@ -23,7 +23,7 @@ jobs:
           cd qa/benchmarks
           mkdir report
           pytest \
-            --random-subset=1 \
+            --dummy \
             --html report/report.html --self-contained-html \
             --track-metrics-report=report/metrics.json
         env:

From b1bb730a3e8bad4b3ab8d473bd935dc215de43ea Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Wed, 24 Jul 2024 13:29:15 +0200
Subject: [PATCH 03/19] Issue #5 use `--basetemp` to have controlled output
 folder

---
 .github/workflows/benchmarks.yaml | 10 +++++++++-
 qa/benchmarks/.gitignore          |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/benchmarks.yaml b/.github/workflows/benchmarks.yaml
index 5095b23..c0680b0 100644
--- a/.github/workflows/benchmarks.yaml
+++ b/.github/workflows/benchmarks.yaml
@@ -22,13 +22,21 @@ jobs:
         run: |
           cd qa/benchmarks
           mkdir report
+          mkdir tmp_path_root
           pytest \
             --dummy \
             --html report/report.html --self-contained-html \
-            --track-metrics-report=report/metrics.json
+            --track-metrics-report=report/metrics.json \
+            --basetemp=tmp_path_root
         env:
           OPENEO_AUTH_METHOD: client_credentials
           OPENEO_AUTH_CLIENT_CREDENTIALS_CDSEFED: ${{ secrets.OPENEO_AUTH_CLIENT_CREDENTIALS_CDSEFED }}
+      - name: List local reports
+        if: always()
+        run: ls -alR qa/benchmarks/report
+      - name: List local results
+        if: always()
+        run: ls -alR qa/benchmarks/tmp_path_root
       - name: upload report
         uses: actions/upload-artifact@v4
         if: always()
diff --git a/qa/benchmarks/.gitignore b/qa/benchmarks/.gitignore
index 6c57698..999fb42 100644
--- a/qa/benchmarks/.gitignore
+++ b/qa/benchmarks/.gitignore
@@ -1 +1,2 @@
 report/
+tmp_path_root/

From 101d4ce280db5f4eb36c58cb0e1407add02c8e8a Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 00:24:01 +0200
Subject: [PATCH 04/19] Issue #5 initial pytest plugin impl to upload results
 on failure to s3

---
 .github/workflows/benchmarks.yaml             |   4 +
 qa/benchmarks/tests/conftest.py               |   1 +
 qa/benchmarks/tests/test_dummy.py             |   8 ++
 .../pytest_upload_assets.py                   | 118 ++++++++++++++++++
 qa/tools/pyproject.toml                       |   2 +
 5 files changed, 133 insertions(+)
 create mode 100644 qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py

diff --git a/.github/workflows/benchmarks.yaml b/.github/workflows/benchmarks.yaml
index c0680b0..2a4cf62 100644
--- a/.github/workflows/benchmarks.yaml
+++ b/.github/workflows/benchmarks.yaml
@@ -31,6 +31,10 @@ jobs:
         env:
           OPENEO_AUTH_METHOD: client_credentials
           OPENEO_AUTH_CLIENT_CREDENTIALS_CDSEFED: ${{ secrets.OPENEO_AUTH_CLIENT_CREDENTIALS_CDSEFED }}
+          UPLOAD_ASSETS_ENDPOINT_URL: "https://s3.waw3-1.cloudferro.com"
+          UPLOAD_ASSETS_BUCKET: "APEx-benchmarks"
+          UPLOAD_ASSETS_ACCESS_KEY_ID: ${{ secrets.UPLOAD_ASSETS_ACCESS_KEY_ID }}
+          UPLOAD_ASSETS_SECRET_ACCESS_KEY: ${{ secrets.UPLOAD_ASSETS_SECRET_ACCESS_KEY }}
       - name: List local reports
         if: always()
         run: ls -alR qa/benchmarks/report
diff --git a/qa/benchmarks/tests/conftest.py b/qa/benchmarks/tests/conftest.py
index 179e9a3..fa751b6 100644
--- a/qa/benchmarks/tests/conftest.py
+++ b/qa/benchmarks/tests/conftest.py
@@ -12,6 +12,7 @@
 
 pytest_plugins = [
     "apex_algorithm_qa_tools.pytest_track_metrics",
+    "apex_algorithm_qa_tools.pytest_upload_assets",
 ]
 
 
diff --git a/qa/benchmarks/tests/test_dummy.py b/qa/benchmarks/tests/test_dummy.py
index 41fb043..31306be 100644
--- a/qa/benchmarks/tests/test_dummy.py
+++ b/qa/benchmarks/tests/test_dummy.py
@@ -36,3 +36,11 @@ def test_produce_files_fail(tmp_path):
     path = tmp_path / "hello.txt"
     path.write_text("Hello, world.\n")
     assert 1 == 2
+
+
+@pytest.mark.parametrize("x", [3, 5])
+def test_upload_assets(tmp_path, upload_assets, x):
+    path = tmp_path / "hello.txt"
+    path.write_text("Hello, world.\n")
+    upload_assets(path)
+    assert x == 5
diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
new file mode 100644
index 0000000..a613395
--- /dev/null
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -0,0 +1,118 @@
+"""
+Pytest plugin to collect files generated during benchmark/test
+and upload them to S3 (e.g. on test failure).
+"""
+
+import logging
+import os
+import re
+import time
+from pathlib import Path
+from typing import Callable, Dict, Union
+
+import boto3
+import pytest
+
+_log = logging.getLogger(__name__)
+
+_PLUGIN_NAME = "upload_assets"
+
+
+def pytest_addoption(parser):
+    # TODO: option to inject github run id
+    # TODO: options for S3 bucket, credentials, ...
+    # TODO: option to always upload (also on success).
+    ...
+
+
+def pytest_configure(config: pytest.Config):
+    if (
+        # TODO only register if enough config is available for setup
+        # Don't register on xdist worker nodes
+        not hasattr(config, "workerinput")
+    ):
+        s3_client = boto3.client(
+            service_name="s3",
+            aws_access_key_id=os.environ.get("UPLOAD_ASSETS_ACCESS_KEY_ID"),
+            aws_secret_access_key=os.environ.get("UPLOAD_ASSETS_SECRET_ACCESS_KEY"),
+            # TODO Option for endpoint url
+            endpoint_url=os.environ.get("UPLOAD_ASSETS_ENDPOINT_URL"),
+        )
+        bucket = os.environ.get("UPLOAD_ASSETS_BUCKET")
+        # TODO: do run id through option
+        if os.environ.get("GITHUB_RUN_ID"):
+            run_id = "github-" + os.environ["GITHUB_RUN_ID"]
+        else:
+            run_id = f"local-{int(time.time())}"
+
+        config.pluginmanager.register(
+            S3UploadPlugin(
+                run_id=run_id,
+                s3_client=s3_client,
+                bucket=bucket,
+            ),
+            name=_PLUGIN_NAME,
+        )
+
+
+class _Collector:
+    """
+    Collects test outcomes and files to upload for a single test node.
+    """
+
+    def __init__(self, nodeid: str) -> None:
+        self.nodeid = nodeid
+        self.outcomes: Dict[str, str] = {}
+        self.assets: Dict[str, Path] = {}
+
+    def set_outcome(self, when: str, outcome: str):
+        self.outcomes[when] = outcome
+
+    def collect(self, path: Path, name: str):
+        self.assets[name] = path
+
+
+class S3UploadPlugin:
+    def __init__(self, run_id: str, s3_client, bucket: str) -> None:
+        # TODO: bucket, credentials, githubrunid, ...
+        self.run_id = run_id
+        self.collector: Union[_Collector, None] = None
+        self.s3_client = s3_client
+        self.bucket = bucket
+
+    def pytest_runtest_logstart(self, nodeid, location):
+        self.collector = _Collector(nodeid=nodeid)
+
+    def pytest_runtest_logreport(self, report: pytest.TestReport):
+        self.collector.set_outcome(when=report.when, outcome=report.outcome)
+
+    def pytest_runtest_logfinish(self, nodeid, location):
+        # TODO: option to also upload on success?
+        if self.collector.outcomes.get("call") == "failed":
+            self._upload(self.collector)
+
+        self.collector = None
+
+    def _upload(self, collector: _Collector):
+        for name, path in collector.assets.items():
+            nodeid = re.sub(r"[^a-zA-Z0-9_.-]", "_", collector.nodeid)
+            key = f"{self.run_id}!{nodeid}!{name}"
+            # TODO: get upload info in report?
+            _log.info(f"Uploading {path} to {self.bucket}/{key}")
+            self.s3_client.upload_file(Filename=str(path), Bucket=self.bucket, Key=key)
+
+
+@pytest.fixture
+def upload_assets(pytestconfig, tmp_path) -> Callable[[Path], None]:
+    """
+    Fixture to register a file (under `tmp_path`) for S3 upload
+    after the test failed.
+    """
+    uploader = pytestconfig.pluginmanager.get_plugin(_PLUGIN_NAME)
+
+    def collect(path: Path):
+        assert path.is_relative_to(tmp_path)
+        name = str(path.relative_to(tmp_path))
+        uploader.collector.collect(path=path, name=name)
+
+    return collect
diff --git a/qa/tools/pyproject.toml b/qa/tools/pyproject.toml
index 0e7fd75..f843233 100644
--- a/qa/tools/pyproject.toml
+++ b/qa/tools/pyproject.toml
@@ -21,4 +21,6 @@ dependencies = [
     "requests>=2.32.0",
     "jsonschema>=4.0.0",
     "openeo>=0.30.0",
+    # TODO: make some of these dependencies optional
+    "boto3>=1.34.0",
 ]

From 14c7444ee6b2a5780141dd3ac51bb719ad07432e Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 00:36:40 +0200
Subject: [PATCH 05/19] Issue #5 apply `upload_assets` on real benchmark

---
 .github/workflows/benchmarks.yaml                  |  3 ++-
 qa/benchmarks/tests/test_benchmarks.py             | 14 +++++++++++---
 .../pytest_upload_assets.py                        |  9 +++++----
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/.github/workflows/benchmarks.yaml b/.github/workflows/benchmarks.yaml
index 2a4cf62..8890f14 100644
--- a/.github/workflows/benchmarks.yaml
+++ b/.github/workflows/benchmarks.yaml
@@ -24,7 +24,8 @@ jobs:
           mkdir report
           mkdir tmp_path_root
           pytest \
-            --dummy \
+            --random-subset=1 \
+            -k max_ndvi_fail_intentionally \
             --html report/report.html --self-contained-html \
             --track-metrics-report=report/metrics.json \
             --basetemp=tmp_path_root
diff --git a/qa/benchmarks/tests/test_benchmarks.py b/qa/benchmarks/tests/test_benchmarks.py
index 0b952d0..1c93e80 100644
--- a/qa/benchmarks/tests/test_benchmarks.py
+++ b/qa/benchmarks/tests/test_benchmarks.py
@@ -19,7 +19,11 @@
     ],
 )
 def test_run_benchmark(
-    scenario: BenchmarkScenario, connection_factory, tmp_path: Path, track_metric
+    scenario: BenchmarkScenario,
+    connection_factory,
+    tmp_path: Path,
+    track_metric,
+    upload_assets,
 ):
     connection: openeo.Connection = connection_factory(url=scenario.backend)
 
@@ -39,8 +43,12 @@ def test_run_benchmark(
 
     # Download actual results
     actual_dir = tmp_path / "actual"
-    job.get_results().download_files(target=actual_dir, include_stac_metadata=True)
-    # TODO: upload actual results to somewhere?
+    paths = job.get_results().download_files(
+        target=actual_dir, include_stac_metadata=True
+    )
+
+    # Upload assets on failure
+    upload_assets(*paths)
 
     # Compare actual results with reference data
     reference_dir = download_reference_data(
diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
index a613395..ee82994 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -110,9 +110,10 @@ def upload_assets(pytestconfig, tmp_path) -> Callable[[Path], None]:
     """
     uploader = pytestconfig.pluginmanager.get_plugin(_PLUGIN_NAME)
 
-    def collect(path: Path):
-        assert path.is_relative_to(tmp_path)
-        name = str(path.relative_to(tmp_path))
-        uploader.collector.collect(path=path, name=name)
+    def collect(*paths: Path):
+        for path in paths:
+            assert path.is_relative_to(tmp_path)
+            name = str(path.relative_to(tmp_path))
+            uploader.collector.collect(path=path, name=name)
 
     return collect

From f20e9b79fd516cd281a1e5c1c06d5b7a202993d9 Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 10:08:07 +0200
Subject: [PATCH 06/19] upload_assets: add option for run id (#5)

---
 .github/workflows/benchmarks.yaml             |  3 ++-
 .../pytest_upload_assets.py                   | 23 ++++++++-----------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/.github/workflows/benchmarks.yaml b/.github/workflows/benchmarks.yaml
index 8890f14..68e4eb5 100644
--- a/.github/workflows/benchmarks.yaml
+++ b/.github/workflows/benchmarks.yaml
@@ -28,7 +28,8 @@ jobs:
             -k max_ndvi_fail_intentionally \
             --html report/report.html --self-contained-html \
             --track-metrics-report=report/metrics.json \
-            --basetemp=tmp_path_root
+            --basetemp=tmp_path_root \
+            --upload-assets-runid="gh-$GITHUB_RUN_ID"
         env:
           OPENEO_AUTH_METHOD: client_credentials
           OPENEO_AUTH_CLIENT_CREDENTIALS_CDSEFED: ${{ secrets.OPENEO_AUTH_CLIENT_CREDENTIALS_CDSEFED }}
diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
index ee82994..0bdab2a 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -6,7 +6,7 @@
 import logging
 import os
 import re
-import time
+import uuid
 from pathlib import Path
 from typing import Callable, Dict, Union
 
@@ -19,10 +19,14 @@
 
 
 def pytest_addoption(parser):
-    # TODO: option to inject github run id
     # TODO: options for S3 bucket, credentials, ...
     # TODO: option to always upload (also on success).
-    ...
+    parser.addoption(
+        "--upload-assets-runid",
+        metavar="ID",
+        action="store",
+        help="The run ID to use for building the S3 key.",
+    )
 
 
 def pytest_configure(config: pytest.Config):
@@ -39,15 +43,9 @@ def pytest_configure(config: pytest.Config):
             endpoint_url=os.environ.get("UPLOAD_ASSETS_ENDPOINT_URL"),
         )
         bucket = os.environ.get("UPLOAD_ASSETS_BUCKET")
-        # TODO: do run id through option
-        if os.environ.get("GITHUB_RUN_ID"):
-            run_id = "github-" + os.environ["GITHUB_RUN_ID"]
-        else:
-            run_id = f"local-{int(time.time())}"
-
         config.pluginmanager.register(
             S3UploadPlugin(
-                run_id=run_id,
+                run_id=config.getoption("upload_assets_runid"),
                 s3_client=s3_client,
                 bucket=bucket,
             ),
@@ -73,9 +71,8 @@ def collect(self, path: Path, name: str):
 
 
 class S3UploadPlugin:
-    def __init__(self, run_id: str, s3_client, bucket: str) -> None:
-        # TODO: bucket, credentials, githubrunid, ...
-        self.run_id = run_id
+    def __init__(self, *, run_id: str | None = None, s3_client, bucket: str) -> None:
+        self.run_id = run_id or uuid.uuid4().hex
         self.collector: Union[_Collector, None] = None
         self.s3_client = s3_client
         self.bucket = bucket

From c013efaa233530c97ff1ed27599db7a014e1e5ce Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 10:20:14 +0200
Subject: [PATCH 07/19] upload_assets: upload with "public-read" ACL (#5)

---
 qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
index 0bdab2a..5758b90 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -96,7 +96,13 @@ def _upload(self, collector: _Collector):
             key = f"{self.run_id}!{nodeid}!{name}"
             # TODO: get upload info in report?
             _log.info(f"Uploading {path} to {self.bucket}/{key}")
-            self.s3_client.upload_file(Filename=str(path), Bucket=self.bucket, Key=key)
+            self.s3_client.upload_file(
+                Filename=str(path),
+                Bucket=self.bucket,
+                Key=key,
+                # TODO: option to override ACL, or ExtraArgs in general?
+                ExtraArgs={"ACL": "public-read"},
+            )
 
 
 @pytest.fixture

From b9712b2ca4966782d2595773520242d8daf61da7 Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 10:21:36 +0200
Subject: [PATCH 08/19] upload_assets: use options instead of env vars for
 endpoint URL and bucket

---
 .github/workflows/benchmarks.yaml             |  6 ++--
 .../pytest_upload_assets.py                   | 33 ++++++++++++-------
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/.github/workflows/benchmarks.yaml b/.github/workflows/benchmarks.yaml
index 68e4eb5..aad2c3c 100644
--- a/.github/workflows/benchmarks.yaml
+++ b/.github/workflows/benchmarks.yaml
@@ -29,12 +29,12 @@ jobs:
             --html report/report.html --self-contained-html \
             --track-metrics-report=report/metrics.json \
             --basetemp=tmp_path_root \
-            --upload-assets-runid="gh-$GITHUB_RUN_ID"
+            --upload-assets-run-id="gh-$GITHUB_RUN_ID" \
+            --upload-assets-endpoint-url="https://s3.waw3-1.cloudferro.com" \
+            --upload-assets-bucket="APEx-benchmarks"
         env:
           OPENEO_AUTH_METHOD: client_credentials
           OPENEO_AUTH_CLIENT_CREDENTIALS_CDSEFED: ${{ secrets.OPENEO_AUTH_CLIENT_CREDENTIALS_CDSEFED }}
-          UPLOAD_ASSETS_ENDPOINT_URL: "https://s3.waw3-1.cloudferro.com"
-          UPLOAD_ASSETS_BUCKET: "APEx-benchmarks"
           UPLOAD_ASSETS_ACCESS_KEY_ID: ${{ secrets.UPLOAD_ASSETS_ACCESS_KEY_ID }}
           UPLOAD_ASSETS_SECRET_ACCESS_KEY: ${{ secrets.UPLOAD_ASSETS_SECRET_ACCESS_KEY }}
       - name: List local reports
diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
index 5758b90..5eed62e 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -19,36 +19,45 @@
 
 
 def pytest_addoption(parser):
-    # TODO: options for S3 bucket, credentials, ...
     # TODO: option to always upload (also on success).
     parser.addoption(
-        "--upload-assets-runid",
+        "--upload-assets-run-id",
         metavar="ID",
         action="store",
         help="The run ID to use for building the S3 key.",
     )
+    parser.addoption(
+        "--upload-assets-endpoint-url",
+        metavar="URL",
+        action="store",
+        help="The S3 endpoint URL to upload to.",
+    )
+    parser.addoption(
+        "--upload-assets-bucket",
+        metavar="BUCKET",
+        action="store",
+        help="The S3 bucket to upload to.",
+    )
 
 
 def pytest_configure(config: pytest.Config):
+    run_id = config.getoption("upload_assets_run_id")
+    endpoint_url = config.getoption("upload_assets_endpoint_url")
+    bucket = config.getoption("upload_assets_bucket")
     if (
-        # TODO only register if enough config is available for setup
+        endpoint_url
+        and bucket
         # Don't register on xdist worker nodes
-        not hasattr(config, "workerinput")
+        and not hasattr(config, "workerinput")
     ):
         s3_client = boto3.client(
             service_name="s3",
             aws_access_key_id=os.environ.get("UPLOAD_ASSETS_ACCESS_KEY_ID"),
             aws_secret_access_key=os.environ.get("UPLOAD_ASSETS_SECRET_ACCESS_KEY"),
-            # TODO Option for endpoint url
-            endpoint_url=os.environ.get("UPLOAD_ASSETS_ENDPOINT_URL"),
+            endpoint_url=endpoint_url,
         )
-        bucket = os.environ.get("UPLOAD_ASSETS_BUCKET")
         config.pluginmanager.register(
-            S3UploadPlugin(
-                run_id=config.getoption("upload_assets_runid"),
-                s3_client=s3_client,
-                bucket=bucket,
-            ),
+            S3UploadPlugin(run_id=run_id, s3_client=s3_client, bucket=bucket),
             name=_PLUGIN_NAME,
         )
 

From b8e52735f5b0c8f4dcf245191d662b8f0ed726fb Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 11:56:16 +0200
Subject: [PATCH 09/19] Issue #5/#7 simplify `pytest_addoption` handling a bit

Let defaults do their thing
---
 .../pytest_track_metrics.py                    | 18 +++++++-----------
 .../pytest_upload_assets.py                    | 13 +++++--------
 2 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py b/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py
index e81ae42..78ef8d3 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py
@@ -31,23 +31,19 @@ def test_dummy(track_metric):
 
 import pytest
 
-_TRACK_METRICS_PATH = "track_metrics_path"
-_TRACK_METRICS_NAME = "track_metrics"
+_TRACK_METRICS_PLUGIN_NAME = "track_metrics"
 
 
-def pytest_addoption(parser):
+def pytest_addoption(parser: pytest.Parser):
     parser.addoption(
         "--track-metrics-report",
         metavar="PATH",
-        action="store",
-        dest=_TRACK_METRICS_PATH,
-        default=None,
         help="Path to JSON file to store test/benchmark metrics.",
     )
 
 
 def pytest_configure(config):
-    track_metrics_path = config.getoption(_TRACK_METRICS_PATH)
+    track_metrics_path = config.getoption("track_metrics_report")
     if (
         track_metrics_path
         # Don't register on xdist worker nodes
@@ -55,13 +51,13 @@ def pytest_configure(config):
     ):
         config.pluginmanager.register(
             TrackMetricsReporter(path=track_metrics_path),
-            name=_TRACK_METRICS_NAME,
+            name=_TRACK_METRICS_PLUGIN_NAME,
         )
 
 
 def pytest_unconfigure(config):
-    if config.pluginmanager.hasplugin(_TRACK_METRICS_NAME):
-        config.pluginmanager.unregister(name=_TRACK_METRICS_NAME)
+    if config.pluginmanager.hasplugin(_TRACK_METRICS_PLUGIN_NAME):
+        config.pluginmanager.unregister(name=_TRACK_METRICS_PLUGIN_NAME)
 
 
 class TrackMetricsReporter:
@@ -122,7 +118,7 @@ def track_metric(
     """
 
     reporter: Union[TrackMetricsReporter, None] = pytestconfig.pluginmanager.get_plugin(
-        _TRACK_METRICS_NAME
+        _TRACK_METRICS_PLUGIN_NAME
     )
 
     if reporter:
diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
index 5eed62e..b5c05e6 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -15,27 +15,24 @@
 
 _log = logging.getLogger(__name__)
 
-_PLUGIN_NAME = "upload_assets"
+_UPLOAD_ASSETS_PLUGIN_NAME = "upload_assets"
 
 
-def pytest_addoption(parser):
+def pytest_addoption(parser: pytest.Parser):
     # TODO: option to always upload (also on success).
     parser.addoption(
         "--upload-assets-run-id",
-        metavar="ID",
-        action="store",
+        metavar="RUNID",
         help="The run ID to use for building the S3 key.",
     )
     parser.addoption(
         "--upload-assets-endpoint-url",
         metavar="URL",
-        action="store",
         help="The S3 endpoint URL to upload to.",
     )
     parser.addoption(
         "--upload-assets-bucket",
         metavar="BUCKET",
-        action="store",
         help="The S3 bucket to upload to.",
     )
 
@@ -58,7 +55,7 @@ def pytest_configure(config: pytest.Config):
         )
         config.pluginmanager.register(
             S3UploadPlugin(run_id=run_id, s3_client=s3_client, bucket=bucket),
-            name=_PLUGIN_NAME,
+            name=_UPLOAD_ASSETS_PLUGIN_NAME,
         )
 
 
@@ -120,7 +117,7 @@ def upload_assets(pytestconfig, tmp_path) -> Callable[[Path], None]:
     Fixture to register a file (under `tmp_path`) for S3 upload
     after the test failed.
     """
-    uploader = pytestconfig.pluginmanager.get_plugin(_PLUGIN_NAME)
+    uploader = pytestconfig.pluginmanager.get_plugin(_UPLOAD_ASSETS_PLUGIN_NAME)
 
     def collect(*paths: Path):
         for path in paths:

From 49903617857dbb8a63c17f3bb245d4398aef07c2 Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 13:03:02 +0200
Subject: [PATCH 10/19] Finetune/synchronize configuration of
 `track_metrics`/`upload_assets` (#5/#7)

---
 .../pytest_track_metrics.py                   | 24 +++---
 .../pytest_upload_assets.py                   | 73 +++++++++++++++----
 2 files changed, 74 insertions(+), 23 deletions(-)

diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py b/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py
index 78ef8d3..8216134 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py
@@ -43,18 +43,26 @@ def pytest_addoption(parser: pytest.Parser):
 
 
 def pytest_configure(config):
+    if hasattr(config, "workerinput"):
+        warnings.warn("`track_metrics` plugin is not supported on xdist worker nodes.")
+        return
+
     track_metrics_path = config.getoption("track_metrics_report")
-    if (
-        track_metrics_path
-        # Don't register on xdist worker nodes
-        and not hasattr(config, "workerinput")
-    ):
+    if track_metrics_path:
         config.pluginmanager.register(
             TrackMetricsReporter(path=track_metrics_path),
             name=_TRACK_METRICS_PLUGIN_NAME,
         )
 
 
+def pytest_report_header(config):
+    plugin: TrackMetricsReporter | None = config.pluginmanager.get_plugin(
+        _TRACK_METRICS_PLUGIN_NAME
+    )
+    if plugin:
+        return f"Plugin `track_metrics` is active, reporting to {plugin.path}"
+
+
 def pytest_unconfigure(config):
     if config.pluginmanager.hasplugin(_TRACK_METRICS_PLUGIN_NAME):
         config.pluginmanager.unregister(name=_TRACK_METRICS_PLUGIN_NAME)
@@ -117,7 +125,7 @@ def track_metric(
     Returns a callable that expects a metric name and value
     """
 
-    reporter: Union[TrackMetricsReporter, None] = pytestconfig.pluginmanager.get_plugin(
+    reporter: TrackMetricsReporter | None = pytestconfig.pluginmanager.get_plugin(
         _TRACK_METRICS_PLUGIN_NAME
     )
 
@@ -126,9 +134,7 @@ def track_metric(
         def append(name: str, value: Any):
             reporter.get_metrics(request.node.user_properties).append((name, value))
     else:
-        warnings.warn(
-            "The `track_metric` fixture is requested, but no output file is defined (e.g. with `--metrics-tracker-report=path/to/metrics.json`."
-        )
+        warnings.warn("Fixture `track_metric` is a no-op (incomplete set up).")
 
         def append(name: str, value: Any):
             pass
diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
index b5c05e6..3f6fa03 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -1,12 +1,37 @@
 """
 Pytest plugin to collect files generated during benchmark/test
 and upload them to S3 (e.g. on test failure).
+
+Usage:
+
+-  Enable the plugin in `conftest.py`:
+
+    ```python
+    pytest_plugins = [
+        "apex_algorithm_qa_tools.pytest_upload_assets",
+    ]
+
+-  Use the `upload_assets` fixture to register files for upload:
+
+    ```python
+    def test_dummy(upload_assets, tmp_path):
+        path = tmp_path / "dummy.txt"
+        path.write_text("dummy content")
+        upload_assets(path)
+    ```
+
+- Run the tests with:
+    - `--upload-assets-run-id=RUNID` (optional, defaults to random UUID)
+    - `--upload-assets-endpoint-url=URL`
+    - `--upload-assets-bucket=BUCKET`
+    - and env vars `UPLOAD_ASSETS_ACCESS_KEY_ID` and `UPLOAD_ASSETS_SECRET_ACCESS_KEY` set.
 """
 
 import logging
 import os
 import re
 import uuid
+import warnings
 from pathlib import Path
 from typing import Callable, Dict, Union
 
@@ -41,12 +66,7 @@ def pytest_configure(config: pytest.Config):
     run_id = config.getoption("upload_assets_run_id")
     endpoint_url = config.getoption("upload_assets_endpoint_url")
     bucket = config.getoption("upload_assets_bucket")
-    if (
-        endpoint_url
-        and bucket
-        # Don't register on xdist worker nodes
-        and not hasattr(config, "workerinput")
-    ):
+    if endpoint_url and bucket:
         s3_client = boto3.client(
             service_name="s3",
             aws_access_key_id=os.environ.get("UPLOAD_ASSETS_ACCESS_KEY_ID"),
@@ -59,6 +79,19 @@ def pytest_configure(config: pytest.Config):
         )
 
 
+def pytest_report_header(config):
+    plugin: S3UploadPlugin | None = config.pluginmanager.get_plugin(
+        _UPLOAD_ASSETS_PLUGIN_NAME
+    )
+    if plugin:
+        return f"Plugin `upload_assets` is active, with upload to {plugin.bucket!r}"
+
+
+def pytest_unconfigure(config):
+    if config.pluginmanager.hasplugin(_UPLOAD_ASSETS_PLUGIN_NAME):
+        config.pluginmanager.unregister(name=_UPLOAD_ASSETS_PLUGIN_NAME)
+
+
 class _Collector:
     """
     Collects test outcomes and files to upload for a single test node.
@@ -112,17 +145,29 @@ def _upload(self, collector: _Collector):
 
 
 @pytest.fixture
-def upload_assets(pytestconfig, tmp_path) -> Callable[[Path], None]:
+def upload_assets(pytestconfig: pytest.Config, tmp_path) -> Callable:
     """
     Fixture to register a file (under `tmp_path`) for S3 upload
-    after the test failed.
+    after the test failed. The fixture is a function that
+    can be called with one or more `Path` objects to upload.
     """
-    uploader = pytestconfig.pluginmanager.get_plugin(_UPLOAD_ASSETS_PLUGIN_NAME)
+    uploader: S3UploadPlugin | None = pytestconfig.pluginmanager.get_plugin(
+        _UPLOAD_ASSETS_PLUGIN_NAME
+    )
+
+    if uploader:
+
+        def collect(*paths: Path):
+            for path in paths:
+                # TODO: option to make relative from other root
+                #       (e.g. when test uses an `actual` folder for actual results)
+                assert path.is_relative_to(tmp_path)
+                name = str(path.relative_to(tmp_path))
+                uploader.collector.collect(path=path, name=name)
+    else:
+        warnings.warn("Fixture `upload_assets` is a no-op (incomplete set up).")
 
-    def collect(*paths: Path):
-        for path in paths:
-            assert path.is_relative_to(tmp_path)
-            name = str(path.relative_to(tmp_path))
-            uploader.collector.collect(path=path, name=name)
+        def collect(*paths: Path):
+            pass
 
     return collect

From e3160f24266a7c07b86e90088d6a92319141f77a Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 15:43:47 +0200
Subject: [PATCH 11/19] upload_assets: refactor to something simpler and add
 initial tests (#5)

---
 .../pytest_upload_assets.py                   |  77 ++++++------
 qa/unittests/requirements.txt                 |   1 +
 .../tests/test_pytest_upload_assets.py        | 110 ++++++++++++++++++
 3 files changed, 149 insertions(+), 39 deletions(-)
 create mode 100644 qa/unittests/tests/test_pytest_upload_assets.py

diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
index 3f6fa03..7e78067 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -15,8 +15,8 @@
 
     ```python
     def test_dummy(upload_assets, tmp_path):
-        path = tmp_path / "dummy.txt"
-        path.write_text("dummy content")
+        path = tmp_path / "hello.txt"
+        path.write_text("Hello world.")
         upload_assets(path)
     ```
 
@@ -33,7 +33,7 @@ def test_dummy(upload_assets, tmp_path):
 import uuid
 import warnings
 from pathlib import Path
-from typing import Callable, Dict, Union
+from typing import Callable, Dict
 
 import boto3
 import pytest
@@ -80,6 +80,7 @@ def pytest_configure(config: pytest.Config):
 
 
 def pytest_report_header(config):
+    # TODO Move inside S3UploadPlugin
     plugin: S3UploadPlugin | None = config.pluginmanager.get_plugin(
         _UPLOAD_ASSETS_PLUGIN_NAME
     )
@@ -92,49 +93,42 @@ def pytest_unconfigure(config):
         config.pluginmanager.unregister(name=_UPLOAD_ASSETS_PLUGIN_NAME)
 
 
-class _Collector:
-    """
-    Collects test outcomes and files to upload for a single test node.
-    """
-
-    def __init__(self, nodeid: str) -> None:
-        self.nodeid = nodeid
-        self.outcomes: Dict[str, str] = {}
-        self.assets: Dict[str, Path] = {}
-
-    def set_outcome(self, when: str, outcome: str):
-        self.outcomes[when] = outcome
-
-    def collect(self, path: Path, name: str):
-        self.assets[name] = path
-
-
 class S3UploadPlugin:
     def __init__(self, *, run_id: str | None = None, s3_client, bucket: str) -> None:
         self.run_id = run_id or uuid.uuid4().hex
-        self.collector: Union[_Collector, None] = None
+        self.collected_assets: Dict[str, Path] | None = None
         self.s3_client = s3_client
         self.bucket = bucket
+        self.upload_stats = {"uploaded": 0}
 
-    def pytest_runtest_logstart(self, nodeid, location):
-        self.collector = _Collector(nodeid=nodeid)
-
-    def pytest_runtest_logreport(self, report: pytest.TestReport):
-        self.collector.set_outcome(when=report.when, outcome=report.outcome)
-
-    def pytest_runtest_logfinish(self, nodeid, location):
-        # TODO: option to also upload on success?
-        if self.collector.outcomes.get("call") == "failed":
-            self._upload(self.collector)
+    def collect(self, path: Path, name: str):
+        """Collect assets to upload"""
+        assert self.collected_assets is not None, "No active collection of assets"
+        self.collected_assets[name] = path
 
-        self.collector = None
+    def pytest_runtest_logstart(self, nodeid):
+        # Start new collection of assets for current test node
+        self.collected_assets = {}
 
-    def _upload(self, collector: _Collector):
-        for name, path in collector.assets.items():
-            nodeid = re.sub(r"[^a-zA-Z0-9_.-]", "_", collector.nodeid)
-            key = f"{self.run_id}!{nodeid}!{name}"
-            # TODO: get upload info in report?
-            _log.info(f"Uploading {path} to {self.bucket}/{key}")
+    def pytest_runtest_logreport(self, report: pytest.TestReport):
+        # TODO: option to upload on other outcome as well?
+        if report.when == "call" and report.outcome == "failed":
+            # TODO: what to do when upload fails?
+            uploaded = self._upload(nodeid=report.nodeid)
+            # TODO: report the uploaded assets somewhere (e.g. in user_properties or JSON report?)
+
+    def pytest_runtest_logfinish(self, nodeid):
+        # Reset collection of assets
+        self.collected_assets = None
+
+    def _upload(self, nodeid: str) -> Dict[str, str]:
+        assets = {}
+        for name, path in self.collected_assets.items():
+            safe_nodeid = re.sub(r"[^a-zA-Z0-9_.-]", "_", nodeid)
+            key = f"{self.run_id}!{safe_nodeid}!{name}"
+            # TODO: is this manual URL building correct and isn't there a boto utility for that?
+            url = f"{self.s3_client.meta.endpoint_url.rstrip('/')}/{self.bucket}/{key}"
+            _log.info(f"Uploading {path} to {url}")
             self.s3_client.upload_file(
                 Filename=str(path),
                 Bucket=self.bucket,
@@ -142,6 +136,11 @@ def _upload(self, collector: _Collector):
                 # TODO: option to override ACL, or ExtraArgs in general?
                 ExtraArgs={"ACL": "public-read"},
             )
+            assets[name] = url
+            self.upload_stats["uploaded"] += 1
+
+    def pytest_terminal_summary(self, terminalreporter):
+        terminalreporter.write_sep("-", f"`upload_assets` stats: {self.upload_stats}")
 
 
 @pytest.fixture
@@ -163,7 +162,7 @@ def collect(*paths: Path):
                 #       (e.g. when test uses an `actual` folder for actual results)
                 assert path.is_relative_to(tmp_path)
                 name = str(path.relative_to(tmp_path))
-                uploader.collector.collect(path=path, name=name)
+                uploader.collect(path=path, name=name)
     else:
         warnings.warn("Fixture `upload_assets` is a no-op (incomplete set up).")
 
diff --git a/qa/unittests/requirements.txt b/qa/unittests/requirements.txt
index da3d4b1..a371f5e 100644
--- a/qa/unittests/requirements.txt
+++ b/qa/unittests/requirements.txt
@@ -1,2 +1,3 @@
 apex-algorithm-qa-tools
 pytest>=8.2.0
+moto[s3, server]
diff --git a/qa/unittests/tests/test_pytest_upload_assets.py b/qa/unittests/tests/test_pytest_upload_assets.py
new file mode 100644
index 0000000..82c25bf
--- /dev/null
+++ b/qa/unittests/tests/test_pytest_upload_assets.py
@@ -0,0 +1,110 @@
+import uuid
+
+import boto3
+import moto.server
+import pytest
+
+
+@pytest.fixture(scope="module")
+def moto_server() -> str:
+    """Fixture to run a mocked AWS server for testing."""
+    server = moto.server.ThreadedMotoServer()
+    server.start()
+    # TODO: avoid hardcoded port (5000)
+    yield "http://localhost:5000"
+    server.stop()
+
+
+@pytest.fixture(autouse=True)
+def aws_credentials(monkeypatch):
+    monkeypatch.setenv("AWS_ACCESS_KEY_ID", "test123")
+    monkeypatch.setenv("AWS_SECRET_ACCESS_KEY", "test456")
+
+
+@pytest.fixture
+def s3_client(moto_server):
+    return boto3.client("s3", endpoint_url=moto_server)
+
+
+@pytest.fixture
+def s3_bucket(s3_client) -> str:
+    # Unique bucket name for test isolation
+    bucket = f"test-bucket-{uuid.uuid4().hex}"
+    s3_client.create_bucket(Bucket=bucket)
+    return bucket
+
+
+def test_basic_upload_on_fail(
+    pytester: pytest.Pytester, moto_server, s3_client, s3_bucket
+):
+    pytester.makeconftest(
+        """
+        pytest_plugins = [
+            "apex_algorithm_qa_tools.pytest_upload_assets",
+        ]
+        """
+    )
+    pytester.makepyfile(
+        test_file_maker="""
+            def test_fail_and_upload(upload_assets, tmp_path):
+                path = tmp_path / "hello.txt"
+                path.write_text("Hello world.")
+                upload_assets(path)
+                assert 3 == 5
+        """
+    )
+
+    run_result = pytester.runpytest_subprocess(
+        "--upload-assets-run-id=test-run-123",
+        f"--upload-assets-endpoint-url={moto_server}",
+        f"--upload-assets-bucket={s3_bucket}",
+    )
+    run_result.stdout.re_match_lines(
+        [r"Plugin `upload_assets` is active, with upload to 'test-bucket-"]
+    )
+    run_result.assert_outcomes(failed=1)
+
+    object_listing = s3_client.list_objects(Bucket=s3_bucket)
+    assert len(object_listing["Contents"])
+    keys = [obj["Key"] for obj in object_listing["Contents"]]
+    expected_key = "test-run-123!test_file_maker.py__test_fail_and_upload!hello.txt"
+    assert keys == [expected_key]
+
+    actual = s3_client.get_object(Bucket=s3_bucket, Key=expected_key)
+    assert actual["Body"].read().decode("utf8") == "Hello world."
+
+    run_result.stdout.re_match_lines([r".*`upload_assets` stats: \{'uploaded': 1\}"])
+
+
+def test_nop_on_success(pytester: pytest.Pytester, moto_server, s3_client, s3_bucket):
+    pytester.makeconftest(
+        """
+        pytest_plugins = [
+            "apex_algorithm_qa_tools.pytest_upload_assets",
+        ]
+        """
+    )
+    pytester.makepyfile(
+        test_file_maker="""
+            def test_success(upload_assets, tmp_path):
+                path = tmp_path / "hello.txt"
+                path.write_text("Hello world.")
+                upload_assets(path)
+                assert 3 == 3
+        """
+    )
+
+    run_result = pytester.runpytest_subprocess(
+        "--upload-assets-run-id=test-run-123",
+        f"--upload-assets-endpoint-url={moto_server}",
+        f"--upload-assets-bucket={s3_bucket}",
+    )
+    run_result.stdout.re_match_lines(
+        [r"Plugin `upload_assets` is active, with upload to 'test-bucket-"]
+    )
+    run_result.assert_outcomes(passed=1)
+
+    object_listing = s3_client.list_objects(Bucket=s3_bucket)
+    assert object_listing.get("Contents", []) == []
+
+    run_result.stdout.re_match_lines([r".*`upload_assets` stats: \{'uploaded': 0\}"])

From 2fb14cdb5b7aee47b689282a9ff4643a3eafbfbf Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 16:56:29 +0200
Subject: [PATCH 12/19] upload_assets: update TODO note about always-upload
 feature #5 #22

---
 qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
index 7e78067..d49fa0e 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -44,7 +44,7 @@ def test_dummy(upload_assets, tmp_path):
 
 
 def pytest_addoption(parser: pytest.Parser):
-    # TODO: option to always upload (also on success).
+    # TODO #22: option to always upload (also on success).
     parser.addoption(
         "--upload-assets-run-id",
         metavar="RUNID",
@@ -111,7 +111,7 @@ def pytest_runtest_logstart(self, nodeid):
         self.collected_assets = {}
 
     def pytest_runtest_logreport(self, report: pytest.TestReport):
-        # TODO: option to upload on other outcome as well?
+        # TODO #22: option to upload on other outcome as well?
         if report.when == "call" and report.outcome == "failed":
             # TODO: what to do when upload fails?
             uploaded = self._upload(nodeid=report.nodeid)

From 4eb026ac987a2eccb5e533bece86eb61b4f7efd7 Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 17:18:14 +0200
Subject: [PATCH 13/19] track_metrics/upload_assets: simplify hook handling

---
 .../pytest_track_metrics.py                     | 16 +++-------------
 .../pytest_upload_assets.py                     | 17 +++--------------
 qa/unittests/tests/test_pytest_track_metrics.py | 12 +++++++++---
 3 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py b/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py
index 8216134..f7ba99f 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_track_metrics.py
@@ -55,19 +55,6 @@ def pytest_configure(config):
         )
 
 
-def pytest_report_header(config):
-    plugin: TrackMetricsReporter | None = config.pluginmanager.get_plugin(
-        _TRACK_METRICS_PLUGIN_NAME
-    )
-    if plugin:
-        return f"Plugin `track_metrics` is active, reporting to {plugin.path}"
-
-
-def pytest_unconfigure(config):
-    if config.pluginmanager.hasplugin(_TRACK_METRICS_PLUGIN_NAME):
-        config.pluginmanager.unregister(name=_TRACK_METRICS_PLUGIN_NAME)
-
-
 class TrackMetricsReporter:
     def __init__(
         self, path: Union[str, Path], user_properties_key: str = "track_metrics"
@@ -95,6 +82,9 @@ def pytest_sessionfinish(self, session):
         with self.path.open("w", encoding="utf8") as f:
             json.dump(self.metrics, f, indent=2)
 
+    def pytest_report_header(self):
+        return f"Plugin `track_metrics` is active, reporting to {self.path}"
+
     def pytest_terminal_summary(self, terminalreporter):
         terminalreporter.write_sep("-", f"Generated track_metrics report: {self.path}")
 
diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
index d49fa0e..4b2d34e 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -79,20 +79,6 @@ def pytest_configure(config: pytest.Config):
         )
 
 
-def pytest_report_header(config):
-    # TODO Move inside S3UploadPlugin
-    plugin: S3UploadPlugin | None = config.pluginmanager.get_plugin(
-        _UPLOAD_ASSETS_PLUGIN_NAME
-    )
-    if plugin:
-        return f"Plugin `upload_assets` is active, with upload to {plugin.bucket!r}"
-
-
-def pytest_unconfigure(config):
-    if config.pluginmanager.hasplugin(_UPLOAD_ASSETS_PLUGIN_NAME):
-        config.pluginmanager.unregister(name=_UPLOAD_ASSETS_PLUGIN_NAME)
-
-
 class S3UploadPlugin:
     def __init__(self, *, run_id: str | None = None, s3_client, bucket: str) -> None:
         self.run_id = run_id or uuid.uuid4().hex
@@ -139,6 +125,9 @@ def _upload(self, nodeid: str) -> Dict[str, str]:
             assets[name] = url
             self.upload_stats["uploaded"] += 1
 
+    def pytest_report_header(self):
+        return f"Plugin `upload_assets` is active, with upload to {self.bucket!r}"
+
     def pytest_terminal_summary(self, terminalreporter):
         terminalreporter.write_sep("-", f"`upload_assets` stats: {self.upload_stats}")
 
diff --git a/qa/unittests/tests/test_pytest_track_metrics.py b/qa/unittests/tests/test_pytest_track_metrics.py
index 4775857..c1af9a3 100644
--- a/qa/unittests/tests/test_pytest_track_metrics.py
+++ b/qa/unittests/tests/test_pytest_track_metrics.py
@@ -25,11 +25,17 @@ def test_3plus(track_metric, x):
     )
 
     metrics_path = tmp_path / "metrics.json"
-    result = pytester.runpytest(f"--track-metrics-report={metrics_path}")
-    result.assert_outcomes(passed=1, failed=1)
+    run_result = pytester.runpytest(
+        f"--track-metrics-report={metrics_path}",
+    )
+    run_result.stdout.re_match_lines(
+        [r"Plugin `track_metrics` is active, reporting to"]
+    )
+
+    run_result.assert_outcomes(passed=1, failed=1)
 
     assert metrics_path.exists()
-    result.stdout.re_match_lines([f".*Generated.*{re.escape(str(metrics_path))}.*"])
+    run_result.stdout.re_match_lines([f".*Generated.*{re.escape(str(metrics_path))}.*"])
 
     with metrics_path.open("r", encoding="utf8") as f:
         metrics = json.load(f)

From 89de8d0bb63200c9c4c91c7b6bf126a8fe1c9119 Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 17:38:30 +0200
Subject: [PATCH 14/19] Run benchmarks with log level INFO #5 #7

---
 .github/workflows/benchmarks.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.github/workflows/benchmarks.yaml b/.github/workflows/benchmarks.yaml
index aad2c3c..989bd13 100644
--- a/.github/workflows/benchmarks.yaml
+++ b/.github/workflows/benchmarks.yaml
@@ -24,8 +24,10 @@ jobs:
           mkdir report
           mkdir tmp_path_root
           pytest \
+            -vv \
             --random-subset=1 \
             -k max_ndvi_fail_intentionally \
+            --log-cli-level=INFO \
             --html report/report.html --self-contained-html \
             --track-metrics-report=report/metrics.json \
             --basetemp=tmp_path_root \

From c573a9b2c15457fbde3677bc526ce07a0a889db1 Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Thu, 25 Jul 2024 18:57:47 +0200
Subject: [PATCH 15/19] upload_assets: report uploads with terminalreporter for
 now #5

---
 .../pytest_upload_assets.py                   | 65 +++++++++++++------
 .../tests/test_pytest_upload_assets.py        |  7 +-
 2 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
index 4b2d34e..ace51ff 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -27,6 +27,7 @@ def test_dummy(upload_assets, tmp_path):
     - and env vars `UPLOAD_ASSETS_ACCESS_KEY_ID` and `UPLOAD_ASSETS_SECRET_ACCESS_KEY` set.
 """
 
+import collections
 import logging
 import os
 import re
@@ -85,7 +86,8 @@ def __init__(self, *, run_id: str | None = None, s3_client, bucket: str) -> None
         self.collected_assets: Dict[str, Path] | None = None
         self.s3_client = s3_client
         self.bucket = bucket
-        self.upload_stats = {"uploaded": 0}
+        self.upload_stats = collections.defaultdict(int, uploaded=0)
+        self.upload_reports: Dict[str, dict] = {}
 
     def collect(self, path: Path, name: str):
         """Collect assets to upload"""
@@ -99,37 +101,58 @@ def pytest_runtest_logstart(self, nodeid):
     def pytest_runtest_logreport(self, report: pytest.TestReport):
         # TODO #22: option to upload on other outcome as well?
         if report.when == "call" and report.outcome == "failed":
-            # TODO: what to do when upload fails?
-            uploaded = self._upload(nodeid=report.nodeid)
-            # TODO: report the uploaded assets somewhere (e.g. in user_properties or JSON report?)
+            self._upload_collected_assets(nodeid=report.nodeid)
 
     def pytest_runtest_logfinish(self, nodeid):
         # Reset collection of assets
         self.collected_assets = None
 
-    def _upload(self, nodeid: str) -> Dict[str, str]:
-        assets = {}
+    def _upload_collected_assets(self, nodeid: str):
+        upload_report = {}
         for name, path in self.collected_assets.items():
-            safe_nodeid = re.sub(r"[^a-zA-Z0-9_.-]", "_", nodeid)
-            key = f"{self.run_id}!{safe_nodeid}!{name}"
-            # TODO: is this manual URL building correct and isn't there a boto utility for that?
-            url = f"{self.s3_client.meta.endpoint_url.rstrip('/')}/{self.bucket}/{key}"
-            _log.info(f"Uploading {path} to {url}")
-            self.s3_client.upload_file(
-                Filename=str(path),
-                Bucket=self.bucket,
-                Key=key,
-                # TODO: option to override ACL, or ExtraArgs in general?
-                ExtraArgs={"ACL": "public-read"},
-            )
-            assets[name] = url
-            self.upload_stats["uploaded"] += 1
+            try:
+                url = self._upload_asset(nodeid=nodeid, name=name, path=path)
+                upload_report[name] = {"url": url}
+                self.upload_stats["uploaded"] += 1
+            except Exception as e:
+                _log.error(f"Failed to upload asset {name=} from {path=}: {e}")
+                upload_report[name] = {"error": str(e)}
+                self.upload_stats["failed"] += 1
+        self.upload_reports[nodeid] = upload_report
+
+    def _upload_asset(self, nodeid: str, name: str, path: Path) -> str:
+        safe_nodeid = re.sub(r"[^a-zA-Z0-9_.-]", "_", nodeid)
+        key = f"{self.run_id}!{safe_nodeid}!{name}"
+        # TODO: is this manual URL building correct? And isn't there a boto utility for that?
+        url = f"{self.s3_client.meta.endpoint_url.rstrip('/')}/{self.bucket}/{key}"
+        _log.info(f"Uploading asset {name=} from {path=} to {url=}")
+        self.s3_client.upload_file(
+            Filename=str(path),
+            Bucket=self.bucket,
+            Key=key,
+            # TODO: option to override ACL, or ExtraArgs in general?
+            ExtraArgs={"ACL": "public-read"},
+        )
+        return url
 
     def pytest_report_header(self):
         return f"Plugin `upload_assets` is active, with upload to {self.bucket!r}"
 
     def pytest_terminal_summary(self, terminalreporter):
-        terminalreporter.write_sep("-", f"`upload_assets` stats: {self.upload_stats}")
+        terminalreporter.write_sep(
+            "-", f"`upload_assets` stats: {dict(self.upload_stats)}"
+        )
+        for nodeid, upload_report in self.upload_reports.items():
+            terminalreporter.write_line(f"- {nodeid}:")
+            for name, report in upload_report.items():
+                if "url" in report:
+                    terminalreporter.write_line(
+                        f"  - {name!r} uploaded to {report['url']!r}"
+                    )
+                elif "error" in report:
+                    terminalreporter.write_line(
+                        f"  - {name!r} failed with: {report['error']!r}"
+                    )
 
 
 @pytest.fixture
diff --git a/qa/unittests/tests/test_pytest_upload_assets.py b/qa/unittests/tests/test_pytest_upload_assets.py
index 82c25bf..57e2c72 100644
--- a/qa/unittests/tests/test_pytest_upload_assets.py
+++ b/qa/unittests/tests/test_pytest_upload_assets.py
@@ -73,7 +73,12 @@ def test_fail_and_upload(upload_assets, tmp_path):
     actual = s3_client.get_object(Bucket=s3_bucket, Key=expected_key)
     assert actual["Body"].read().decode("utf8") == "Hello world."
 
-    run_result.stdout.re_match_lines([r".*`upload_assets` stats: \{'uploaded': 1\}"])
+    run_result.stdout.re_match_lines(
+        [
+            r".*`upload_assets` stats: \{'uploaded': 1\}",
+            r"\s+-\s+'hello.txt' uploaded to 'http://.*?/test-bucket-\w+/test-run-123!test_file_maker.py__test_fail_and_upload!hello.txt'",
+        ]
+    )
 
 
 def test_nop_on_success(pytester: pytest.Pytester, moto_server, s3_client, s3_bucket):

From 80f6e27da9f3b5723a23ebbbcad253a9ff880654 Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Fri, 26 Jul 2024 16:19:23 +0200
Subject: [PATCH 16/19] upload_assets tests: automatically pick free port for
 ThreadedMotoServer #5

---
 qa/unittests/tests/test_pytest_upload_assets.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qa/unittests/tests/test_pytest_upload_assets.py b/qa/unittests/tests/test_pytest_upload_assets.py
index 57e2c72..cdcf02d 100644
--- a/qa/unittests/tests/test_pytest_upload_assets.py
+++ b/qa/unittests/tests/test_pytest_upload_assets.py
@@ -8,10 +8,12 @@
 @pytest.fixture(scope="module")
 def moto_server() -> str:
     """Fixture to run a mocked AWS server for testing."""
-    server = moto.server.ThreadedMotoServer()
+    # Note: pass `port=0` to get a random free port.
+    # TODO avoid the private `_server` attribute https://github.com/getmoto/moto/issues/7894
+    server = moto.server.ThreadedMotoServer(port=0)
     server.start()
-    # TODO: avoid hardcoded port (5000)
-    yield "http://localhost:5000"
+    host, port = server._server.server_address
+    yield f"http://{host}:{port}"
     server.stop()
 
 

From 57549522eb32d3bc99270973e871229347ea86b0 Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Fri, 26 Jul 2024 16:30:14 +0200
Subject: [PATCH 17/19] upload_assets: run `random-subset` filtering as late as
 possible #5

Other selection/filter mechanisms (e.g. standard `-k`) should come before final sampling (typically just 1 sample)
---
 qa/benchmarks/tests/conftest.py | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/qa/benchmarks/tests/conftest.py b/qa/benchmarks/tests/conftest.py
index fa751b6..d234bb9 100644
--- a/qa/benchmarks/tests/conftest.py
+++ b/qa/benchmarks/tests/conftest.py
@@ -51,19 +51,22 @@ def pytest_ignore_collect(collection_path, config):
         return None
 
 
+@pytest.hookimpl(trylast=True)
 def pytest_collection_modifyitems(session, config, items):
     """
     Pytest hook to filter/reorder collected test items.
     """
-
     # Optionally, select a random subset of benchmarks to run.
     # based on https://alexwlchan.net/til/2024/run-random-subset-of-tests-in-pytest/
+    # Note that with current pytest versions the collection/summary stats might be messed up,
+    # see https://github.com/pytest-dev/pytest/issues/12663
     subset_size = config.getoption("--random-subset")
     if subset_size >= 0:
         _log.warning(
             f"Selecting random subset of {subset_size} from {len(items)} benchmarks."
         )
-        items[:] = random.sample(items, k=subset_size)
+        if subset_size < len(items):
+            items[:] = random.sample(items, k=subset_size)
 
 
 def _get_client_credentials_env_var(url: str) -> str:

From a06941be841ad0950933a4f56003b2828d39f334 Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Fri, 26 Jul 2024 17:41:01 +0200
Subject: [PATCH 18/19] upload_assets plugin rename fixture to
 `upload_assets_on_fail`

is more clear and better "grep-able"
---
 qa/benchmarks/tests/test_benchmarks.py                   | 4 ++--
 qa/benchmarks/tests/test_dummy.py                        | 4 ++--
 qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py | 9 +++++----
 qa/unittests/tests/test_pytest_upload_assets.py          | 8 ++++----
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/qa/benchmarks/tests/test_benchmarks.py b/qa/benchmarks/tests/test_benchmarks.py
index 1c93e80..5f49fd0 100644
--- a/qa/benchmarks/tests/test_benchmarks.py
+++ b/qa/benchmarks/tests/test_benchmarks.py
@@ -23,7 +23,7 @@ def test_run_benchmark(
     connection_factory,
     tmp_path: Path,
     track_metric,
-    upload_assets,
+    upload_assets_on_fail,
 ):
     connection: openeo.Connection = connection_factory(url=scenario.backend)
 
@@ -48,7 +48,7 @@ def test_run_benchmark(
     )
 
     # Upload assets on failure
-    upload_assets(*paths)
+    upload_assets_on_fail(*paths)
 
     # Compare actual results with reference data
     reference_dir = download_reference_data(
diff --git a/qa/benchmarks/tests/test_dummy.py b/qa/benchmarks/tests/test_dummy.py
index 31306be..1b9b44e 100644
--- a/qa/benchmarks/tests/test_dummy.py
+++ b/qa/benchmarks/tests/test_dummy.py
@@ -39,8 +39,8 @@ def test_produce_files_fail(tmp_path):
 
 
 @pytest.mark.parametrize("x", [3, 5])
-def test_upload_assets(tmp_path, upload_assets, x):
+def test_upload_assets(tmp_path, upload_assets_on_fail, x):
     path = tmp_path / "hello.txt"
     path.write_text("Hello, world.\n")
-    upload_assets(path)
+    upload_assets_on_fail(path)
     assert x == 5
diff --git a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
index ace51ff..65e9517 100644
--- a/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
+++ b/qa/tools/apex_algorithm_qa_tools/pytest_upload_assets.py
@@ -11,13 +11,14 @@
         "apex_algorithm_qa_tools.pytest_upload_assets",
     ]
 
--  Use the `upload_assets` fixture to register files for upload:
+-  Use the `upload_assets_on_fail` fixture to register files for upload
+   when the test fails:
 
     ```python
-    def test_dummy(upload_assets, tmp_path):
+    def test_dummy(upload_assets_on_fail, tmp_path):
         path = tmp_path / "hello.txt"
         path.write_text("Hello world.")
-        upload_assets(path)
+        upload_assets_on_fail(path)
     ```
 
 - Run the tests with:
@@ -156,7 +157,7 @@ def pytest_terminal_summary(self, terminalreporter):
 
 
 @pytest.fixture
-def upload_assets(pytestconfig: pytest.Config, tmp_path) -> Callable:
+def upload_assets_on_fail(pytestconfig: pytest.Config, tmp_path) -> Callable:
     """
     Fixture to register a file (under `tmp_path`) for S3 upload
     after the test failed. The fixture is a function that
diff --git a/qa/unittests/tests/test_pytest_upload_assets.py b/qa/unittests/tests/test_pytest_upload_assets.py
index cdcf02d..348f690 100644
--- a/qa/unittests/tests/test_pytest_upload_assets.py
+++ b/qa/unittests/tests/test_pytest_upload_assets.py
@@ -48,10 +48,10 @@ def test_basic_upload_on_fail(
     )
     pytester.makepyfile(
         test_file_maker="""
-            def test_fail_and_upload(upload_assets, tmp_path):
+            def test_fail_and_upload(upload_assets_on_fail, tmp_path):
                 path = tmp_path / "hello.txt"
                 path.write_text("Hello world.")
-                upload_assets(path)
+                upload_assets_on_fail(path)
                 assert 3 == 5
         """
     )
@@ -93,10 +93,10 @@ def test_nop_on_success(pytester: pytest.Pytester, moto_server, s3_client, s3_bu
     )
     pytester.makepyfile(
         test_file_maker="""
-            def test_success(upload_assets, tmp_path):
+            def test_success(upload_assets_on_fail, tmp_path):
                 path = tmp_path / "hello.txt"
                 path.write_text("Hello world.")
-                upload_assets(path)
+                upload_assets_on_fail(path)
                 assert 3 == 3
         """
     )

From f673cc9e11c801c122ff60377d2ee37f5c705822 Mon Sep 17 00:00:00 2001
From: Stefaan Lippens <stefaan.lippens@vito.be>
Date: Fri, 26 Jul 2024 17:45:00 +0200
Subject: [PATCH 19/19] upload_assets: remove debug config from benchmarks.yaml
 #5

---
 .github/workflows/benchmarks.yaml | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/.github/workflows/benchmarks.yaml b/.github/workflows/benchmarks.yaml
index 989bd13..b35815f 100644
--- a/.github/workflows/benchmarks.yaml
+++ b/.github/workflows/benchmarks.yaml
@@ -25,9 +25,8 @@ jobs:
           mkdir tmp_path_root
           pytest \
             -vv \
-            --random-subset=1 \
-            -k max_ndvi_fail_intentionally \
             --log-cli-level=INFO \
+            --random-subset=1 \
             --html report/report.html --self-contained-html \
             --track-metrics-report=report/metrics.json \
             --basetemp=tmp_path_root \