Skip to content

Commit 75c15fa

Browse files
committed
bugfix: Fix wptconsumer
Currently, the wpt consumer job is failing [logs](https://cloudlogging.app.goo.gl/Cqax2o5RFX342mXr7). The reason why it is failing comes from a few reasons: - We are using the imported Go code from wpt.fyi incorrectly. It causes the formed URL to be wrong and cause 404. - No `safari_ios` product exists in the product spec [code](https://github.com/web-platform-tests/wpt.fyi/blob/da8187c63fe9ac7e6dddb9137db5657063e32f74/shared/product_spec.go#L71-L110) This change addresses those. Additionally, I configured the linter to also check for exhaustiveness on maps and not just switch statements. That way we can update the integration test there too.
1 parent ebc9bad commit 75c15fa

7 files changed

Lines changed: 119 additions & 21 deletions

File tree

.golangci.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,10 @@ linters:
5757
dupl:
5858
# The default is 150
5959
threshold: 175
60+
exhaustive:
61+
check:
62+
- switch
63+
- map
6064
exclusions:
6165
generated: lax
6266
presets:

lib/gcpspanner/spanneradapters/chromium_historgram_enum_consumer_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
8181
},
8282
},
8383
data: metricdatatypes.HistogramMapping{
84-
"histogram": []metricdatatypes.HistogramEnumValue{
84+
metricdatatypes.WebDXFeatureEnum: []metricdatatypes.HistogramEnumValue{
8585
{Value: 1, Label: "EnumLabel"},
8686
},
8787
},
@@ -99,7 +99,7 @@ func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
9999
getIDFromFeatureKey: nil,
100100
},
101101
data: metricdatatypes.HistogramMapping{
102-
"histogram": []metricdatatypes.HistogramEnumValue{
102+
metricdatatypes.WebDXFeatureEnum: []metricdatatypes.HistogramEnumValue{
103103
{Value: 1, Label: "EnumLabel"},
104104
},
105105
},
@@ -120,7 +120,7 @@ func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
120120
getIDFromFeatureKey: nil,
121121
},
122122
data: metricdatatypes.HistogramMapping{
123-
"histogram": []metricdatatypes.HistogramEnumValue{
123+
metricdatatypes.WebDXFeatureEnum: []metricdatatypes.HistogramEnumValue{
124124
{Value: 1, Label: "EnumLabel"},
125125
},
126126
},
@@ -144,7 +144,7 @@ func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
144144
upsertWebFeatureChromiumHistogramEnumValue: nil,
145145
},
146146
data: metricdatatypes.HistogramMapping{
147-
"histogram": []metricdatatypes.HistogramEnumValue{
147+
metricdatatypes.WebDXFeatureEnum: []metricdatatypes.HistogramEnumValue{
148148
{Value: 1, Label: "EnumLabel"},
149149
},
150150
},
@@ -171,7 +171,7 @@ func TestChromiumHistogramEnumConsumer_SaveHistogramEnums(t *testing.T) {
171171
},
172172
},
173173
data: metricdatatypes.HistogramMapping{
174-
"histogram": []metricdatatypes.HistogramEnumValue{
174+
metricdatatypes.WebDXFeatureEnum: []metricdatatypes.HistogramEnumValue{
175175
{Value: 1, Label: "EnumLabel"},
176176
},
177177
},

lib/gcpspanner/spanneradapters/wptconsumertypes/types.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,17 @@ var ErrUnableToStoreWPTRunFeatureMetrics = errors.New("unable to store wpt run f
4343
// BrowserName is an enumeration of the supported browsers for WPT runs.
4444
type BrowserName string
4545

46+
// nolint:lll // WONTFIX: commit URL is useful
47+
// Only use browsers from
48+
// https://github.com/web-platform-tests/wpt.fyi/blob/da8187c63fe9ac7e6dddb9137db5657063e32f74/shared/product_spec.go#L71-L110
49+
// to avoid a 400 error.
50+
// Update the link above when the next snapshot of the list is used.
51+
// Also, update the list used in workflows/steps/services/wpt_consumer/cmd/job/main.go so that it will be consumed.
4652
const (
4753
Chrome BrowserName = "chrome"
4854
Edge BrowserName = "edge"
4955
Firefox BrowserName = "firefox"
5056
Safari BrowserName = "safari"
5157
ChromeAndroid BrowserName = "chrome_android"
5258
FirefoxAndroid BrowserName = "firefox_android"
53-
SafariIos BrowserName = "safari_ios"
5459
)

lib/wptfyi/client.go

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ package wptfyi
1616

1717
import (
1818
"context"
19+
"strings"
1920
"time"
2021

2122
mapset "github.com/deckarep/golang-set"
@@ -44,9 +45,19 @@ func (w HTTPClient) GetRuns(
4445
//nolint:exhaustruct
4546
// External struct does not need comply with exhaustruct.
4647
apiOptions := shared.TestRunFilter{
47-
From: &from,
48+
From: &from,
49+
// TODO: Modify the upstream code so that we can use uint instead of int.
4850
MaxCount: &pageSize,
49-
Labels: mapset.NewSetWith(browserName, channelName),
51+
Products: shared.ProductSpecs{
52+
{
53+
ProductAtRevision: shared.ProductAtRevision{
54+
Product: shared.Product{
55+
BrowserName: browserName,
56+
},
57+
},
58+
Labels: mapset.NewSetWith(channelName),
59+
},
60+
},
5061
}
5162

5263
allRuns := shared.TestRuns{}
@@ -58,6 +69,11 @@ func (w HTTPClient) GetRuns(
5869
}
5970
runs, err := shared.FetchRuns(w.hostname, apiOptions)
6071
if err != nil {
72+
if is404Error(err) {
73+
// No more results
74+
break
75+
}
76+
6177
return nil, err
6278
}
6379

@@ -84,3 +100,13 @@ func (w HTTPClient) GetRuns(
84100

85101
return allRuns, nil
86102
}
103+
104+
func is404Error(err error) bool {
105+
// nolint:lll // WONTFIX: commit URL is useful
106+
// TODO. This is brittle. Instead, we should modify the imported wpt.fyi
107+
// upstream client code to return specific error values.
108+
// https://github.com/web-platform-tests/wpt.fyi/blob/da8187c63fe9ac7e6dddb9137db5657063e32f74/shared/fetch_runs.go#L24-L52
109+
errorStr := err.Error()
110+
111+
return strings.Contains(errorStr, "Bad response code") && strings.Contains(errorStr, ": 404")
112+
}

lib/wptfyi/client_integration_test.go

Lines changed: 74 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,84 @@ import (
1818
"context"
1919
"testing"
2020
"time"
21+
22+
"github.com/GoogleChrome/webstatus.dev/lib/gcpspanner/spanneradapters/wptconsumertypes"
2123
)
2224

2325
func TestGetRunsIntegration(t *testing.T) {
26+
specialNoResultsExpectedValue := -1
27+
type testCase struct {
28+
// expectedMinimumResultSize specifies the minimum number of runs expected for a given browser
29+
// and channel over the tested time period. This is used to verify that the client
30+
// correctly fetches all pages of results and not just the first page (which maxes out at 100).
31+
// A value of -1 indicates that no results are expected yet
32+
expectedMinimumResultSize int
33+
willFail bool
34+
}
2435
client := NewHTTPClient("wpt.fyi")
25-
pageSize := 100
26-
runs, err := client.GetRuns(context.TODO(), time.Now().AddDate(0, 0, -365).UTC(), pageSize, "chrome", "stable")
27-
if err != nil {
28-
t.Errorf("unexpected error getting runs: %s\n", err.Error())
36+
// longTermResultSize means the product has been around for awhile and should get at least 100 results
37+
longTermResultSize := 100
38+
browsers := map[wptconsumertypes.BrowserName]testCase{
39+
// Desktop browsers
40+
wptconsumertypes.Chrome: {
41+
expectedMinimumResultSize: longTermResultSize,
42+
willFail: false,
43+
},
44+
wptconsumertypes.Edge: {
45+
expectedMinimumResultSize: longTermResultSize,
46+
willFail: false,
47+
},
48+
wptconsumertypes.Firefox: {
49+
expectedMinimumResultSize: longTermResultSize,
50+
willFail: false,
51+
},
52+
wptconsumertypes.Safari: {
53+
expectedMinimumResultSize: longTermResultSize,
54+
willFail: false,
55+
},
56+
57+
// Mobile browsers
58+
// Stable results just started coming for ChromeAndroid
59+
wptconsumertypes.ChromeAndroid: {
60+
expectedMinimumResultSize: 1,
61+
willFail: false,
62+
},
63+
// No stable results for FirefoxAndroid yet
64+
wptconsumertypes.FirefoxAndroid: {
65+
expectedMinimumResultSize: specialNoResultsExpectedValue,
66+
willFail: false,
67+
},
68+
69+
// Bad browser name
70+
wptconsumertypes.BrowserName("badname"): {
71+
expectedMinimumResultSize: 0,
72+
willFail: true,
73+
},
2974
}
30-
// Looking back a year, we should have more than 100 runs given there is a one run per day
31-
// This test is only to make sure we get more than the pageSize of results because currently
32-
// the external client will fetch the first pageSize of results but there may be actually more.
33-
// Our code ensures we get all the pages, not just the first page.
34-
if len(runs) <= pageSize {
35-
t.Errorf("unexpected page size %d. expected more than %d runs", len(runs), pageSize)
75+
for browser, testCase := range browsers {
76+
// For now, we only care about stable channel.
77+
runs, err := client.GetRuns(context.TODO(), time.Now().AddDate(0, 0, -365).UTC(),
78+
longTermResultSize, string(browser), "stable")
79+
if err != nil && !testCase.willFail {
80+
t.Errorf("unexpected error getting runs: %s\n", err.Error())
81+
} else if err == nil && testCase.willFail {
82+
t.Error("expected an error but received none")
83+
}
84+
85+
// Looking back a year, we should have more than 100 runs given there is a one run per day
86+
// This test is only to make sure we get more than the pageSize of results because currently
87+
// the external client will fetch the first pageSize of results but there may be actually more.
88+
// Our code ensures we get all the pages, not just the first page.
89+
if err == nil {
90+
// Special handling for cases where no results are expected.
91+
// In such cases, a successful call with 0 results is acceptable.
92+
if testCase.expectedMinimumResultSize == specialNoResultsExpectedValue && len(runs) == 0 {
93+
// No stable results expected and none received, test passes for this condition.
94+
return
95+
} else if len(runs) < testCase.expectedMinimumResultSize {
96+
t.Errorf("unexpected number of runs for %s. Expected at least %d runs, but got %d.",
97+
browser, testCase.expectedMinimumResultSize, len(runs))
98+
}
99+
}
36100
}
37101
}

workflows/steps/services/chromium_histogram_enums/workflow/job_processor_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,9 @@ var (
3333
)
3434

3535
func TestProcess(t *testing.T) {
36-
sampleHistograms := []metricdatatypes.HistogramName{"TestHistogram1", "TestHistogram2"}
36+
sampleHistograms := []metricdatatypes.HistogramName{metricdatatypes.WebDXFeatureEnum, "TestHistogram2"}
3737
sampleMapping := metricdatatypes.HistogramMapping{
38-
"TestHistogram1": {
38+
metricdatatypes.WebDXFeatureEnum: {
3939
{
4040
Label: "EnumValue1",
4141
Value: 1,

workflows/steps/services/wpt_consumer/cmd/job/main.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,6 @@ func main() {
113113
string(wptconsumertypes.Safari),
114114
string(wptconsumertypes.ChromeAndroid),
115115
string(wptconsumertypes.FirefoxAndroid),
116-
string(wptconsumertypes.SafariIos),
117116
}
118117
channels := []string{shared.StableLabel, shared.ExperimentalLabel}
119118
for _, browser := range browsers {

0 commit comments

Comments
 (0)