From 09c5dddd5fd1b3deaa4bc84094799a240ae2fc9c Mon Sep 17 00:00:00 2001 From: Luke Bjerring Date: Wed, 29 Nov 2017 12:26:16 -0500 Subject: [PATCH] Add support for filtering change types when diffing (#303) * Add support for filtering change types when diffing * Add note that Git inspired filter param --- api_handlers.go | 8 +++++++- params.go | 41 +++++++++++++++++++++++++++++++++++++ params_test.go | 39 +++++++++++++++++++++++++++++++++++ run_diff.go | 53 +++++++++++++++++++++++++++++------------------- run_diff_test.go | 33 ++++++++++++++++++++++++++++-- 5 files changed, 150 insertions(+), 24 deletions(-) diff --git a/api_handlers.go b/api_handlers.go index bc5207935..3a018a7fe 100644 --- a/api_handlers.go +++ b/api_handlers.go @@ -288,7 +288,13 @@ func apiDiffHandler(w http.ResponseWriter, r *http.Request) { return } - diffJSON := diffResults(beforeJSON, afterJSON) + var filter DiffFilterParam + if filter, err = ParseDiffFilterParam(r); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + diffJSON := getResultsDiff(beforeJSON, afterJSON, filter) var bytes []byte if bytes, err = json.Marshal(diffJSON); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) diff --git a/params.go b/params.go index 276864720..6da0f0d36 100644 --- a/params.go +++ b/params.go @@ -112,3 +112,44 @@ func ParseMaxCountParam(r *http.Request) (count int, err error) { } return count, err } + +// DiffFilterParam represents the types of changed test paths to include. +type DiffFilterParam struct { + // Added tests are present in the 'after' state of the diff, but not present + // in the 'before' state of the diff. + Added bool + + // Deleted tests are present in the 'before' state of the diff, but not present + // in the 'after' state of the diff. + Deleted bool + + // Changed tests are present in both the 'before' and 'after' states of the diff, + // but the number of passes, failures, or total tests has changed. + Changed bool +} + +// ParseDiffFilterParam splits the filter param into the differences to include. +// The filter param is inspired by Git's --diff-filter flag. +func ParseDiffFilterParam(r *http.Request) (param DiffFilterParam, err error) { + param = DiffFilterParam{ + true, + true, + true, + } + if filter := r.URL.Query().Get("filter"); filter != "" { + param = DiffFilterParam{} + for _, char := range filter { + switch char { + case 'A': + param.Added = true + case 'D': + param.Deleted = true + case 'C': + param.Changed = true + default: + return param, fmt.Errorf("invalid filter character %c", char) + } + } + } + return param, nil +} diff --git a/params_test.go b/params_test.go index b4fed3d6e..a23e3371b 100644 --- a/params_test.go +++ b/params_test.go @@ -178,3 +178,42 @@ func TestParseMaxCountParam_TooLarge(t *testing.T) { assert.Nil(t, err) assert.Equal(t, MaxCountMaxValue, count) } + +func TestParseDiffFilterParam(t *testing.T) { + r := httptest.NewRequest("GET", "http://wpt.fyi/api/diff?filter=A", nil) + filter, _ := ParseDiffFilterParam(r) + assert.Equal(t, DiffFilterParam{true, false, false}, filter) + + r = httptest.NewRequest("GET", "http://wpt.fyi/api/diff?filter=D", nil) + filter, _ = ParseDiffFilterParam(r) + assert.Equal(t, DiffFilterParam{false, true, false}, filter) + + r = httptest.NewRequest("GET", "http://wpt.fyi/api/diff?filter=C", nil) + filter, _ = ParseDiffFilterParam(r) + assert.Equal(t, DiffFilterParam{false, false, true}, filter) + + r = httptest.NewRequest("GET", "http://wpt.fyi/api/diff?filter=CAD", nil) + filter, _ = ParseDiffFilterParam(r) + assert.Equal(t, DiffFilterParam{true, true, true}, filter) + + r = httptest.NewRequest("GET", "http://wpt.fyi/api/diff?filter=CD", nil) + filter, _ = ParseDiffFilterParam(r) + assert.Equal(t, DiffFilterParam{false, true, true}, filter) + + r = httptest.NewRequest("GET", "http://wpt.fyi/api/diff?filter=CACA", nil) + filter, _ = ParseDiffFilterParam(r) + assert.Equal(t, DiffFilterParam{true, false, true}, filter) +} + +func TestParseDiffFilterParam_Empty(t *testing.T) { + r := httptest.NewRequest("GET", "http://wpt.fyi/api/diff", nil) + filter, err := ParseDiffFilterParam(r) + assert.Nil(t, err) + assert.Equal(t, true, filter.Changed && filter.Added && filter.Deleted) +} + +func TestParseDiffFilterParam_Invalid(t *testing.T) { + r := httptest.NewRequest("GET", "http://wpt.fyi/api/diff?filter=Z", nil) + _, err := ParseDiffFilterParam(r) + assert.NotNil(t, err) +} diff --git a/run_diff.go b/run_diff.go index 510c32564..923cb179c 100644 --- a/run_diff.go +++ b/run_diff.go @@ -124,32 +124,43 @@ func fetchRunResultsJSON(ctx context.Context, r *http.Request, run TestRun) (res return results, nil } -// diffResults returns a map of test name to an array of [count-different-tests, total-tests], for tests which had +// getResultsDiff returns a map of test name to an array of [count-different-tests, total-tests], for tests which had // different results counts in their map (which is test name to array of [count-passed, total-tests]). -func diffResults(before map[string][]int, after map[string][]int) map[string][]int { +// +func getResultsDiff(before map[string][]int, after map[string][]int, filter DiffFilterParam) map[string][]int { diff := make(map[string][]int) - for test, resultsBefore := range before { - if resultsAfter, ok := after[test]; !ok { - // Missing? Then N / N tests are 'different' - diff[test] = []int{resultsBefore[1], resultsBefore[1]} - } else { - passDiff := abs(resultsBefore[0] - resultsAfter[0]) - countDiff := abs(resultsBefore[1] - resultsAfter[1]) - if countDiff == 0 && passDiff == 0 { - continue - } - // Changed tests is at most the number of different outcomes, - // but newly introduced tests should still be counted (e.g. 0/2 => 0/5) - diff[test] = []int{ - max(passDiff, countDiff), - max(resultsBefore[1], resultsAfter[1]), + if filter.Deleted || filter.Changed { + for test, resultsBefore := range before { + if resultsAfter, ok := after[test]; !ok { + // Missing? Then N / N tests are 'different'. + if !filter.Deleted { + continue + } + diff[test] = []int{resultsBefore[1], resultsBefore[1]} + } else { + if !filter.Changed { + continue + } + passDiff := abs(resultsBefore[0] - resultsAfter[0]) + countDiff := abs(resultsBefore[1] - resultsAfter[1]) + if countDiff == 0 && passDiff == 0 { + continue + } + // Changed tests is at most the number of different outcomes, + // but newly introduced tests should still be counted (e.g. 0/2 => 0/5) + diff[test] = []int{ + max(passDiff, countDiff), + max(resultsBefore[1], resultsAfter[1]), + } } } } - for test, resultsAfter := range after { - if _, ok := before[test]; !ok { - // Missing? Then N / N tests are 'different' - diff[test] = []int{resultsAfter[1], resultsAfter[1]} + if filter.Added { + for test, resultsAfter := range after { + if _, ok := before[test]; !ok { + // Missing? Then N / N tests are 'different' + diff[test] = []int{resultsAfter[1], resultsAfter[1]} + } } } return diff diff --git a/run_diff_test.go b/run_diff_test.go index f9f93edee..7c634b694 100644 --- a/run_diff_test.go +++ b/run_diff_test.go @@ -69,17 +69,46 @@ func TestDiffResults_Removed(t *testing.T) { assertDelta(t, []int{1, 2}, []int{1, 1}, []int{1, 2}) } +func TestDiffResults_Filtered(t *testing.T) { + changedFilter := DiffFilterParam{Changed: true} + addedFilter := DiffFilterParam{Added: true} + deletedFilter := DiffFilterParam{Deleted: true} + const removedPath = "/mock/removed.html" + const changedPath = "/mock/changed.html" + const addedPath = "/mock/added.html" + + before := map[string][]int{ + removedPath: {1, 2}, + changedPath: {2, 5}, + } + after := map[string][]int{ + changedPath: {3, 5}, + addedPath: {1, 3}, + } + assert.Equal(t, map[string][]int{changedPath: {1, 5}}, getResultsDiff(before, after, changedFilter)) + assert.Equal(t, map[string][]int{addedPath: {3, 3}}, getResultsDiff(before, after, addedFilter)) + assert.Equal(t, map[string][]int{removedPath: {2, 2}}, getResultsDiff(before, after, deletedFilter)) +} + func assertNoDeltaDifferences(t *testing.T, before []int, after []int) { + assertNoDeltaDifferencesWithFilter(t, before, after, DiffFilterParam{true, true, true}) +} + +func assertNoDeltaDifferencesWithFilter(t *testing.T, before []int, after []int, filter DiffFilterParam) { rBefore, rAfter := getDeltaResultsMaps(before, after) - assert.Equal(t, map[string][]int{}, diffResults(rBefore, rAfter)) + assert.Equal(t, map[string][]int{}, getResultsDiff(rBefore, rAfter, filter)) } func assertDelta(t *testing.T, before []int, after []int, delta []int) { + assertDeltaWithFilter(t, before, after, delta, DiffFilterParam{true, true, true}) +} + +func assertDeltaWithFilter(t *testing.T, before []int, after []int, delta []int, filter DiffFilterParam) { rBefore, rAfter := getDeltaResultsMaps(before, after) assert.Equal( t, map[string][]int{mockTestPath: delta}, - diffResults(rBefore, rAfter)) + getResultsDiff(rBefore, rAfter, filter)) } func getDeltaResultsMaps(before []int, after []int) (map[string][]int, map[string][]int) {