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 1 commit
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
40 changes: 40 additions & 0 deletions params.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,3 +112,43 @@ 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.
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