Run diff prototype #228

lukebjerring · 2017-11-10T20:15:19Z

Running at https://20171122t141813-dot-wptdashboard.appspot.com/

Example: Chrome 7a0cf8ade7 vs 6ed1ba18d0 (Nov 2nd vs Nov 4th)
https://20171122t141813-dot-wptdashboard.appspot.com/api/diff?before=chrome@7a0cf8ade7&after=chrome@6ed1ba18d0

This PR adds /api/diff?before=platform@sha&after=platform@sha endpoint, for (dangerously simplified) summaries of the diff counts between 2 runs.

Produces the same JSON format as /results?browser={platform}&sha={sha}, only instead of

{ "foo": [pass, total], "bar": ... }

it produces

{ "foo": [pass_delta, max(total_tests)], ... }

And omits anything with the same counts. This format can* then be piped into <wpt-results>

* Not yet prototyped

/cc @jeffcarp

foolip · 2017-11-22T22:31:03Z

I can't see "summary" in the diff, but this is based on the summary pass/fail numbers alone, right? Would anything much need to change in order to use the full pass/fail data once that's available in a more convenient format than tens of thousands of individual files?

lukebjerring · 2017-11-23T00:02:24Z

No, should be straightforward to diff all the data once it's unsharded. This current state does not include subtests.

…

On Wed, Nov 22, 2017, 5:31 PM Philip Jägenstedt ***@***.***> wrote: I can't see "summary" in the diff, but this is based on the summary pass/fail numbers alone, right? Would anything much need to change in order to use the full pass/fail data once that's available in a more convenient format than tens of thousands of individual files? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#228 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAve5DsFsfiIM_ZzTs9cawwb9C5zETufks5s5KCogaJpZM4QaDd3> .

foolip · 2017-11-23T15:11:25Z

api_handlers.go

+		return
+	}
+
+	diffJSON := diffResults(beforeJSON, afterJSON)


My first time reading go code. If I'm reading this right, almost everything up to this point is error handling :)

Yeah, go's error handling is .. verbose.

foolip · 2017-11-23T15:15:08Z

run_diff.go

