TRT-1702: Risk Analysis: ensure Test ID is populated #1852
Conversation
|
@sosiouxme: This pull request references TRT-1702 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 "4.17.0" version, but no target version was set. In response to this:
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. |
openshift-ci-robot
added
the
jira/valid-reference
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sosiouxme The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
sosiouxme
force-pushed
the
20240711-fix-test-id
branch
from
6c547f6
to
fd4747d
Compare
|
@sosiouxme: This pull request references TRT-1702 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 "4.17.0" version, but no target version was set. In response to this:
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. |
sosiouxme
force-pushed
the
20240711-fix-test-id
branch
from
fd4747d
to
08cb762
Compare
pkg/api/tests.go
Outdated
| processedResults := dbc.DB.Table("(?) as results", rawQuery). | ||
| Select(`ROW_NUMBER() OVER() as id, watchlist, name, jira_component, jira_component_id,` + variantSelect + query.QueryTestSummarizer). | ||
| Select(`id, watchlist, name, jira_component, jira_component_id,` + variantSelect + query.QueryTestSummarizer). |
There was a problem hiding this comment.
This is likely worth additional review. I'm wondering if there was a reason id wasn't used before and if this impacts other parts of the UI like the tests report.
There was a problem hiding this comment.
Could select id as test_id and then keep the existing ROW_NUMBER() OVER() statement. Don't know that we need it but if we wanted to keep the query as it was and just add in the test id.
There was a problem hiding this comment.
from the deleted FIXME comment and the state of the matview, i gathered this was written at a point where the test id didn't exist in the view, and so ROW_NUMBER was just a stand-in so there would be something, with the intent to go back and update the matview and the query. well, the matview got updated with a test id, but the query never did until this.
There was a problem hiding this comment.
Looks like apitype.Test expects ID to be a float64:
Line 535 in cc0ba0e
There was a problem hiding this comment.
/hold
the tests report is using the "synthetic" id and it gets the table wrong if results repeat the same id. that seems like an abuse of Test.ID to me but i suppose we can work around it.
no idea what the float64 cast is for but we can leave Test.ID to its current purpose and look up the real test id in a new field.
At the time that openshift-e2e-tests requests RA, test results haven't yet been associated with test IDs (we just have junits). So it is posting a `ProwJobRun` with partial `Test` records to the API, sans IDs. This PR changes the past-results queries by test name to also return the test ID, and uses that to populate RA results.
openshift-ci
bot
added
the
do-not-merge/hold
sosiouxme
force-pushed
the
20240711-fix-test-id
branch
from
08cb762
to
7f29fb9
Compare
pkg/apis/api/types.go
Outdated
| @@ -757,7 +758,7 @@ type ProwJobRunRiskAnalysis struct { | |||
|
|
|||
| type ProwJobRunTestRiskAnalysis struct { | |||
| Name string | |||
| TestID uint | |||
| TestID string | |||
There was a problem hiding this comment.
Curious how this will play out with the autodl table. We currently have TestID as an int. We can delete those tables and start over but if you think we will keep an int value and add the 'unique' string id maybe we should add a new string field here instead? I wonder if TestUniqueId or similar is a good identifier, like what you have in:
town.unique_id as test_str_id,
|
@sosiouxme: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
At the time that
openshift-e2e-testsrequests it, test results haven't yet been associated with test IDs (we just have junits). So it is posting aProwJobRunwith partialTestrecords to the API, sans IDs. This PR changes the past-results queries by test name to also return the test ID, and uses that to populate RA results.Messing with the queries worries me a little; every instance of a test name I saw only had a single test ID across multiple jobs, but I'm not certain that's reliable, so check me.