Add pass percentage to run summary#849
Draft
mshriver wants to merge 1 commit into
Draft
Conversation
Contributor
Reviewer's GuideAdds computation, storage, display, and filtering support for a numeric pass percentage on test runs, including a backfill migration and an index for efficient range queries, plus tests across backend and frontend. Sequence diagram for pass_percent range filteringsequenceDiagram
actor User
participant UI as Frontend_RunList
participant API as Backend_API
participant Filters as convert_filter
participant DB as DB_runs
User->>UI: Set filter summary.pass_percent>=50
UI->>API: GET /runs?filter=summary.pass_percent>=50
API->>Filters: convert_filter("summary.pass_percent>=50")
Filters->>DB: WHERE (summary->>'pass_percent')::float>=50
DB-->>API: Matching runs
API-->>UI: Runs with pass_percent field
UI->>UI: getRunPassPercent(summary)
UI-->>User: Render Pass % column and filters result
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (73.48%) is below the target coverage (85.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #849 +/- ##
==========================================
+ Coverage 73.44% 73.48% +0.03%
==========================================
Files 154 155 +1
Lines 7562 7572 +10
Branches 662 663 +1
==========================================
+ Hits 5554 5564 +10
Misses 1788 1788
Partials 220 220
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
mshriver
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a "pass percentage" metric to runs: stored in the run summary JSON on update, surfaced in the Run detail page and Runs list, exposed as a numeric filter field, backfilled for existing runs, and indexed in PostgreSQL for fast range queries.
Changes:
- Backend: compute and persist
summary.pass_percentinupdate_run, registersummary.pass_percentas a numeric filter field, broaden numeric-field handling infilters.pyto cast JSON columns viaas_float(), and add an Alembic migration that backfillspass_percentand creates a B-tree expression index (ix_runs_pass_percent). - Frontend: new
getRunPassPercenthelper (re-exported viautilities/index.js) with unit tests, plus rendering of the Pass % in the runs table (tables.js,run-list.js) and run detail page (run.js); addsummary.pass_percenttoNUMERIC_RUN_FIELDS. - Tests: backend coverage for
update_runsettingpass_percent(including the floor case) and for>/</=filter behavior onsummary.pass_percent; frontend test updates the column-list comment.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/ibutsu_server/tasks/runs.py | Adds pass_percent calculation (truncated int) to run summary on update. |
| backend/ibutsu_server/constants.py | Registers summary.pass_percent as a NUMERIC_FIELDS entry. |
| backend/ibutsu_server/filters.py | Casts JSON numeric-field columns via as_float() and routes numeric values through _to_int_or_float. |
| backend/alembic/versions/d18de2b3253f_backfill_pass_percent_in_run_summary.py | Postgres-only migration that backfills pass_percent in three passes and creates an expression index. |
| backend/tests/tasks/test_runs.py | Asserts pass_percent for normal, none-summary, empty-summary, and 999/1000 floor cases. |
| backend/tests/controllers/test_run_controller.py | Adds >, <, = filter tests on summary.pass_percent. |
| frontend/src/utilities/run.js | New getRunPassPercent helper with stored-value preference and counter-based fallback. |
| frontend/src/utilities/run.test.js | Unit tests for the new helper. |
| frontend/src/utilities/index.js | Re-exports ./run. |
| frontend/src/utilities/tables.js | Adds Pass % cell to the runs table row. |
| frontend/src/pages/run.js | Adds Pass % data-list row to the run summary panel. |
| frontend/src/pages/run-list.js | Adds 'Pass %' column header. |
| frontend/src/pages/run-list.test.js | Updates column-list comment to include Pass %. |
| frontend/src/constants.js | Adds summary.pass_percent to NUMERIC_RUN_FIELDS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The frontend
getRunPassPercentreturns'N/A'whensummary.tests === 0, while the backend always storespass_percent = 0for zero-tests runs; consider aligning these semantics so users and filters see a consistent value. - In the migration,
pass_percentis stored as an integer percentage but the index expression is((summary->>'pass_percent')::int)while the filter path casts the field to float; you may want to standardize on a single type (int) throughout to avoid subtle planner/type issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The frontend `getRunPassPercent` returns `'N/A'` when `summary.tests === 0`, while the backend always stores `pass_percent = 0` for zero-tests runs; consider aligning these semantics so users and filters see a consistent value.
- In the migration, `pass_percent` is stored as an integer percentage but the index expression is `((summary->>'pass_percent')::int)` while the filter path casts the field to float; you may want to standardize on a single type (int) throughout to avoid subtle planner/type issues.
## Individual Comments
### Comment 1
<location path="backend/alembic/versions/d18de2b3253f_backfill_pass_percent_in_run_summary.py" line_range="37-46" />
<code_context>
+ return
+
+ # Backfill pass_percent for runs that have passes and tests > 0
+ result = conn.execute(
+ sa.text("""
+ UPDATE runs
+ SET summary = jsonb_set(
+ summary,
+ '{pass_percent}',
+ to_jsonb(
+ floor(
+ ((summary->>'passes')::numeric
+ / (summary->>'tests')::numeric) * 100
+ )::int
+ )
+ )
+ WHERE summary IS NOT NULL
+ AND summary->>'tests' IS NOT NULL
+ AND (summary->>'tests')::int > 0
+ AND summary->>'passes' IS NOT NULL
+ """)
+ )
+ logger.info("Updated %d runs with pass_percent (from passes)", result.rowcount)
+
+ # Handle runs that have tests but no passes key (very old runs)
</code_context>
<issue_to_address>
**issue:** Guard the backfill against non-numeric or malformed JSON counter values
This UPDATE casts `(summary->>'passes')` and `(summary->>'tests')` to `numeric` without validating them first. If any existing rows contain non-numeric or oddly formatted strings, the migration will fail. Consider restricting the UPDATE to rows where both fields are numeric (e.g. `summary->>'tests' ~ '^[0-9]+$'` and similarly for `passes`), or using a `NULLIF`/`TRY_CAST` pattern where available, to avoid migration failures on dirty data.
</issue_to_address>
### Comment 2
<location path="backend/tests/controllers/test_run_controller.py" line_range="564" />
<code_context>
assert "runs" in response_data
+def test_filter_pass_percent_greater_than(flask_app, make_project, make_run, auth_headers):
+ """Test filtering runs where pass_percent > threshold returns only matching runs."""
+ client, jwt_token = flask_app
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `>=` and `<=` pass_percent filters and boundary conditions to reflect the documented behavior.
The PR description calls out `>= 50`, `> 50`, `<= 50`, and `< 50` semantics, but the tests only cover `>`, `<`, and `=` and don’t assert boundary behavior. Please add tests for `summary.pass_percent>=X` and `summary.pass_percent<=X` using values like `49`, `50`, and `51` to verify inclusion/exclusion at the boundary and guard against regressions in filter parsing.
</issue_to_address>
### Comment 3
<location path="backend/tests/tasks/test_runs.py" line_range="231" />
<code_context>
+ assert updated_run.summary["pass_percent"] == 100
+
+
+def test_update_run_pass_percent_floors(make_project, make_run, make_result, flask_app, fixed_time):
+ """Test that pass_percent is floored, not rounded -- 999/1000 should be 99, not 100."""
+ client, _ = flask_app
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for the pass_percent behavior when there are zero tests to avoid division-by-zero or unexpected non-zero values.
You already cover flooring (`999/1000 -> 99`). Please also add a case for `tests == 0` in `update_run`, where `pass_percent` is forced to `0`. For example, create a run whose summary has `collected > 0` but no results (so `tests` stays `0`) and assert `summary['pass_percent'] == 0` to guard against future division‑by‑zero or `NaN`/`None` regressions.
```suggestion
def test_update_run_with_empty_summary(make_project, make_run, make_result, flask_app):
assert "tests" in updated_run.summary
assert updated_run.summary["tests"] == 1
assert updated_run.summary["passes"] == 1
assert updated_run.summary["pass_percent"] == 100
def test_update_run_pass_percent_zero_tests(make_project, make_run, flask_app):
"""When there are zero tests, pass_percent should be forced to 0."""
client, _ = flask_app
with client.application.app_context():
project = make_project(name="test-project")
# collected > 0 but no results created, so tests should remain 0
run = make_run(
project_id=project.id,
summary={"collected": 10, "tests": 0, "failures": 0, "errors": 0},
)
updated_run = update_run(run.id)
assert "tests" in updated_run.summary
assert updated_run.summary["tests"] == 0
assert updated_run.summary["passes"] == 0
assert updated_run.summary["pass_percent"] == 0
def test_update_run_pass_percent_floors(make_project, make_run, make_result, flask_app, fixed_time):
```
</issue_to_address>
### Comment 4
<location path="backend/alembic/versions/d18de2b3253f_backfill_pass_percent_in_run_summary.py" line_range="29" />
<code_context>
+logger = logging.getLogger("alembic.versions.d18de2b3253f")
+
+
+def _is_postgresql():
+ """Check if the current database is PostgreSQL."""
+ return op.get_bind().dialect.name == "postgresql"
</code_context>
<issue_to_address>
**issue (complexity):** Consider consolidating the index helper functions and merging the three backfill UPDATEs into a single CASE-based query to reduce duplication and mental overhead.
You can simplify both the index helpers and the backfill SQL without changing behavior.
### 1. Collapse index helpers into a single utility
Four helpers for a single index add mental overhead. You can keep the idempotent behavior but reduce surface area by using a single helper that handles dialect + existence + create/drop:
```python
def _ensure_index(name: str, table: str, columns, *, create: bool, **kwargs) -> None:
conn = op.get_bind()
if conn.dialect.name != "postgresql":
return
exists = conn.execute(
sa.text(
"""
SELECT EXISTS (
SELECT 1
FROM pg_indexes
WHERE indexname = :index_name
AND tablename = :table_name
)
"""
),
{"index_name": name, "table_name": table},
).scalar()
if create and not exists:
op.create_index(name, table, columns, **kwargs)
elif not create and exists:
op.drop_index(name, table_name=table)
```
Usage:
```python
# upgrade
_ensure_index(
"ix_runs_pass_percent",
"runs",
[sa.text("((summary->>'pass_percent')::int)")],
create=True,
unique=False,
)
# downgrade
_ensure_index("ix_runs_pass_percent", "runs", None, create=False)
```
This removes `_is_postgresql`, `_index_exists`, `_create_index_if_not_exists`, `_drop_index_if_exists` while preserving behavior.
---
### 2. Combine the three `UPDATE` statements into one with `CASE`
The three updates share most predicates and the `jsonb_set` pattern; you can express the same logic in a single statement with a `CASE` expression, which also guarantees the conditions can’t drift apart:
```python
result = conn.execute(
sa.text(
"""
UPDATE runs
SET summary = jsonb_set(
COALESCE(summary, '{}'::jsonb),
'{pass_percent}',
to_jsonb(
CASE
-- Case 1: passes present, compute from passes/tests
WHEN summary IS NOT NULL
AND summary->>'tests' IS NOT NULL
AND (summary->>'tests')::int > 0
AND summary->>'passes' IS NOT NULL
THEN floor(
((summary->>'passes')::numeric /
(summary->>'tests')::numeric) * 100
)::int
-- Case 2: no passes key, derive from counters
WHEN summary IS NOT NULL
AND summary->>'tests' IS NOT NULL
AND (summary->>'tests')::int > 0
AND summary->>'passes' IS NULL
THEN floor(
(
(summary->>'tests')::numeric
- COALESCE((summary->>'failures')::numeric, 0)
- COALESCE((summary->>'errors')::numeric, 0)
- COALESCE((summary->>'skips')::numeric, 0)
- COALESCE((summary->>'xpasses')::numeric, 0)
- COALESCE((summary->>'xfailures')::numeric, 0)
) / (summary->>'tests')::numeric * 100
)::int
-- Case 3: everything else with a summary gets 0
ELSE 0
END
)
)
WHERE summary IS NOT NULL
AND summary->'pass_percent' IS NULL
"""
)
)
```
This keeps all existing behaviors:
- First branch matches your first `UPDATE`.
- Second branch matches your “very old runs” `UPDATE`.
- Final `ELSE 0` covers the third `UPDATE`’s “remaining runs” behavior.
Net result: same functionality, less duplication and a single, coherent backfill definition.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert "runs" in response_data | ||
|
|
||
|
|
||
| def test_filter_pass_percent_greater_than(flask_app, make_project, make_run, auth_headers): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for >= and <= pass_percent filters and boundary conditions to reflect the documented behavior.
The PR description calls out >= 50, > 50, <= 50, and < 50 semantics, but the tests only cover >, <, and = and don’t assert boundary behavior. Please add tests for summary.pass_percent>=X and summary.pass_percent<=X using values like 49, 50, and 51 to verify inclusion/exclusion at the boundary and guard against regressions in filter parsing.
| assert updated_run.summary["pass_percent"] == 50 | ||
|
|
||
|
|
||
| def test_update_run_with_empty_summary(make_project, make_run, make_result, flask_app): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test for the pass_percent behavior when there are zero tests to avoid division-by-zero or unexpected non-zero values.
You already cover flooring (999/1000 -> 99). Please also add a case for tests == 0 in update_run, where pass_percent is forced to 0. For example, create a run whose summary has collected > 0 but no results (so tests stays 0) and assert summary['pass_percent'] == 0 to guard against future division‑by‑zero or NaN/None regressions.
Suggested change
| def test_update_run_with_empty_summary(make_project, make_run, make_result, flask_app): | |
| def test_update_run_with_empty_summary(make_project, make_run, make_result, flask_app): | |
| assert "tests" in updated_run.summary | |
| assert updated_run.summary["tests"] == 1 | |
| assert updated_run.summary["passes"] == 1 | |
| assert updated_run.summary["pass_percent"] == 100 | |
| def test_update_run_pass_percent_zero_tests(make_project, make_run, flask_app): | |
| """When there are zero tests, pass_percent should be forced to 0.""" | |
| client, _ = flask_app | |
| with client.application.app_context(): | |
| project = make_project(name="test-project") | |
| # collected > 0 but no results created, so tests should remain 0 | |
| run = make_run( | |
| project_id=project.id, | |
| summary={"collected": 10, "tests": 0, "failures": 0, "errors": 0}, | |
| ) | |
| updated_run = update_run(run.id) | |
| assert "tests" in updated_run.summary | |
| assert updated_run.summary["tests"] == 0 | |
| assert updated_run.summary["passes"] == 0 | |
| assert updated_run.summary["pass_percent"] == 0 | |
| def test_update_run_pass_percent_floors(make_project, make_run, make_result, flask_app, fixed_time): |
add to run page, runs table, and add index for fast filtering include unit tests for backend and frontend Co-authored-by: Claude <noreply@anthropic.com>
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.
add to run page, runs table, and add index for fast filtering
FIXES #847
Runs Table
Run Summary
Runs Filtering
>= 50
Includes 50% run:
> 50
Excludes 50% run:
<= 50
Includes 50% run:
<50
Excludes 50% run:
Summary by Sourcery
Add pass percentage calculation and display for test runs, including backfilling existing data and indexing for efficient filtering.
New Features:
Enhancements:
Tests:
Summary by Sourcery
Add pass percentage support to test run summaries, including storage, backfill, filtering, and UI display.
New Features:
Enhancements:
Tests: