Skip to content

Commit cee243f

Browse files
Use searchcache results if summary file is old format (#2920)
* mark new summaries with version file * Changes suggested by @jcscottiii * python formatting * python formatting fixes * change function args * fix go go tests * fix medium tests * Better error handling * change results file names to v2 * Revert "fix medium tests" This reverts commit 15eb662. * Revert "fix go go tests" This reverts commit 5853b78. * add _v2 to summary references * fix small tests * refactoring search.go * add "_v2" suffix * old v2 for diff view * handle searchcache unavailable * update test data links * remove version file creation * code cleanup * test suffix fix * Update populate_dev_data.go * update test data with correct names * test file renames * changes based on reviews * remove references to old summary format * more test fixes * remove unnecessary testing tag * changes suggested by @jcscottiii * fix logic * commenting for export * error handling suggested by @jcscottiii
1 parent 0753645 commit cee243f

33 files changed

+412
-373
lines changed

api/README.md

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ __`pr`__ (Beta): GitHub PR number. Shows runs for commits that belong to the PR.
7171
"os_version": "4.4",
7272
"revision": "2bd11b91d4",
7373
"full_revision_hash": "2bd11b91d490ddd5237bcb6d8149a7f25faaa101",
74-
"results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary.json.gz",
74+
"results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary_v2.json.gz",
7575
"created_at": "2018-06-05T08:27:30.627865Z",
7676
"raw_results_url": "https://storage.googleapis.com/wptd-results/2bd11b91d490ddd5237bcb6d8149a7f25faaa101/chrome_67.0.3396.62_linux_4.4/report.json"
7777
}
@@ -97,7 +97,7 @@ https://wpt.fyi/api/runs/5184362994728960
9797
"os_version": "4.4",
9898
"revision": "2bd11b91d4",
9999
"full_revision_hash": "2bd11b91d490ddd5237bcb6d8149a7f25faaa101",
100-
"results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary.json.gz",
100+
"results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary_v2.json.gz",
101101
"created_at": "2018-06-05T08:27:30.627865Z",
102102
"raw_results_url": "https://storage.googleapis.com/wptd-results/2bd11b91d490ddd5237bcb6d8149a7f25faaa101/chrome_67.0.3396.62_linux_4.4/report.json"
103103
}
@@ -128,7 +128,7 @@ https://wpt.fyi/api/run?sha=latest&product=chrome
128128
"os_version": "4.4",
129129
"revision": "2bd11b91d4",
130130
"full_revision_hash": "2bd11b91d490ddd5237bcb6d8149a7f25faaa101",
131-
"results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary.json.gz",
131+
"results_url": "https://storage.googleapis.com/wptd/2bd11b91d4/chrome-stable-linux-summary_v2.json.gz",
132132
"created_at": "2018-06-05T08:27:30.627865Z",
133133
"raw_results_url": "https://storage.googleapis.com/wptd-results/2bd11b91d490ddd5237bcb6d8149a7f25faaa101/chrome_67.0.3396.62_linux_4.4/report.json"
134134
}
@@ -196,7 +196,7 @@ __`sha`__ : SHA[0:10] of the TestRun to fetch, or the keyword `latest`. Defaults
196196

197197
https://wpt.fyi/api/results?product=chrome
198198

199-
<details><summary><b>Example JSON</b> (from the summary.json.gz output)</summary>
199+
<details><summary><b>Example JSON</b> (from the summary_v2.json.gz output)</summary>
200200

