TRT-2741: fix duplicate rows on Tests page

[mstaeble]

Summary

• Removed the synthetic id field from Test and TestBQ structs and all query paths (PG and BQ), since it was misleading and no longer needed after the matview removal in PR TRT-2741: Phase 2 - Replace prow_test_report matviews with cumulative summary queries #3769.
• Added a getRowId prop to the MUI DataGrid in TestTable.js using a composite key (name + suite_name + variants) with \x1f (Unit Separator) delimiter to prevent duplicate row rendering.
• Fixed a SQL column reference bug in the stats CTE (test_queries.go) where the renamed column was not updated in a downstream subquery.
• Removed stale ROW_NUMBER() OVER() as id from benchmark tests.

Test plan

• make lint passes
• make test passes
• make e2e passes
• Verify on staging that the Tests page no longer shows duplicate rows
• Verify Tests By Variant view no longer shows duplicates
• Verify "Overall" row still renders correctly

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved consistency and accuracy across PostgreSQL and BigQuery test reports.
  • Corrected report data matching when displaying uncollapsed test results.
  • Removed unsupported synthetic test identifiers from report data and API responses.
• UI Improvements
  • Test tables now use more reliable row identification, helping preserve stable rendering when tests share similar names or variants.

[openshift-merge-bot]

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@mstaeble: This pull request references TRT-2741 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

• Removed the synthetic id field from Test and TestBQ structs and all query paths (PG and BQ), since it was misleading and no longer needed after the matview removal in PR TRT-2741: Phase 2 - Replace prow_test_report matviews with cumulative summary queries #3769.
• Added a getRowId prop to the MUI DataGrid in TestTable.js using a composite key (name + suite_name + variants) with \x1f (Unit Separator) delimiter to prevent duplicate row rendering.
• Fixed a SQL column reference bug in the stats CTE (test_queries.go) where the renamed column was not updated in a downstream subquery.
• Removed stale ROW_NUMBER() OVER() as id from benchmark tests.

Test plan

• make lint passes
• make test passes
• make e2e passes
• Verify on staging that the Tests page no longer shows duplicate rows
• Verify Tests By Variant view no longer shows duplicates
• Verify "Overall" row still renders correctly

🤖 Generated with Claude Code

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Walkthrough

Report APIs and SQL queries no longer expose synthetic test IDs. PostgreSQL and BigQuery projections are updated accordingly, and the test table now derives row identity from test metadata.

Changes

Synthetic test ID removal

