feat(stats): report testrun stats to the tcache - #3717
Open
OlufemiAdeOlusile wants to merge 5 commits into
Open
OlufemiAdeOlusile wants to merge 5 commits into
OlufemiAdeOlusile wants to merge 5 commits into
Conversation
The tcache gained a `/stats` endpoint. Nothing called it. This adds the client half, so a regression run records how many tests it ran and how they went. `scripts/stats_json.py` writes the document. It does not count the tests itself. `count_test_results.py` already groups Allure result files by `historyId`, so the `--skipall` registration pass of `run_tests.sh` is not counted twice, and this script reuses that logic. It adds only the fields a JUnit report cannot carry: the run identity, the timing, the software versions and the CLI coverage. `runner/report_stats.sh` uploads it. It runs after `save_artifacts.sh`, because `create_results.sh` is what creates `$WORKDIR/allure-results`. It is called with `|| :` like its neighbours, so a failed upload cannot mask the pytest exit code. Auth reuses `TCACHE_BASIC_AUTH`. No new secret is needed. The existing `TCACHE_URL` points at the `/results` prefix, so the script replaces that suffix with `/stats`, the same way the nightly `/history` upload does. The secrets are added to the `Run Regression Tests` step. They were only in scope for the two steps that call the tcache directly. Without this the upload would have skipped silently and recorded nothing. Three details that are easy to get wrong: - The duration is the wall clock span of the real results. Tests run in parallel under xdist, so the sum of the per-test durations is many times the elapsed time. - `never_run` counts tests the registration pass registered that never got a real result. It means the run was interrupted, so every count is a floor. It is a subset of `skipped`. - The testrun name is scrubbed exactly as the workflows scrub it, which drops dots. `node-10.5.0` becomes `node-1050`. That is lossy, and the tcache accepts dots, but matching the existing calls matters more. A different name here could not be joined to the `/import` rows. Verified against a real Allure directory of 4269 result files. The counts match `count_test_results.py` exactly: 2145 total, 1892 passed, 253 skipped. The 659 byte document was uploaded to a real tcache running under gunicorn and read back unchanged. 19 unit tests in `framework_tests/test_stats_json.py`.
OlufemiAdeOlusile
requested review from
mkoura and
saratomaz
as code owners
September 29, 2026 04:53
CodeQL raised a high severity alert on the PR: `py/clear-text-logging-sensitive-data`. It treats the output of the `secrets` module as a credential, and the run id is printed as part of the document. The alert is fair. The suffix only separates two runs started in the same second. Nothing about it is secret, and taking it from `secrets` said that it was. `uuid.uuid4().hex[:8]` carries the same uniqueness and states the real intent. The id keeps its shape, `local-<UTC stamp>-<8 hex>`, so the test that pins it is unchanged.
CodeQL raised `py/clear-text-logging-sensitive-data` on the document this script prints. The flow it followed starts at `report["cardano-cli"]`, so the heuristic is matching "card" inside "cardano" and treating the coverage numbers as payment data. It is a false positive, and a known one: two alerts of this rule on `cardano_cli_coverage.py`, which reads and prints the same keys, are already dismissed as false positives. Rather than ask for a third dismissal, the pattern is written out. The report holds one top-level entry, named after the tool it covers, and repeats that name in its own summary keys. The name is now read from the report instead of hard-coded, so no literal is needed and the script keeps working if the coverage report is ever produced for another tool. The two values are also coerced now. They are read from a file this script does not write and go straight into an upload, so a value that is not a finite number becomes None instead of travelling on. Seven more unit tests: a report for another tool, a non-numeric or non-finite coverage value, and a report with an unexpected shape.
The upgrade path was left out because Martin scoped the stats upload to `regression.sh` and the non-upgrade scripts. His reason no longer holds. It was that `node_upgrade_pytest.sh` emits no JUnit XML, which is still true, but the stats now come from Allure, and that script does produce results: it calls `create_results.sh` three times, writing `allure-results-step1`, `-step2` and `-step3`. The `step` column exists for exactly this. Each step uploads after its own `create_results.sh`, with its own pytest exit code, so the three rows share one run id and stay distinct. Verified against a real tcache: run 9001 holds step1, step2 and step3 with exit codes 0, 1 and 2. Each call is `|| :`. The script runs under `set -Eeuo pipefail` with an ERR trap, so an upload that failed would otherwise abort the upgrade run and change the exit code the step reports. No CLI coverage is passed. `cli_coverage.sh` runs in the `finish` step and its report covers the whole run, so attributing it to one step would misreport it. The secrets have to reach the script. `upgrade_reusable.yaml` did not declare them, so no caller could pass them, and its `Run Upgrade Tests` step carried only `GITHUB_TOKEN`. Both are fixed, and both callers now pass them: `nightly_upgrade.yaml` alongside the four secrets it already passes, and `upgrade.yaml`, which passed none at all.
Found by running `regression.sh` for real. The stored row said `cardano_node: 10.5.0` for a run that used **11.1.1**. `regression.sh` applies `PATH_PREPEND` to `PATH` at line 412, inside the `nix develop ... bash -c '...'` block that ends at line 446. The stats upload runs after that block, in the outer shell, where the built binaries were never on `PATH`. So `cardano-node --version` returned whatever the machine had installed. The failure mode is the bad kind: no error, no warning, a plausible version number, and a row that quietly misreports one of the two fields the endpoint exists to carry. `report_stats.sh` now prepends `PATH_PREPEND` itself. That variable is exported by `regression.sh` at line 205, so it is available in the outer shell. Verified against the real run: the row now reads `cardano_node: 11.1.1` and `cardano_cli: 11.2.3.1`. The upgrade path needs nothing. `node_upgrade_pytest.sh` exports its own `PATH` per step (lines 65 and 144), and the uploads sit inside those branches, so `PATH_PREPEND` is unset there and the new block is a no-op.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What it does
A test run now reports its test counts to the tcache.
The tcache gained a
/statsendpoint. Nothing called it. This adds the clienthalf.
scripts/stats_json.pyrunner/report_stats.shIt is wired into both test scripts that produce Allure results:
runner/regression.shstep=mainrunner/node_upgrade_pytest.shstep1,step2,step3How it works
scripts/stats_json.pydoes not count the tests itself.count_test_results.pyalready groups Allure result files byhistoryId, sothe
--skipallregistration pass ofrun_tests.shis not counted twice. Thisscript reuses that logic. It adds only the fields a JUnit report cannot carry:
the run identity, the timing, the software versions and the CLI coverage.
runner/report_stats.shruns aftercreate_results.sh, which is what createsthe Allure directory. Every call is
|| :, so a failed upload cannot mask thepytest exit code.
The document is about 660 bytes:
{ "schema": 1, "project": "cardano-node-tests", "testrun_name": "node-1050", "run_id": "1234", "step": "main", "origin": "ci", "timestamp": "2026-05-30T23:39:29.232000+00:00", "duration": 4527.316, "exit_code": 0, "filtered": false, "counts": {"total": 2145, "passed": 1892, "failed": 0, "broken": 0, "skipped": 253}, "quality": {"never_run": 0, "no_history_id": 0, "read_errors": 0}, "versions": {"cardano_node": "10.5.0", "cardano_cli": "10.11.1.0", "db_sync": null}, "commands": {"count": 213130, "coverage_pct": 31.0} }Auth
It reuses
TCACHE_BASIC_AUTH. No new secret is needed.TCACHE_URLpoints at the/resultsprefix, so the script replaces thatsuffix with
/stats. The nightly/historyupload does the same.The secrets were not in scope for the steps that run the test scripts.
They were set only on the steps that call the tcache with
curldirectly.Without this the upload would have skipped silently and recorded nothing.
upgrade_reusable.yamldid not even declare them, so no caller could passthem. Fixed in
regression_reusable.yaml,upgrade_reusable.yaml,nightly_upgrade.yamlandupgrade.yaml.Why the upgrade path is included
It was going to be left out, because you scoped this to
regression.shandthe non-upgrade scripts. That reason was that
node_upgrade_pytest.shemitsno JUnit XML. Still true, but the counts come from Allure now, and that script
calls
create_results.shthree times. Thestepcolumn already exists forthis. Say the word and I will drop it.
No CLI coverage is sent for the upgrade steps.
cli_coverage.shruns in thefinishstep and covers the whole run, so attributing it to one step wouldmisreport it.
Three details that are easy to get wrong
parallel under xdist, so the sum of the per-test durations is many times the
elapsed time.
never_runmeans the run was interrupted. It counts tests theregistration pass registered that never got a real result, so every count is
a floor. It is a subset of
skipped.dots, so
node-10.5.0becomesnode-1050. That is lossy, and the tcacheitself accepts dots. Matching the existing calls matters more, because a
different name here could not be joined to the
/importrows. Happy tochange this if you prefer the dots kept everywhere.
Testing
26 unit tests in
framework_tests/test_stats_json.py. All 474 framework testspass. No new ruff, shellcheck, actionlint, zizmor or mypy findings.
Verified against a real Allure directory of 4269 result files. The counts match
count_test_results.pyexactly: 2145 total, 1892 passed, 253 skipped. Thedocument was uploaded through
report_stats.shto a real tcache undergunicorn and read back unchanged. The three upgrade steps were checked too:
one run id holding
step1,step2andstep3with exit codes 0, 1 and 2.Depends on
The server side, mkoura/testing-results-cache#25. Merge that first, or this
uploads to an endpoint that does not exist yet. The upload is best effort, so
nothing breaks in the meantime.
🤖 Generated with Claude Code