201201
{
202202
"/css/css-text/i18n/css3-text-line-break-opclns-213.html": [1, 1],
@@ -406,7 +406,7 @@ NOTE: structured search queries are not supported.
406406
"os_version": "16.04",
407407
"revision": "2dda7b8c10",
408408
"full_revision_hash": "2dda7b8c10c7566fa6167a32b09c85d51baf2a85",
409-
"results_url": "https:\/\/storage.googleapis.com\/wptd-staging\/2dda7b8c10c7566fa6167a32b09c85d51baf2a85\/chrome-68.0.3440.106-linux-16.04-edf200244e-summary.json.gz",
409+
"results_url": "https:\/\/storage.googleapis.com\/wptd-staging\/2dda7b8c10c7566fa6167a32b09c85d51baf2a85\/chrome-68.0.3440.106-linux-16.04-edf200244e-summary_v2.json.gz",
410410
"created_at": "2018-08-17T08:12:29.219847Z",
411411
"time_start": "2018-08-17T06:26:52.33Z",
412412
"time_end": "2018-08-17T07:50:09.155Z",
@@ -425,7 +425,7 @@ NOTE: structured search queries are not supported.
425425
"os_version": "16.04",
426426
"revision": "2dda7b8c10",
427427
"full_revision_hash": "2dda7b8c10c7566fa6167a32b09c85d51baf2a85",
428-
"results_url": "https:\/\/storage.googleapis.com\/wptd-staging\/2dda7b8c10c7566fa6167a32b09c85d51baf2a85\/firefox-61.0.2-linux-16.04-75ff911c43-summary.json.gz",
428+
"results_url": "https:\/\/storage.googleapis.com\/wptd-staging\/2dda7b8c10c7566fa6167a32b09c85d51baf2a85\/firefox-61.0.2-linux-16.04-75ff911c43-summary_v2.json.gz",
429429
"created_at": "2018-08-17T08:31:38.580221Z",
430430
"time_start": "2018-08-17T06:47:29.643Z",
431431
"time_end": "2018-08-17T08:15:18.612Z",

api/query/query.go

Lines changed: 42 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,19 @@ package query
66

77
import (
88
"encoding/json"
9+
"errors"
910
"fmt"
1011
"net/http"
12+
"net/url"
1113
"strconv"
14+
"strings"
1215
"sync"
1316

1417
"github.com/web-platform-tests/wpt.fyi/shared"
1518
)
1619

17-
// SummaryResult is the format of the data from summary files generated with the newest aggregation method.
20+
// SummaryResult is the format of the data from summary files generated with the
21+
// newest aggregation method.
1822
type SummaryResult struct {
1923
// Status represents the 1-2 character abbreviation for the status of the test.
2024
Status string `json:"s"`
@@ -23,22 +27,18 @@ type SummaryResult struct {
2327
}
2428

2529
// summary is the golang type for the JSON format in pass/total summary files.
26-
// It has an old structure and a new structure - each which represent summary files
27-
// that match the old or new summary format.
28-
type summary struct {
29-
// oldFormat This holds summary information if the data is aggregated with the old method.
30-
// TODO (danielrsmith): This format should be removed once old summary files are invalidated.
31-
oldFormat map[string][]int
32-
// newFormat This holds summary information if the data is aggregated with the new method.
33-
newFormat map[string]SummaryResult
34-
}
30+
type summary map[string]SummaryResult
31+
3532
type queryHandler struct {
3633
store shared.Datastore
3734
dataSource shared.CachedStore
3835
client *http.Client
3936
logger shared.Logger
4037
}
4138

39+
// ErrBadSummaryVersion occurs when the summary file URL is not the correct version.
40+
var ErrBadSummaryVersion = errors.New("invalid/unsupported summary version")
41+
4242
func (qh queryHandler) processInput(w http.ResponseWriter, r *http.Request) (*shared.QueryFilter, shared.TestRuns, []summary, error) {
4343
filters, err := shared.ParseQueryFilterParams(r.URL.Query())
4444
if err != nil {
@@ -61,6 +61,31 @@ func (qh queryHandler) processInput(w http.ResponseWriter, r *http.Request) (*sh
6161
return &filters, testRuns, summaries, nil
6262
}
6363

64+
func (qh queryHandler) validateSummaryVersions(v url.Values, logger shared.Logger) error {
65+
filters, err := shared.ParseQueryFilterParams(v)
66+
if err != nil {
67+
return err
68+
}
69+
testRuns, _, err := qh.getRunsAndFilters(filters)
70+
if err != nil {
71+
return err
72+
}
73+
74+
for _, testRun := range testRuns {
75+
summaryURL := shared.GetResultsURL(testRun, "")
76+
if !qh.summaryIsValid(summaryURL) {
77+
logger.Infof("summary URL has invalid suffix: %s", summaryURL)
78+
return fmt.Errorf("%w for URL %s", ErrBadSummaryVersion, summaryURL)
79+
}
80+
}
81+
return nil
82+
}
83+
84+
func (qh queryHandler) summaryIsValid(summaryURL string) bool {
85+
// All new summary URLs end with "-summary_v2.json.gz". Any others are invalid.
86+
return strings.HasSuffix(summaryURL, "-summary_v2.json.gz")
87+
}
88+
6489
func (qh queryHandler) getRunsAndFilters(in shared.QueryFilter) (shared.TestRuns, shared.QueryFilter, error) {
6590
filters := in
6691
var testRuns shared.TestRuns
@@ -108,25 +133,17 @@ func (qh queryHandler) loadSummaries(testRuns shared.TestRuns) ([]summary, error
108133
defer wg.Done()
109134

110135
var data []byte
111-
s := summary{
112-
oldFormat: nil,
113-
newFormat: nil,
114-
}
136+
s := summary{}
115137
data, loadErr := qh.loadSummary(testRun)
116138
if err == nil && loadErr != nil {
117139
err = fmt.Errorf("Failed to load test run %v: %s", testRun.ID, loadErr.Error())
118140
return
119141
}
120142
// Try to unmarshal the json using the new aggregation structure.
121-
marshalErr := json.Unmarshal(data, &s.newFormat)
143+
marshalErr := json.Unmarshal(data, &s)
122144
if err == nil && marshalErr != nil {
123-
// If that failed, this is likely an old summary format.
124-
// Umarshal using the old structure.
125-
oldMarshalErr := json.Unmarshal(data, &s.oldFormat)
126-
if oldMarshalErr != nil {
127-
err = oldMarshalErr
128-
return
129-
}
145+
err = marshalErr
146+
return
130147
}
131148
summaries[i] = s
132149
}(i, testRun)
@@ -137,15 +154,15 @@ func (qh queryHandler) loadSummaries(testRuns shared.TestRuns) ([]summary, error
137154
}
138155

139156
func (qh queryHandler) loadSummary(testRun shared.TestRun) ([]byte, error) {
140-
mkey := getRedisKey(testRun)
157+
mkey := getSummaryFileRedisKey(testRun)
141158
url := shared.GetResultsURL(testRun, "")
142159
var data []byte
143160
err := qh.dataSource.Get(mkey, url, &data)
144161
return data, err
145162
}
146163

147-
func getRedisKey(testRun shared.TestRun) string {
148-
return "RESULTS_SUMMARY-" + strconv.FormatInt(testRun.ID, 10)
164+
func getSummaryFileRedisKey(testRun shared.TestRun) string {
165+
return "RESULTS_SUMMARY_v2-" + strconv.FormatInt(testRun.ID, 10)
149166
}
150167

151168
func isRequestCacheable(r *http.Request) bool {

api/query/query_test.go

Lines changed: 30 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -20,18 +20,18 @@ import (
2020
)
2121

2222
func TestGetRedisKey(t *testing.T) {
23-
assert.Equal(t, "RESULTS_SUMMARY-1", getRedisKey(shared.TestRun{
23+
assert.Equal(t, "RESULTS_SUMMARY_v2-1", getSummaryFileRedisKey(shared.TestRun{
2424
ID: 1,
2525
}))
2626
}
2727

28-
func TestLoadOldSummaries_success(t *testing.T) {
28+
func TestLoadSummaries_success(t *testing.T) {
2929
mockCtrl := gomock.NewController(t)
3030
defer mockCtrl.Finish()
3131

3232
urls := []string{
33-
"https://example.com/1-summary.json.gz",
34-
"https://example.com/2-summary.json.gz",
33+
"https://example.com/1-summary_v2.json.gz",
34+
"https://example.com/2-summary_v2.json.gz",
3535
}
3636
testRuns := []shared.TestRun{
3737
{
@@ -44,64 +44,8 @@ func TestLoadOldSummaries_success(t *testing.T) {
4444
},
4545
}
4646
keys := []string{
47-
getRedisKey(testRuns[0]),
48-
getRedisKey(testRuns[1]),
49-
}
50-
51-
cachedStore := sharedtest.NewMockCachedStore(mockCtrl)
52-
sh := unstructuredSearchHandler{queryHandler{dataSource: cachedStore}}
53-
summaryBytes := [][]byte{
54-
[]byte(`{"/a/b/c":[1,2]}`),
55-
[]byte(`{"/x/y/z":[3,4]}`),
56-
}
57-
summaries := []summary{
58-
{
59-
oldFormat: map[string][]int{"/a/b/c": {1, 2}},
60-
newFormat: map[string]SummaryResult{"/a/b/c": {Status: "", Counts: []int(nil)}},
61-
},
62-
{
63-
oldFormat: map[string][]int{"/x/y/z": {3, 4}},
64-
newFormat: map[string]SummaryResult{"/x/y/z": {Status: "", Counts: []int(nil)}},
65-
},
66-
}
67-
68-
bindCopySlice := func(i int) func(_, _, _ interface{}) {
69-
return func(cid, sid, iv interface{}) {
70-
ptr := iv.(*[]byte)
71-
*ptr = summaryBytes[i]
72-
}
73-
}
74-
for i, key := range keys {
75-
cachedStore.EXPECT().Get(key, urls[i], gomock.Any()).Do(bindCopySlice(i)).Return(nil)
76-
}
77-
78-
ss, err := sh.loadSummaries(testRuns)
79-
assert.Nil(t, err)
80-
assert.Equal(t, summaries[0], ss[0])
81-
assert.Equal(t, summaries[1], ss[1])
82-
}
83-
84-
func TestLoadNewSummaries_success(t *testing.T) {
85-
mockCtrl := gomock.NewController(t)
86-
defer mockCtrl.Finish()
87-
88-
urls := []string{
89-
"https://example.com/1-summary.json.gz",
90-
"https://example.com/2-summary.json.gz",
91-
}
92-
testRuns := []shared.TestRun{
93-
{
94-
ID: 1,
95-
ResultsURL: urls[0],
96-
},
97-
{
98-
ID: 2,
99-
ResultsURL: urls[1],
100-
},
101-
}
102-
keys := []string{
103-
getRedisKey(testRuns[0]),
104-
getRedisKey(testRuns[1]),
47+
getSummaryFileRedisKey(testRuns[0]),
48+
getSummaryFileRedisKey(testRuns[1]),
10549
}
10650

10751
cachedStore := sharedtest.NewMockCachedStore(mockCtrl)
@@ -111,14 +55,8 @@ func TestLoadNewSummaries_success(t *testing.T) {
11155
[]byte(`{"/x/y/z":{"s":"E","c":[3,4]}}`),
11256
}
11357
summaries := []summary{
114-
{
115-
oldFormat: nil,
116-
newFormat: map[string]SummaryResult{"/a/b/c": {Status: "O", Counts: []int{1, 2}}},
117-
},
118-
{
119-
oldFormat: nil,
120-
newFormat: map[string]SummaryResult{"/x/y/z": {Status: "E", Counts: []int{3, 4}}},
121-
},
58+
map[string]SummaryResult{"/a/b/c": {Status: "O", Counts: []int{1, 2}}},
59+
map[string]SummaryResult{"/x/y/z": {Status: "E", Counts: []int{3, 4}}},
12260
}
12361

12462
bindCopySlice := func(i int) func(_, _, _ interface{}) {
@@ -142,8 +80,8 @@ func TestLoadSummaries_fail(t *testing.T) {
14280
defer mockCtrl.Finish()
14381

14482
urls := []string{
145-
"https://example.com/1-summary.json.gz",
146-
"https://example.com/2-summary.json.gz",
83+
"https://example.com/1-summary_v2.json.gz",
84+
"https://example.com/2-summary_v2.json.gz",
14785
}
14886
testRuns := []shared.TestRun{
14987
{
@@ -156,14 +94,14 @@ func TestLoadSummaries_fail(t *testing.T) {
15694
},
15795
}
15896
keys := []string{
159-
getRedisKey(testRuns[0]),
160-
getRedisKey(testRuns[1]),
97+
getSummaryFileRedisKey(testRuns[0]),
98+
getSummaryFileRedisKey(testRuns[1]),
16199
}
162100

163101
cachedStore := sharedtest.NewMockCachedStore(mockCtrl)
164102
sh := unstructuredSearchHandler{queryHandler{dataSource: cachedStore}}
165103
summaryBytes := [][]byte{
166-
[]byte(`{"/a/b/c":[1,2]}`),
104+
[]byte(`{"/a/b/c":{"s":"O","c":[1,2]}}`),
167105
}
168106

169107
storeMiss := errors.New("No such summary file")
@@ -177,6 +115,19 @@ func TestLoadSummaries_fail(t *testing.T) {
177115
assert.Contains(t, err.Error(), storeMiss.Error())
178116
}
179117

118+
func TestSummaryIsValid_v1(t *testing.T) {
119+
qh := queryHandler{}
120+
// Summaries without the "_v2" suffix should not be used.
121+
url := "https://example.com/invalid-summary.json.gz"
122+
assert.False(t, qh.summaryIsValid(url))
123+
}
124+
125+
func TestSummaryIsValid_v2(t *testing.T) {
126+
qh := queryHandler{}
127+
url := "https://example.com/valid-summary_v2.json.gz"
128+
assert.True(t, qh.summaryIsValid(url))
129+
}
130+
180131
func TestGetRunsAndFilters_default(t *testing.T) {
181132
mockCtrl := gomock.NewController(t)
182133
defer mockCtrl.Finish()
@@ -190,8 +141,8 @@ func TestGetRunsAndFilters_default(t *testing.T) {
190141

191142
runIDs := []int64{1, 2}
192143
urls := []string{
193-
"https://example.com/1-summary.json.gz",
194-
"https://example.com/2-summary.json.gz",
144+
"https://example.com/1-summary_v2.json.gz",
145+
"https://example.com/2-summary_v2.json.gz",
195146
}
196147
chrome, _ := shared.ParseProductSpec("chrome")
197148
edge, _ := shared.ParseProductSpec("edge")
@@ -240,8 +191,8 @@ func TestGetRunsAndFilters_specificRunIDs(t *testing.T) {
240191

241192
runIDs := []int64{1, 2}
242193
urls := []string{
243-
"https://example.com/1-summary.json.gz",
244-
"https://example.com/2-summary.json.gz",
194+
"https://example.com/1-summary_v2.json.gz",
195+
"https://example.com/2-summary_v2.json.gz",
245196
}
246197
chrome, _ := shared.ParseProductSpec("chrome")
247198
edge, _ := shared.ParseProductSpec("edge")

0 commit comments

Comments
 (0)