Layer / File(s)	Summary
Remove synthetic IDs from API results pkg/apis/api/types.go, pkg/api/tests.go	Test and TestBQ no longer expose or resolve synthetic IDs, and overall results no longer assign sentinel IDs.
Align report query projections and aliases pkg/api/tests.go, pkg/db/query/*.go, pkg/flags/postgres_benchmarking_test.go	PostgreSQL and BigQuery queries stop projecting generated IDs, while uncollapsed statistics joins use the propagated test_id alias.
Provide composite grid row identity sippy-ng/src/tests/TestTable.js	The data grid derives row IDs from test name, suite name, and variants.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: dgoodwin, neisw

🚥 Pre-merge checks | ✅ 19 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Coverage For New Features	⚠️ Warning	computeOverallTest is covered, but no tests cover TestTable getRowId or the changed query builders, and no regression test covers removed Test/TestBQ id behavior.	Add regression tests for TestTable row identity and the modified PG/BQ SQL builders; if removing 'id' is intentional, add a test for the new unknown-field behavior.

✅ Passed checks (19 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Go Error Handling	✅ Passed	Patch only removes synthetic IDs and renames SQL columns; no added lines introduce ignored errors, panics, or nil dereferences.
Sql Injection Prevention	✅ Passed	The touched query builders keep user-supplied values behind placeholders/@params; new string concatenations use only internal constants/identifiers, not request data.
Excessive Css In React Should Use Styles	✅ Passed	PASS: The changed React file uses only tiny dynamic inline styles (one prop each) and a useStyles hook for the only fixed CSS; no large inline style objects were added.
Single Responsibility And Clear Naming	✅ Passed	The PR removes misleading synthetic IDs, aligns test row structs with real identity, and adds a clearly named composite getRowId; no new vague abstractions were introduced.
Feature Documentation	✅ Passed	PASS: docs/features only contains job-analysis-symptoms.md, which is unrelated; no Tests-page feature doc exists there to update.
Stable And Deterministic Test Names	✅ Passed	No changed Ginkgo titles contain dynamic values; touched tests use static t.Run names, and the PR only changed SQL/data paths plus a static getRowId.
Test Structure And Quality	✅ Passed	No Ginkgo specs were added or modified; the only Go test change is a standard benchmark using testing.T, so the checklist is not applicable.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; changed files are query/type/benchmark/UI code and contain no MicroShift-sensitive APIs or guards to review.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No Ginkgo e2e tests were added; the touched files are API/query/JS code and a standard Go benchmark test, with no SNO-sensitive assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	Only API/query/frontend code changed; no manifests, controllers, or topology-aware scheduling constraints were added.
Ote Binary Stdout Contract	✅ Passed	No process-level stdout writes were added; the only fmt.Print/Printf calls are inside benchmark test functions, not main/init/TestMain/suite setup.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added in the changed files, and I found no hardcoded IPv4, localhost, or external-host networking assumptions.
No-Weak-Crypto	✅ Passed	Touched files add no weak crypto, custom crypto, or secret comparisons; the patch only removes synthetic IDs and adds DataGrid row identity.
Container-Privileges	✅ Passed	No changed manifest files, and repo search found no privileged:true/hostPID/hostNetwork/hostIPC/allowPrivilegeEscalation:true, SYS_ADMIN, or root settings.
No-Sensitive-Data-In-Logs	✅ Passed	No new sensitive logging appears in the touched hunks; the PR only changes SQL projections and DataGrid row IDs, with no added log/console output.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: fixing duplicate rows on the Tests page.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mstaeble]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

🧹 Nitpick comments (1)

pkg/api/tests.go (1)

465-472: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: Avoid relying on exact slice capacity when appending.

Both collapsedColumns and selectColumns are created by appending to testMetadataColumns. This works safely only because testMetadataColumns is initialized with a composite literal whose length equals its capacity, forcing append to allocate a new backing array each time. If testMetadataColumns is ever refactored to have excess capacity (e.g., using make with a larger capacity), the second append will silently overwrite the results of the first. Consider explicitly copying the slice or constructing them independently to prevent future regression.

♻️ Proposed refactor

-		collapsedColumns := append(testMetadataColumns, query.QueryTestFields)
+		collapsedColumns := append(append([]string(nil), testMetadataColumns...), query.QueryTestFields)
 		rawQuery := dbc.DB.WithContext(ctx).
 			Table("(?) AS r", collapsedQuery).
 			Select(strings.Join(collapsedColumns, ","))
 
 		// Collapsed path: inner query only has counts, so the outer layer computes
 		// percentages and net improvements via QueryTestSummarizer.
-		selectColumns := append(testMetadataColumns, query.QueryTestSummarizer)
+		selectColumns := append(append([]string(nil), testMetadataColumns...), query.QueryTestSummarizer)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/api/tests.go` around lines 465 - 472, Update the construction of
collapsedColumns and selectColumns so each append operates on an independent
copy of testMetadataColumns, rather than relying on its current capacity.
Preserve the existing column contents and query behavior while preventing the
second append from overwriting the first if testMetadataColumns later has spare
capacity.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/api/tests.go`:
- Around line 465-472: Update the construction of collapsedColumns and
selectColumns so each append operates on an independent copy of
testMetadataColumns, rather than relying on its current capacity. Preserve the
existing column contents and query behavior while preventing the second append
from overwriting the first if testMetadataColumns later has spare capacity.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 298c7c80-70ca-4cb6-ae55-28fd7270062f

📥 Commits

Reviewing files that changed from the base of the PR and between 6b1a47a and 077aff1.

📒 Files selected for processing (6)

• pkg/api/tests.go
• pkg/apis/api/types.go
• pkg/db/query/cumulative_query.go
• pkg/db/query/test_queries.go
• pkg/flags/postgres_benchmarking_test.go
• sippy-ng/src/tests/TestTable.js

[mstaeble]

Re: nitpick on append(testMetadataColumns, ...) — dismissed.

testMetadataColumns is a 4-element composite literal (len == cap == 4), so both append calls always allocate fresh backing arrays. The defensive copy pattern adds verbosity for a low-probability future risk.

[openshift-merge-bot]

Scheduling required tests:
/test e2e

[mstaeble]

[mstaeble]

[smg247]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mstaeble, smg247

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [smg247]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

@mstaeble: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.