+	} else {
+		platformAtRevision.Revision = pieces[1]
+	}
+	if IsBrowserName(platformAtRevision.Platform) {


Can you add a comment about this? Seems curious to check if the platform is a browser.

Platform is actually browser[-version[-os[-version]]], in this case enforced as only the browser.

foolip · 2017-11-23T15:33:07Z

run_diff.go

+
+	var results []TestRun
+	query := baseQuery.
+		Filter("BrowserName =", revision.Platform)


This looks funny in the same way, so now I'm thinking we're just using browser and platform as synonyms in these parts. Is that the case?

TODO's added.

foolip · 2017-11-23T15:34:23Z

run_diff.go

+	return results[0], nil
+}
+
+func fetchRunResultsJSON(ctx context.Context, r *http.Request, run TestRun) (results map[string][]int, err error) {


Can you sprinkle some TODOs around here about not fetching lots of little files, and linking to the issue for that?

I don't think we should open the issue until this code lands (issues against branches feels dirty). However, I think this diff is valid once all data is in the one blob, so probably not a TODO for this code either.

foolip · 2017-11-23T15:35:35Z

run_diff.go

+		reqURL.Path = url
+	}
+	var resp *http.Response
+	if resp, err = client.Get(url); err != nil {


If this is where network errors end up, it seems somewhat likely that we'll see some. Will the client see a HTTP 500 then? If it's not very unlikely to happen, seems like some retry logic will be needed in the Travis integration?

Yes, 500, idk about the likelyhood, not going to overengineer until we see an actual problem (travis makes a HUGE number of network fetches for pip etc anyway).

Sure, I suppose that this will only make a handful of requests, I was thinking about what @mdittmer told me about in code where he fetched 10k files or so. If we expected that kind of load some precautions might be in order.

foolip · 2017-11-23T15:36:50Z

run_diff.go

+	return results, nil
+}
+
+func diffResults(before map[string][]int, after map[string][]int) map[string][]int {


@mdittmer, here are a bunch of loops over test results, this is the shape of things I was imagining would be involved in computing metrics. Just FYI, let's keep going in the design doc.

foolip · 2017-11-23T15:38:39Z

run_diff.go

+	diff := make(map[string][]int)
+	for test, resultsBefore := range before {
+		if resultsAfter, ok := after[test]; !ok {
+			// Missing? Then N / N tests are 'different'


Do we have any tests in the code base yet? This is a bit of code that would easier to review the tests for than convince oneself about correctness in edge cases by executing in head.

Will add some tests shortly and push them into this PR.

foolip · 2017-11-23T15:39:42Z

run_diff.go

+}
+
+func diffResults(before map[string][]int, after map[string][]int) map[string][]int {
+	diff := make(map[string][]int)


Can you add a comment about what the 2-tuple int represents here? I think "number of tests different" and "total number of tests known"?

foolip · 2017-11-23T15:42:25Z

util.go

+	}
+	for _, browser := range browsers {
+		if browser == name {
+			return true


Is there no array.contains(thing) in Go?

foolip · 2017-11-23T15:43:23Z

util.go

+	return false
+}
+
+func abs(x int) int {


Don't know go, but if https://golang.org/pkg/math/ is part of the standard library, is it most idiomatic to use that or roll your own?

math lib works with floats, not ints, arguing that ints are 'trivial to roll your own'.

(I don't like Go in that regard.)

foolip · 2017-11-24T15:12:27Z

run_diff_test.go

+	assertDelta(t, []int {0, 1}, []int {0, 2}, []int {1, 2})
+
+	// One new test, new test passing
+	assertDelta(t, []int {0, 1}, []int {1, 2}, []int {1, 2})


If I'm reading this right, the numbers returned as the same when a new test is pass and when it's failing, is that right? That would make it impossible to notify about new failing tests but not new passing tests, which I think we should.

Yeah, currently the case. Removing the abs call would let you see negative values; however, this API will be extended to take a filters param, and you'd filter to only newly-failing in the fetch which is used for notifying.

Sounds like a plan!

lukebjerring added 10 commits November 10, 2017 12:16

WIP

c45a166

WIP

a6044da

WIP

d9b85d9

Run gofmt

c29290b

Merge master branch

e0aad6f

Merge branch 'master' into run-diff

acfacc4

Merge branch 'master' into run-diff

53cace4

WIP

98ecc0f

Run gofmt

1efcefa

Fix broken query.

96ad6d1

lukebjerring requested review from jeffcarp and foolip November 22, 2017 19:45

foolip reviewed Nov 23, 2017

View reviewed changes

Luke Bjerring added 5 commits November 23, 2017 12:14

Changes as per review

f65e012

Merge branch 'master' into run-diff

fb76541

Merge branch 'master' into run-diff

16f133e

Add unit tests

4d5eab0

Run gofmt

9696b9a

foolip reviewed Nov 24, 2017

View reviewed changes

foolip approved these changes Nov 24, 2017

View reviewed changes

Merge branch 'master' into run-diff

630da32

lukebjerring removed the request for review from jeffcarp November 24, 2017 16:11

Remove duplicated message

5453666

lukebjerring merged commit 2329f76 into web-platform-tests:master Nov 24, 2017

lukebjerring deleted the run-diff branch November 24, 2017 16:51

foolip mentioned this pull request Nov 28, 2017

Revert "Dockerize WPT runs & add Jenkins k8s specs" and following changes #305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run diff prototype #228

Run diff prototype #228

lukebjerring commented Nov 10, 2017 •

edited

Loading

foolip commented Nov 22, 2017

lukebjerring commented Nov 23, 2017 via email

foolip Nov 23, 2017

lukebjerring Nov 23, 2017

foolip Nov 23, 2017

lukebjerring Nov 23, 2017

foolip Nov 23, 2017

lukebjerring Nov 23, 2017

foolip Nov 23, 2017

lukebjerring Nov 23, 2017 •

edited

Loading

foolip Nov 23, 2017

lukebjerring Nov 23, 2017

foolip Nov 24, 2017

foolip Nov 23, 2017

foolip Nov 23, 2017

lukebjerring Nov 23, 2017

foolip Nov 23, 2017

lukebjerring Nov 23, 2017

foolip Nov 23, 2017

lukebjerring Nov 23, 2017

foolip Nov 24, 2017

foolip Nov 23, 2017

lukebjerring Nov 23, 2017

foolip Nov 24, 2017 •

edited

Loading

lukebjerring Nov 24, 2017

foolip Nov 24, 2017

Run diff prototype #228

Run diff prototype #228

Conversation

lukebjerring commented Nov 10, 2017 • edited Loading

foolip commented Nov 22, 2017

lukebjerring commented Nov 23, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebjerring Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foolip Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebjerring commented Nov 10, 2017 •

edited

Loading

lukebjerring Nov 23, 2017 •

edited

Loading

foolip Nov 24, 2017 •

edited

Loading