Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TRT-1702: Risk Analysis: ensure Test ID is populated #1852

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cmd/sippy/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,11 @@ func (f *ServerFlags) Validate() error {

func NewServeCommand() *cobra.Command {
f := NewServerFlags()

cmd := newServeFlagsCommand(f)
f.BindFlags(cmd.Flags())
return cmd
}
func newServeFlagsCommand(f *ServerFlags) *cobra.Command {
cmd := &cobra.Command{
Use: "serve",
Short: "Run the sippy server",
Expand Down
10 changes: 9 additions & 1 deletion pkg/api/job_runs.go
Original file line number Diff line number Diff line change
Expand Up @@ -528,9 +528,17 @@ func runTestRunAnalysis(failedTest models.ProwJobRunTest, jobRun *models.ProwJob
return apitype.ProwJobRunTestRiskAnalysis{}, errJobNames
}

// one of our data sources should have the test ID
testID := failedTest.Test.TestID
if testID == 0 && testResultsJobNames != nil {
testID = testResultsJobNames.TestID
}
if testID == 0 && testResultsVariants != nil {
testID = testResultsVariants.TestID
}
analysis := apitype.ProwJobRunTestRiskAnalysis{
Name: failedTest.Test.Name,
TestID: failedTest.Test.ID,
TestID: testID,
OpenBugs: failedTest.Test.Bugs,
}
// Watch out for tests that ran in previous period, but not current, no sense comparing to 0 runs:
Expand Down
5 changes: 2 additions & 3 deletions pkg/api/tests.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ func BuildTestsResults(dbc *db.DB, release, period string, collapse, includeOver
// Collapse groups the test results together -- otherwise we return the test results per-variant combo (NURP+)
variantSelect := ""
if collapse {
rawQuery = rawQuery.Select(`name,watchlist,jira_component,jira_component_id,` + query.QueryTestSummer).Group("name,watchlist,jira_component,jira_component_id")
rawQuery = rawQuery.Select(`test_id,name,watchlist,jira_component,jira_component_id,` + query.QueryTestSummer).Group("test_id,name,watchlist,jira_component,jira_component_id")
} else {
rawQuery = query.TestsByNURPAndStandardDeviation(dbc, release, table)
variantSelect = "suite_name, variants," +
Expand All @@ -210,9 +210,8 @@ func BuildTestsResults(dbc *db.DB, release, period string, collapse, includeOver
}

testReports := make([]apitype.Test, 0)
// FIXME: Add test id to matview, for now generate with ROW_NUMBER OVER
processedResults := dbc.DB.Table("(?) as results", rawQuery).
Select(`ROW_NUMBER() OVER() as id, watchlist, name, jira_component, jira_component_id,` + variantSelect + query.QueryTestSummarizer).
Select(`ROW_NUMBER() OVER() as id, test_id, watchlist, name, jira_component, jira_component_id,` + variantSelect + query.QueryTestSummarizer).
Where("current_runs > 0 or previous_runs > 0")

finalResults := dbc.DB.Table("(?) as final_results", processedResults)
Expand Down
3 changes: 2 additions & 1 deletion pkg/apis/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,7 @@ func (run JobRun) GetArrayValue(param string) ([]string, error) {
// of this struct is suitable for use in a data table.
type Test struct {
ID int `json:"id,omitempty"`
TestID string `json:"test_id,omitempty"`
Name string `json:"name"`
SuiteName string `json:"suite_name"`
Variant string `json:"variant,omitempty"`
Expand Down Expand Up @@ -757,7 +758,7 @@ type ProwJobRunRiskAnalysis struct {

type ProwJobRunTestRiskAnalysis struct {
Name string
TestID uint
TestID string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Risk TestFailureRisk
OpenBugs []models.Bug
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/db/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type log2LogrusWriter struct {
}

func (w log2LogrusWriter) Printf(msg string, args ...interface{}) {
w.entry.Debugf(msg, args...)
w.entry.Infof(msg, args...)
}

func New(dsn string, logLevel gormlogger.LogLevel) (*DB, error) {
Expand Down
8 changes: 6 additions & 2 deletions pkg/db/matviews.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ GROUP BY
const testAnalysisByVariantMatView = `
SELECT
tests.id AS test_id,
town.unique_id as test_str_id,
tests.name AS test_name,
tests.watchlist,
date(prow_job_runs."timestamp") AS date,
Expand All @@ -254,17 +255,19 @@ SELECT
FROM
prow_job_run_tests
JOIN tests ON tests.id = prow_job_run_tests.test_id
JOIN test_ownerships town ON town.test_id = tests.id
JOIN prow_job_runs ON prow_job_runs.id = prow_job_run_tests.prow_job_run_id
JOIN prow_jobs ON prow_jobs.id = prow_job_runs.prow_job_id
WHERE
prow_job_run_tests.created_at > (|||TIMENOW||| - '14 days'::interval) AND prow_job_runs."timestamp" > (|||TIMENOW||| - '14 days'::interval)
GROUP BY
tests.name, tests.id, date(prow_job_runs."timestamp"), unnest(prow_jobs.variants), prow_jobs.release
tests.name, town.unique_id, tests.id, date(prow_job_runs."timestamp"), unnest(prow_jobs.variants), prow_jobs.release
`

const testAnalysisByJobMatView = `
SELECT
tests.id AS test_id,
town.unique_id as test_str_id,
tests.name AS test_name,
tests.watchlist,
date(prow_job_runs."timestamp") AS date,
Expand All @@ -277,12 +280,13 @@ SELECT
FROM
prow_job_run_tests
JOIN tests ON tests.id = prow_job_run_tests.test_id
JOIN test_ownerships town ON town.test_id = tests.id
JOIN prow_job_runs ON prow_job_runs.id = prow_job_run_tests.prow_job_run_id
JOIN prow_jobs ON prow_jobs.id = prow_job_runs.prow_job_id
WHERE
prow_job_run_tests.created_at > (|||TIMENOW||| - '14 days'::interval) AND prow_job_runs."timestamp" > (|||TIMENOW||| - '14 days'::interval)
GROUP BY
tests.name, tests.id, date(prow_job_runs."timestamp"), prow_jobs.release, prow_jobs.name
tests.name, town.unique_id, tests.id, date(prow_job_runs."timestamp"), prow_jobs.release, prow_jobs.name
`

const prowJobFailedTestsMatView = `
Expand Down
2 changes: 1 addition & 1 deletion pkg/db/query/test_queries.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const (

QueryTestSummarizer = QueryTestFields + "," + QueryTestPercentages

QueryTestAnalysis = "select current_successes, current_runs, current_successes * 100.0 / NULLIF(current_runs, 0) AS current_pass_percentage from ( select sum(runs) as current_runs, sum(passes) as current_successes from prow_test_analysis_by_job_14d_matview where test_name = '%s' AND job_name in (%s))t"
QueryTestAnalysis = "select test_id as id, current_successes, current_runs, current_successes * 100.0 / NULLIF(current_runs, 0) AS current_pass_percentage from ( select test_id, sum(runs) as current_runs, sum(passes) as current_successes from prow_test_analysis_by_job_14d_matview join where test_name = '%s' AND job_name in (%s) GROUP BY test_id)t"
)

// TestReportsByVariant returns a test report for every test in the db matching the given substrings, separated by variant.
Expand Down