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
separate the test result intermediate types from test result output types #66
separate the test result intermediate types from test result output types #66
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deads2k 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 |
1a415d9
to
5048cf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slight disagreement on the direction of one of the TODOs, otherwise seems fine
// TODO Inside a particular job, only bugs matching the job are present. | ||
// TODO Inside a platform, only bugs matching the platform are present. | ||
BugList []bugsv1.Bug `json:"bugList"` | ||
// TODO fix search link to properly take into account release, job, and platform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure we want that. it's a bit of a debate in my head how much to filter the ci-search results. Filtering by release makes reasonable sense to me (i don't care about test failures from another release, they are a different codebase and may not be relevant). But I probably do care about failures from other platforms/jobs within the same release. They may not have the same cause, but it's informative to know where else the test is failing.
Since the search link already accounts for the release, i'd vote for just removing this todo as i don't think it's an appropriate goal. but i guess we can debate that elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the search link already accounts for the release, i'd vote for just removing this todo as i don't think it's an appropriate goal. but i guess we can debate that elsewhere.
you'll get a chance to object if anyone actually makes it do this. Based on context, I think we want different ci-search links.
0. for a release (like the top failing test view) - restrict by release
- for a variant (like the by variant view) - restrict by variant and release
- for a job (like the by-job view and the BZ failure view) - restrict by job and release
It's much easier to remove the restrictions after clicking that knowing the format to create them to begin with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it already does those things today, so is this change removing that behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In master, the only spot I found setting SearchLink is here:
Lines 708 to 710 in 953f785
testSearchUrl := gohtml.EscapeString(regexp.QuoteMeta(test.Name)) | |
testLink := fmt.Sprintf("<a target=\"_blank\" href=\"https://search.ci.openshift.org/?maxAge=48h&context=1&type=bug%%2Bjunit&name=&maxMatches=5&maxBytes=20971520&groupBy=job&search=%s\">%s</a>", testSearchUrl, test.Name) | |
test.SearchLink = testLink |
However I think that testresults in the various ByFoo
should have different SearchLinks.
This starts to bring a more principled pipeline and will help developers understand what information is available at what point. Also, turns out some fields were never filled in before!