Add summary MD to checks #745
Conversation
|
Staging instance deployed by Travis CI! |
|
Staging instance deployed by Travis CI! |
lukebjerring
force-pushed
the
check-title
branch
from
e170095 to
b31600a
Compare
api/checks/webhook.go
Outdated
| hostname := shared.GetHostname(ctx) | ||
| detailsURL, _ := url.Parse(fmt.Sprintf("https://%s/results/", hostname)) | ||
| // Diff for the given product at master vs the given sha. | ||
| func getMasterDiffURL(ctx context.Context, sha string, product shared.ProductSpec) *url.URL { |
There was a problem hiding this comment.
This wasn't changed in this PR, but the link that will be exposed now isn't a stable one, and the comparison will quickly become full of differences due to changes on master as opposed to the PR. Can we get the run ID or product+sha and mint a stable URL instead?
There was a problem hiding this comment.
Let's discuss this in an issue instead; there's some implications in both directions.
The most-recent master run might be lagging behind when a PR lands (they're faster), so fixing to a value that's behind might not be desirable. Using the current SHA of master may also be a problem, if we aren't producing results for that SHA for some reason. And, as you suggest, having a rolling master might be bad.
There was a problem hiding this comment.
I think the run we want would be one for the merge base if it exists, something as soon as possible after if it doesn't, and something as soon as possible before if there aren't any runs after yet. (Reason to prefer runs after the merge base is that the PR might build on something recent that affects the results of the tests being changed.)
But yeah, separate issue sounds good, or we'll just make sure to have two runs always for PRs and not have any logic for this :)
| return false, err | ||
| } | ||
|
|
||
| title := fmt.Sprintf("wpt.fyi - %s results", product.DisplayName()) |
There was a problem hiding this comment.
Since this is the title of the check run and not the check suite, just product.DisplayName() as the name or perhaps including " results" seems fine. Where is the title of the check suite set? Simply "wpt.fyi" as the title would work I think?
There was a problem hiding this comment.
It's set in the App, I think. In the main landing page of a PR, we don't see the suite title, just the run title(s), so the context might be nicer. An example for another run is
Taskcluster (pull_request)
There was a problem hiding this comment.
Just as well to get this deployed then I can see what strings show up where directly :)
| @@ -0,0 +1,7 @@ | |||
| Results have successfully been scraped and added to [{{ .HostName }}]({{ .HostURL }}). | |||
There was a problem hiding this comment.
Is this the URL that will link to something like https://wpt.fyi/pr/1234?
There was a problem hiding this comment.
Not yet; we don't have support for that param (and I'd probably go ?pr=1234; the extra URL path handling cases isn't worth the extra headache).
It's currently just pointing to the homepage.
There was a problem hiding this comment.
Alright, ?pr=1234 would be cool.
| They can be compared to the latest `master` run here: | ||
| {{ .DiffURL }} | ||
|
|
||
| Or, to view _all_ the latest runs for the same revision: |
There was a problem hiding this comment.
Do you have an example of what this URL will be? From the wording it's not clear if "same revision" refers to the PR head revision or the master revision.
There was a problem hiding this comment.
Neither; the SHA that was associated with the check_run (which was, at the time, the PR HeadSHA)
There was a problem hiding this comment.
That'd be what I called PR head revision. Point is given the context I thought "same revision" would probably refer to "master", just that doesn't quite make sense.
Perhaps best to just land the PR so it's possible to see this in action to see what the URLs all are :)
| @@ -0,0 +1,3 @@ | |||
| Results have been produced, and are being collected as we speak... | |||
|
|
|||
| They'll eventually be visible on [{{ .HostName }}]({{ .RunsURL }}). | |||
There was a problem hiding this comment.
Oh, should this URL not be somewhere in completed.md too?
There was a problem hiding this comment.
I don't think so. The intention is that the /test-runs UI will show pending runs eventually, whereas once the run has landed, we can link straight to it.
There was a problem hiding this comment.
Oh, so this just links to /runs?
shared/params.go
Outdated
| @@ -141,6 +141,22 @@ func (p ProductSpec) Matches(run TestRun) bool { | |||
| return true | |||
| } | |||
|
|
|||
| // DisplayName returns a capitalized version of the product's name. | |||
There was a problem hiding this comment.
Do we have enough info to include the channel name here, or at the call site?
There was a problem hiding this comment.
Yes, the channel is available in the label set. I'd do that in a separate PR, though.
|
Ping @foolip |
Description
Adds a more user-friendly title and summary body to the checks.
Part of #712
Review Information
@Hexcles - I suspect my golang "what dir am I in" code might not play well on appengine. Do you happen to know whether it will work?