Skip to content

Commit d46a02d

Browse files
spiffxpspiffxp
committed
make test: stop hitting github for unit tests
Instead of fetching sigs.yaml and OWNERS_ALIASES from GitHub all the time, allow unit tests to mock fetching. Replace the global api.FetchFoo(...) function calls with a GroupFetcher interface that can be plumbed through. Provide default and mock implementations. Replace uses of repo.NewRepo(path) with repo.NewRepo(path, fetcher) in tests Move QueryOpts.Validate logic that requires knowledge of group info over to Repo.PrepareQueryOpts. Call that in Repo.Query, but leave it public for ease of testing. Do something similar for pkg/legacy but with not as much thought behind api reuse, since that package is on its way out.
1 parent aefb309 commit d46a02d

File tree

19 files changed

+218
-283
lines changed

19 files changed

+218
-283
lines changed

‎api/approval.go

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -94,19 +94,6 @@ type PRRMilestone struct {
9494

9595
type PRRHandler Parser
9696

97-
func NewPRRHandler() (*PRRHandler, error) {
98-
handler := &PRRHandler{}
99-
100-
approvers, err := FetchPRRApprovers()
101-
if err != nil {
102-
return nil, errors.Wrap(err, "fetching PRR approvers")
103-
}
104-
105-
handler.PRRApprovers = approvers
106-
107-
return handler, nil
108-
}
109-
11097
// TODO(api): Make this a generic parser for all `Document` types
11198
func (p *PRRHandler) Parse(in io.Reader) (*PRRApproval, error) {
11299
scanner := bufio.NewScanner(in)

‎api/groups.go

Lines changed: 60 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -29,19 +29,65 @@ import (
2929
"sigs.k8s.io/yaml"
3030
)
3131

32-
const (
33-
// URLs
34-
sigListURL = "https://raw.githubusercontent.com/kubernetes/community/master/sigs.yaml"
35-
ownersAliasURL = "https://raw.githubusercontent.com/kubernetes/enhancements/master/OWNERS_ALIASES"
32+
// GroupFetcher is responsible for getting informationg about groups in the
33+
// Kubernetes Community
34+
type GroupFetcher interface {
35+
// FetchGroups returns the list of valid Kubernetes Community groups
36+
// e.g. (SIGs, WGs, UGs, Committees)
37+
FetchGroups() ([]string, error)
38+
// FetchPRRApprovers returns the list of valid PRR Approvers
39+
FetchPRRApprovers() ([]string, error)
40+
}
3641

37-
// Aliases
38-
prrApproversAlias = "prod-readiness-approvers"
39-
)
42+
// DefaultGroupFetcher returns the default GroupFetcher, which depends on GitHub
43+
func DefaultGroupFetcher() GroupFetcher {
44+
return &RemoteGroupFetcher{
45+
GroupsListURL: "https://raw.githubusercontent.com/kubernetes/community/master/sigs.yaml",
46+
OwnersAliasesURL: "https://raw.githubusercontent.com/kubernetes/enhancements/master/OWNERS_ALIASES",
47+
PRRApproversAlias: "prod-readiness-approvers",
48+
}
49+
}
50+
51+
// MockGroupFetcher returns the given Groups and PRR Approvers
52+
type MockGroupFetcher struct {
53+
Groups []string
54+
PRRApprovers []string
55+
}
56+
57+
var _ GroupFetcher = &MockGroupFetcher{}
58+
59+
func NewMockGroupFetcher(groups, prrApprovers []string) GroupFetcher {
60+
return &MockGroupFetcher{Groups: groups, PRRApprovers: prrApprovers}
61+
}
62+
63+
func (f *MockGroupFetcher) FetchGroups() ([]string, error) {
64+
result := make([]string, len(f.Groups))
65+
copy(result, f.Groups)
66+
return result, nil
67+
}
68+
69+
func (f *MockGroupFetcher) FetchPRRApprovers() ([]string, error) {
70+
result := make([]string, len(f.PRRApprovers))
71+
copy(result, f.PRRApprovers)
72+
return result, nil
73+
}
74+
75+
// RemoteGroupFetcher returns Groups and PRR Approvers from remote sources
76+
type RemoteGroupFetcher struct {
77+
// Basically kubernetes/community/sigs.yaml
78+
GroupsListURL string
79+
// Basically kubernetes/enhancements/OWNERS_ALIASES
80+
OwnersAliasesURL string
81+
// The alias name to look for in OWNERS_ALIASES
82+
PRRApproversAlias string
83+
}
84+
85+
var _ GroupFetcher = &RemoteGroupFetcher{}
4086

41-
// FetchGroups returns a list of Kubernetes governance groups
42-
// (SIGs, Working Groups, User Groups).
43-
func FetchGroups() ([]string, error) {
44-
resp, err := http.Get(sigListURL)
87+
// FetchGroups returns the list of valid Kubernetes Community groups as
88+
// fetched from a remote source
89+
func (f *RemoteGroupFetcher) FetchGroups() ([]string, error) {
90+
resp, err := http.Get(f.GroupsListURL)
4591
if err != nil {
4692
return nil, errors.Wrap(err, "fetching SIG list")
4793
}
@@ -78,8 +124,8 @@ func FetchGroups() ([]string, error) {
78124
}
79125

80126
// FetchPRRApprovers returns a list of PRR approvers.
81-
func FetchPRRApprovers() ([]string, error) {
82-
resp, err := http.Get(ownersAliasURL)
127+
func (f *RemoteGroupFetcher) FetchPRRApprovers() ([]string, error) {
128+
resp, err := http.Get(f.OwnersAliasesURL)
83129
if err != nil {
84130
return nil, errors.Wrap(err, "fetching owners aliases")
85131
}
@@ -108,7 +154,7 @@ func FetchPRRApprovers() ([]string, error) {
108154
}
109155

110156
var result []string
111-
result = append(result, config.Data[prrApproversAlias]...)
157+
result = append(result, config.Data[f.PRRApproversAlias]...)
112158

113159
if len(result) == 0 {
114160
return nil, errors.New("retrieved zero PRR approvers, which is unexpected")

‎api/proposal.go

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -86,26 +86,6 @@ func (p *Proposal) IsMissingStage() bool {
8686

8787
type KEPHandler Parser
8888

89-
func NewKEPHandler() (*KEPHandler, error) {
90-
handler := &KEPHandler{}
91-
92-
groups, err := FetchGroups()
93-
if err != nil {
94-
return nil, errors.Wrap(err, "fetching groups")
95-
}
96-
97-
handler.Groups = groups
98-
99-
approvers, err := FetchPRRApprovers()
100-
if err != nil {
101-
return nil, errors.Wrap(err, "fetching PRR approvers")
102-
}
103-
104-
handler.PRRApprovers = approvers
105-
106-
return handler, nil
107-
}
108-
10989
// TODO(api): Make this a generic parser for all `Document` types
11090
func (k *KEPHandler) Parse(in io.Reader) (*Proposal, error) {
11191
scanner := bufio.NewScanner(in)

‎cmd/kepify/main.go

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@ import (
2525
"path/filepath"
2626
"strings"
2727

28-
"github.com/pkg/errors"
29-
3028
"k8s.io/enhancements/api"
3129
)
3230

@@ -72,8 +70,24 @@ func main() {
7270
os.Exit(1)
7371
}
7472

73+
fetcher := api.DefaultGroupFetcher()
74+
75+
groups, err := fetcher.FetchGroups()
76+
if err != nil {
77+
fmt.Fprintf(os.Stderr, "unable to fetch groups: %v", err)
78+
os.Exit(1)
79+
}
80+
81+
prrApprovers, err := fetcher.FetchPRRApprovers()
82+
if err != nil {
83+
fmt.Fprintf(os.Stderr, "unable to fetch PRR approvers: %v", err)
84+
os.Exit(1)
85+
}
86+
87+
kepHandler := &api.KEPHandler{Groups: groups, PRRApprovers: prrApprovers}
88+
7589
// Parse the files
76-
proposals, err := parseFiles(files)
90+
proposals, err := parseFiles(kepHandler, files)
7791
if err != nil {
7892
fmt.Fprintf(os.Stderr, "error parsing files: %q\n", err)
7993
os.Exit(1)
@@ -112,14 +126,9 @@ func findMarkdownFiles(dirPath *string) ([]string, error) {
112126
return files, err
113127
}
114128

115-
func parseFiles(files []string) (api.Proposals, error) {
129+
func parseFiles(parser *api.KEPHandler, files []string) (api.Proposals, error) {
116130
var proposals api.Proposals
117131
for _, filename := range files {
118-
parser, err := api.NewKEPHandler()
119-
if err != nil {
120-
return nil, errors.Wrap(err, "creating new KEP handler")
121-
}
122-
123132
file, err := os.Open(filename)
124133
if err != nil {
125134
return nil, fmt.Errorf("could not open file: %v", err)

‎pkg/legacy/keps/proposals.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,10 @@ import (
3131
"k8s.io/enhancements/pkg/legacy/keps/validations"
3232
)
3333

34-
type Parser struct{}
34+
type Parser struct {
35+
Groups []string
36+
PRRApprovers []string
37+
}
3538

3639
func (p *Parser) Parse(in io.Reader) *api.Proposal {
3740
scanner := bufio.NewScanner(in)
@@ -71,7 +74,7 @@ func (p *Parser) Parse(in io.Reader) *api.Proposal {
7174
return proposal
7275
}
7376

74-
if err := validations.ValidateStructure(test); err != nil {
77+
if err := validations.ValidateStructure(p.Groups, p.PRRApprovers, test); err != nil {
7578
proposal.Error = errors.Wrap(err, "error validating KEP metadata")
7679
return proposal
7780
}

‎pkg/legacy/keps/proposals_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,10 @@ metrics:
7575
}
7676
for _, tc := range testcases {
7777
t.Run(tc.name, func(t *testing.T) {
78-
p := &keps.Parser{}
78+
p := &keps.Parser{
79+
Groups: []string{"sig-api-machinery", "sig-architecture"},
80+
PRRApprovers: []string{"wojtek-t"},
81+
}
7982
contents := strings.NewReader(tc.fileContents)
8083
out := p.Parse(contents)
8184
if out.Error != nil {

‎pkg/legacy/keps/validations/yaml.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,16 +37,13 @@ var (
3737

3838
// TODO(lint): cyclomatic complexity 50 of func `ValidateStructure` is high (> 30) (gocyclo)
3939
//nolint:gocyclo
40-
func ValidateStructure(parsed map[interface{}]interface{}) error {
40+
func ValidateStructure(listGroups []string, prrApprovers []string, parsed map[interface{}]interface{}) error {
4141
for _, key := range mandatoryKeys {
4242
if _, found := parsed[key]; !found {
4343
return util.NewKeyMustBeSpecified(key)
4444
}
4545
}
4646

47-
listGroups := util.Groups()
48-
prrApprovers := util.PRRApprovers()
49-
5047
for key, value := range parsed {
5148
// First off the key has to be a string. fact.
5249
k, ok := key.(string)

‎pkg/legacy/keps/validations/yaml_test.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,11 @@ func TestUnmarshalSuccess(t *testing.T) {
115115
if err := yaml.Unmarshal(yamlDoc.YAMLDoc(), p); err != nil {
116116
t.Fatal(err)
117117
}
118-
if err := ValidateStructure(p); err != nil {
118+
119+
groups := []string{"sig-architecture"}
120+
prrApprovers := []string{}
121+
122+
if err := ValidateStructure(groups, prrApprovers, p); err != nil {
119123
t.Fatal(err)
120124
}
121125
}
@@ -162,13 +166,15 @@ func TestValidateStructureSuccess(t *testing.T) {
162166
},
163167
},
164168
}
169+
groups := []string{"sig-architecture"}
170+
prrApprovers := []string{}
165171
for _, tc := range testcases {
166172
t.Run(tc.name, func(t *testing.T) {
167173
// TODO: Add required fields
168174
tc.input["title"] = "this is a title"
169175
tc.input["owning-sig"] = "sig-architecture"
170176

171-
err := ValidateStructure(tc.input)
177+
err := ValidateStructure(groups, prrApprovers, tc.input)
172178
if err != nil {
173179
t.Fatalf("did not expect an error: %v", err)
174180
}
@@ -218,9 +224,11 @@ func TestValidateStructureFailures(t *testing.T) {
218224
},
219225
},
220226
}
227+
groups := []string{}
228+
prrApprovers := []string{}
221229
for _, tc := range testcases {
222230
t.Run(tc.name, func(t *testing.T) {
223-
err := ValidateStructure(tc.input)
231+
err := ValidateStructure(groups, prrApprovers, tc.input)
224232
if err == nil {
225233
t.Fatal("expecting an error")
226234
}

‎pkg/legacy/prrs/approvals.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,9 @@ import (
2828
"k8s.io/enhancements/pkg/legacy/prrs/validations"
2929
)
3030

31-
type Parser struct{}
31+
type Parser struct {
32+
PRRApprovers []string
33+
}
3234

3335
func (p *Parser) Parse(in io.Reader) *api.PRRApproval {
3436
scanner := bufio.NewScanner(in)
@@ -50,7 +52,7 @@ func (p *Parser) Parse(in io.Reader) *api.PRRApproval {
5052
approval.Error = errors.Wrap(err, "error unmarshalling YAML")
5153
return approval
5254
}
53-
if err := validations.ValidateStructure(test); err != nil {
55+
if err := validations.ValidateStructure(p.PRRApprovers, test); err != nil {
5456
approval.Error = errors.Wrap(err, "error validating PRR approval metadata")
5557
return approval
5658
}

‎pkg/legacy/prrs/approvals_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@ stable:
3939
}
4040
for _, tc := range testcases {
4141
t.Run(tc.name, func(t *testing.T) {
42-
p := &Parser{}
42+
p := &Parser{
43+
PRRApprovers: []string{"wojtek-t", "johnbelamaric"},
44+
}
4345
contents := strings.NewReader(tc.fileContents)
4446
out := p.Parse(contents)
4547
if out.Error != nil {

0 commit comments

Comments
 (0)