Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Add support for filtering change types when diffing #303

Merged
merged 2 commits into from
Nov 29, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion api_handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
41 changes: 41 additions & 0 deletions params.go
Original file line number Diff line number Diff line change
Expand Up @@ -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':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment around here as well that this is inspired by git --diff-filter=, which, BTW, I really like :) I don't think that rename detection is in any way a straightforward thing, or likely to ever be worth doing, but if we do, it should be 'R'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted, and Done.

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
}
39 changes: 39 additions & 0 deletions params_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
53 changes: 32 additions & 21 deletions run_diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff tool isn't great. FWIW, changes are just:

  • Surround whole code-block in the if-statement above
  • Add continue if-statements

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, very helpful.

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
Expand Down
33 changes: 31 additions & 2 deletions run_diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down