Add test risk analysis html for spyglass #27508
Conversation
xueqzhan
force-pushed
the
risk-analysis-html
branch
from
3363933
to
85522ea
Compare
|
Wrong PR :0 ignore last comment. :) |
|
/retest-required |
xueqzhan
force-pushed
the
risk-analysis-html
branch
from
85522ea
to
2d5b958
Compare
|
/retest-required |
e2echart/test-risk-analysis.html
Outdated
| <p> | ||
| "Risk analysis is performed by Sippy[<a href="TEST_RISK_ANALYSIS_SIPPY_URL_GOES_HERE">link</a>] to attempt to | ||
| determine if the failures in this job are abnormal when compared to results for similar jobs over the past week, | ||
| and amidst on-going incidents in the CI infrastructure. Risk analysis API will not catch everything and is a | ||
| relatively simple implementation today, please reach out to the Technical Release Team if you spot abnormalities | ||
| or have suggestions." | ||
| </p> |
There was a problem hiding this comment.
Spyglass supports "info" tooltips, if you'd prefer to put most of this info except maybe the link in there. Example:
There was a problem hiding this comment.
Does the tooltip support links? If one puts the cursor on and get the tooltip, can he click on the link to get to sippy? I just experimented and it doesn't seem to support that.
There was a problem hiding this comment.
Right, I suggested not putting the link. You can leave everything here if you want, but it'd be more consistent if we described what the page is doing in the tooltip.
There was a problem hiding this comment.
Devan suggested the link. @dgoodwin do you prefer this text in tooltip without the link to Sippy, or a paragraph on the top of the page?
There was a problem hiding this comment.
My thought was here was just to link to top level Sippy page, so a user could see what it was. Not directly to risk analysis. I can live without either though if anyone feels strongly.
And if we keep it, I'd just do the word Sippy as the link, not a [] after it, that was just markdown slipping into my suggestions for the card.
There was a problem hiding this comment.
Sorry if I wasn't clear, I just meant putting the link outside of the tooltip somewhere on the page would be fine, but not including the wall of text someone probably only needs to read once.
There was a problem hiding this comment.
Is there a corresponding PR to openshift/release to add this as an HTML page spyglass should show? There's no reason it can't be there now so we could see the results in the presubmits
@deepsm007 Deep has a PR here: openshift/release#33731
You meant that PR can be merged now so that we can see the spyglass in our job runs. Good point.
pkg/riskanalysis/cmd.go
Outdated
| @@ -91,5 +93,16 @@ func (opt *Options) Run() error { | |||
| } | |||
| fmt.Fprintf(opt.Out, "Successfully wrote: %s\n", outputFile) | |||
|
|
|||
| // Write html file for spyglass | |||
| riskAnalysisHTMLTemplate := testdata.MustAsset("e2echart/test-risk-analysis.html") | |||
| title := fmt.Sprintf("Risk Analysis For Job: %s, ID: %d", finalProwJobRun.ProwJob, finalProwJobRun.ID) | |||
There was a problem hiding this comment.
Nit: the spyglass page already has the job name and ID prominently displayed at the top, I'd omit this and just leave it as "Risk Analysis"
xueqzhan
force-pushed
the
risk-analysis-html
branch
2 times, most recently
from
3432685
to
d67f411
Compare
|
/retest-required |
e2echart/test-risk-analysis.html
Outdated
| <h1> | ||
| </h1> | ||
| <p> | ||
| "<a href="TEST_RISK_ANALYSIS_SIPPY_URL_GOES_HERE">Link to Sippy</a>" |
There was a problem hiding this comment.
I don't think this should be in quotes.
This also just goes to the main page, will it eventually go somewhere else?
There was a problem hiding this comment.
There is nothing planned for this. The risk analysis API requires a body. The top page of Sippy is put here just for convenience purposes. I am open for suggestions though.
|
Is there a corresponding PR to openshift/release to add this as an HTML page spyglass should show? There's no reason it can't be there now so we could see the results in the presubmits |
e2echart/test-risk-analysis.html
Outdated
| // Build rows for all tests | ||
| for (var i = 0; i < testResult.Tests.length; i++) { | ||
| var row$ = $('<tr/>'); | ||
| row$.append($('<td/>').html(testResult.Tests[i].Name)); |
There was a problem hiding this comment.
Is it possible to put a link to the test analysis page in Sippy for each test? That seems more useful to me than a link to the main page of sippy
There was a problem hiding this comment.
As of now, this might not be easy. I need to change the risk analysis API to return release number first to generate the test case URL. Trying to figure out the release from prow job name is not the best way.
xueqzhan
force-pushed
the
risk-analysis-html
branch
2 times, most recently
from
eea44ec
to
522f640
Compare
e2echart/test-risk-analysis.html
Outdated
| for (var i = 0; i < testResult.Tests.length; i++) { | ||
| var row$ = $('<tr/>'); | ||
| testUrl = encodeURI(testLinkPrefix + testResult.Release + testLinkSuffix + testResult.Tests[i].Name) | ||
| row$.append($('<td/>').html("<a href=" + testUrl + ">" + testResult.Tests[i].Name + "</a>")); |
There was a problem hiding this comment.
Can we get this to open in a new tab?
| row$.append($('<td/>').html("<a href=" + testUrl + ">" + testResult.Tests[i].Name + "</a>")); | |
| row$.append($('<td/>').html("<a target=\"_blank\" href=" + testUrl + ">" + testResult.Tests[i].Name + "</a>")); |
|
It looks like the table is getting cut off, do we control the size of the iframe? The intervals chart seems to be able to be taller.
|
e2echart/test-risk-analysis.html
Outdated
| </head> | ||
| <body onLoad="buildTestCaseTable('#test_case_results')"> | ||
| <p> | ||
| <a href="TEST_RISK_ANALYSIS_SIPPY_URL_GOES_HERE">Link to Sippy</a> |
There was a problem hiding this comment.
| <a href="TEST_RISK_ANALYSIS_SIPPY_URL_GOES_HERE">Link to Sippy</a> | |
| <a target="_blank" href="TEST_RISK_ANALYSIS_SIPPY_URL_GOES_HERE">Link to Sippy</a> |
e2echart/test-risk-analysis.html
Outdated
| cell$ = $('<ul/>') | ||
| for (var i = 0; i < openBugs.length; i++) { | ||
| li$ = $('<li>') | ||
| bug = "<a href=" + openBugs[i].url + ">" + openBugs[i].id + "</a>: " + openBugs[i].summary |
There was a problem hiding this comment.
| bug = "<a href=" + openBugs[i].url + ">" + openBugs[i].id + "</a>: " + openBugs[i].summary | |
| bug = "<a target=\"_blank\" href=" + openBugs[i].url + ">" + openBugs[i].id + "</a>: " + openBugs[i].summary |
e2echart/test-risk-analysis.html
Outdated
| <meta name="description" | ||
| content="Risk analysis is performed by Sippy to attempt to determine if the failures in this job are | ||
| abnormal when compared to results for similar jobs over the past week, and amidst on-going incidents | ||
| in the CI infrastructure. Risk analysis API will not catch everything and is a relatively simple | ||
| implementation today, please reach out to the Technical Release Team if you spot abnormalities or | ||
| have suggestions."> | ||
| </head> |
There was a problem hiding this comment.
Can we get the text all on one line? Seems to not like the white space.
xueqzhan
force-pushed
the
risk-analysis-html
branch
from
522f640
to
c1a0ac5
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stbenjam, xueqzhan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest-required |
1 similar comment
|
/retest-required |
|
/override ci/prow/e2e-gcp-ovn-image-ecosystem |
|
@stbenjam: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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 kubernetes/test-infra repository. |
|
/override ci/prow/e2e-gcp-ovn |
|
@stbenjam: Overrode contexts on behalf of stbenjam: ci/prow/e2e-gcp-ovn 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 kubernetes/test-infra repository. |
|
/override ci/prow/e2e-aws-ovn-serial |
|
@stbenjam: Overrode contexts on behalf of stbenjam: ci/prow/e2e-aws-ovn-serial 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 kubernetes/test-infra repository. |
|
@xueqzhan: The following tests 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/test-infra repository. I understand the commands that are listed here. |
TRT-644