From 0cfc7087f0d7356b9f947a3204cf2c31671b85b3 Mon Sep 17 00:00:00 2001 From: jichangjun Date: Thu, 1 Feb 2024 19:19:19 +0800 Subject: [PATCH 1/2] test: add ut for rebase check func --- config/config.go | 29 ++-- config/config.yaml | 11 +- config/defaults.go | 63 +++++++++ .../git-flow/commit-check/commit_check.go | 2 +- .../commit-check/commit_check_test.go | 131 +++++++++++++++--- .../linters/go/staticcheck/staticcheck.go | 2 +- main.go | 2 +- server.go | 33 +++-- 8 files changed, 227 insertions(+), 46 deletions(-) create mode 100644 config/defaults.go diff --git a/config/config.go b/config/config.go index 50e99ba2..5c4f7f81 100644 --- a/config/config.go +++ b/config/config.go @@ -7,14 +7,22 @@ import ( ) // Config maps org or repo to LinterConfig -type Config map[string]map[string]Linter +//type Config map[string]map[string]Linter + +type Config map[string]LinterList + +type LinterList struct { + StaticCheck Linter `json:"staticcheck" yaml:"staticcheck" default:"{\"enable\":true}"` + GoVet Linter `json:"govet" yaml:"govet" default:"{\"enable\":true}"` + LuaCheck Linter `json:"luacheck" yaml:"luacheck" default:"{\"enable\":true}"` +} type Linter struct { // Enable is whether to enable this linter, if false, linter still run but not report. - Enable bool `json:"enable,omitempty"` - WorkDir string `json:"workDir,omitempty"` - Command string `json:"command,omitempty"` - Args []string `json:"args,omitempty"` + Enable bool `json:"enable" yaml:"enable"` + WorkDir string `json:"workDir" yaml:"workDir"` + Command string `json:"command" yaml:"command"` + Args []string `json:"args" yaml:"args"` } // NewConfig returns a new Config. @@ -29,12 +37,17 @@ func NewConfig(conf string) (Config, error) { return nil, err } - // TODO: 应该默认开启staticcheck？ + for k, v := range c { + if err = SetDefaults(&v); err != nil { + return nil, err + } + c[k] = v + } return c, nil } -func (c Config) CustomLinterConfigs(org, repo string) map[string]Linter { +func (c Config) CustomLinterConfigs(org, repo string) LinterList { if repoConfig, ok := c[org+"/"+repo]; ok { return repoConfig } @@ -43,5 +56,5 @@ func (c Config) CustomLinterConfigs(org, repo string) map[string]Linter { return orgConfig } - return nil + return LinterList{} } diff --git a/config/config.yaml b/config/config.yaml index 6fa6f4c9..9614cab8 100644 --- a/config/config.yaml +++ b/config/config.yaml @@ -1,10 +1,17 @@ # This is the default config file, you can override it by creating a config.yaml in the same directory # by default, all linters are enabled if you don't specify. You can disable them by setting enable to false # example1: disable staticcheck for org -# qbox: +#qbox: # staticcheck: # enable: false -# +# govet: +# enable: true +# luacheck: +# enable: true +# workDir: "nginx/Lua" +# #command: luacheck + +# # example2: disable staticcheck for repo # qbox/kodo: # staticcheck: diff --git a/config/defaults.go b/config/defaults.go new file mode 100644 index 00000000..7c155e43 --- /dev/null +++ b/config/defaults.go @@ -0,0 +1,63 @@ +package config + +import ( + "fmt" + "reflect" + + "github.com/creasty/defaults" +) + +// SetDefaults set default values +func SetDefaults(ptr interface{}) error { + err := defaults.Set(ptr) + if err != nil { + return fmt.Errorf("%v: %s", ptr, err.Error()) + } + + v := reflect.ValueOf(ptr).Elem() + t := v.Type() + + for i := 0; i < t.NumField(); i++ { + tf := t.Field(i) + vf := v.Field(i) + if tf.Type.Kind() == reflect.Slice { + for j := 0; j < vf.Len(); j++ { + item := vf.Index(j) + if item.Kind() != reflect.Struct { + continue + } + err := setDefaults(item) + if err != nil { + return err + } + } + } + if tf.Type.Kind() == reflect.Map { + for _, k := range vf.MapKeys() { + item := vf.MapIndex(k) + if item.Kind() != reflect.Struct { + continue + } + tmp := reflect.New(item.Type()) + tmp.Elem().Set(item) + err := setDefaults(tmp.Elem()) + vf.SetMapIndex(k, tmp.Elem()) + if err != nil { + return err + } + } + } + } + return nil +} + +func setDefaults(v reflect.Value) error { + tmp := reflect.New(v.Type()) + tmp.Elem().Set(v) + err := SetDefaults(tmp.Interface()) + if err != nil { + return err + } + v.Set(tmp.Elem()) + return nil +} diff --git a/internal/linters/git-flow/commit-check/commit_check.go b/internal/linters/git-flow/commit-check/commit_check.go index f10a77e6..cfc79eed 100644 --- a/internal/linters/git-flow/commit-check/commit_check.go +++ b/internal/linters/git-flow/commit-check/commit_check.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "fmt" + "github.com/reviewbot/config" "regexp" "strings" "text/template" @@ -27,7 +28,6 @@ import ( "github.com/google/go-github/v57/github" "github.com/qiniu/x/log" "github.com/qiniu/x/xlog" - "github.com/reviewbot/config" "github.com/reviewbot/internal/linters" ) diff --git a/internal/linters/git-flow/commit-check/commit_check_test.go b/internal/linters/git-flow/commit-check/commit_check_test.go index 2339c03c..5761d59e 100644 --- a/internal/linters/git-flow/commit-check/commit_check_test.go +++ b/internal/linters/git-flow/commit-check/commit_check_test.go @@ -1,34 +1,119 @@ package commit_check import ( - "bytes" + "strings" "testing" - "text/template" + + "github.com/google/go-github/v57/github" ) -func TestRebaseSuggestionTmpl(t *testing.T) { - tmpl, err := template.New("rebase_suggestion").Parse(rebaseSuggestionTmpl) - if err != nil { - t.Fatal(err) - } +func TestRebaseCheckRule(t *testing.T) { - var buf bytes.Buffer - var rebaseSuggestion = RebaseSuggestion{ - Author: "author", - Flag: rebaseSuggestionFlag, - TargetCommits: []string{"commit1", "commit2"}, - } - err = tmpl.Execute(&buf, rebaseSuggestion) - if err != nil { - t.Fatal(err) + tcs := []struct { + title string + commits []*github.RepositoryCommit + expected string + }{ + { + title: "filter merge commits", + commits: []*github.RepositoryCommit{ + { + Commit: &github.Commit{ + Message: github.String("feat: add feature 1"), + }, + }, + { + Commit: &github.Commit{ + Message: github.String("Merge a into b"), + }, + }, + { + Commit: &github.Commit{ + Message: github.String("fix: fix bug 2"), + }, + }, + { + Commit: &github.Commit{ + Message: github.String("Merge xxx into xxx"), + }, + }, + }, + expected: "git merge", + }, + { + title: "filter duplicate commits", + commits: []*github.RepositoryCommit{ + { + Commit: &github.Commit{ + Message: github.String("feat: add feature 1"), + }, + }, + { + Commit: &github.Commit{ + Message: github.String("feat: add feature 1"), + }, + }, + { + Commit: &github.Commit{ + Message: github.String("fix: fix bug 2"), + }, + }, + }, + expected: "duplicated", + }, + { + title: "filter duplicate and merge commits", + commits: []*github.RepositoryCommit{ + { + Commit: &github.Commit{ + Message: github.String("feat: add feature 1"), + }, + }, + { + Commit: &github.Commit{ + Message: github.String("feat: add feature 1"), + }, + }, + { + Commit: &github.Commit{ + Message: github.String("Merge xxx into xxx"), + }, + }, + }, + expected: "feat: add feature 1", + }, + { + title: "filter duplicate and merge commits", + commits: []*github.RepositoryCommit{ + { + Commit: &github.Commit{ + Message: github.String("feat: add feature 1"), + }, + }, + { + Commit: &github.Commit{ + Message: github.String("feat: add feature 2"), + }, + }, + }, + expected: "", + }, } - t.Log(buf.String()) -} + for _, tc := range tcs { + t.Run(tc.title, func(t *testing.T) { + comments, err := rebaseCheck(nil, tc.commits) + if err != nil { + t.Fatal(err) + } -func TestRebaseCheckRule(t *testing.T) { - var rule = RebaseCheckRule{ - RebaseSuggestion: RebaseSuggestion - } + if tc.expected == "" && comments != "" { + t.Fatalf("expected %s, got %s", tc.expected, comments) + } + + if !strings.Contains(comments, tc.expected) { + t.Fatalf("expected %s, got %s", tc.expected, comments) + } + }) } -} \ No newline at end of file +} diff --git a/internal/linters/go/staticcheck/staticcheck.go b/internal/linters/go/staticcheck/staticcheck.go index e6c5bc0f..286e968d 100644 --- a/internal/linters/go/staticcheck/staticcheck.go +++ b/internal/linters/go/staticcheck/staticcheck.go @@ -18,6 +18,7 @@ package staticcheck import ( "fmt" + "github.com/reviewbot/config" "os/exec" "path/filepath" "regexp" @@ -27,7 +28,6 @@ import ( "github.com/google/go-github/v57/github" "github.com/qiniu/x/log" "github.com/qiniu/x/xlog" - "github.com/reviewbot/config" "github.com/reviewbot/internal/linters" ) diff --git a/main.go b/main.go index 84a6388b..062c579a 100644 --- a/main.go +++ b/main.go @@ -20,12 +20,12 @@ import ( "errors" "flag" "fmt" + "github.com/reviewbot/config" "net/http" "os" "github.com/google/go-github/v57/github" "github.com/qiniu/x/log" - "github.com/reviewbot/config" "github.com/sirupsen/logrus" gitv2 "k8s.io/test-infra/prow/git/v2" diff --git a/server.go b/server.go index 1ccd5d5a..e190884e 100644 --- a/server.go +++ b/server.go @@ -19,9 +19,6 @@ package main import ( "context" "fmt" - "net/http" - "path/filepath" - "github.com/bradleyfalzon/ghinstallation/v2" "github.com/google/go-github/v57/github" "github.com/gregjones/httpcache" @@ -30,6 +27,8 @@ import ( "github.com/reviewbot/config" "github.com/reviewbot/internal/linters" gitv2 "k8s.io/test-infra/prow/git/v2" + "net/http" + "path/filepath" ) type Server struct { @@ -119,14 +118,19 @@ func (s *Server) handle(log *xlog.Logger, ctx context.Context, event *github.Pul defer r.Clean() customLinterConfigs := s.config.CustomLinterConfigs(org, repo) - log.Infof("found %d custom linter configs for %s\n", len(customLinterConfigs), org+"/"+repo) - for name, fn := range linters.TotalCodeReviewHandlers() { var lingerConfig config.Linter - if v, ok := customLinterConfigs[name]; ok { - lingerConfig = v + //if v, ok := customLinterConfigs[name]; ok { + // lingerConfig = v + //} + switch name { + case "staticcheck": + lingerConfig = customLinterConfigs.StaticCheck + case "govet": + lingerConfig = customLinterConfigs.GoVet + case "luacheck": + lingerConfig = customLinterConfigs.LuaCheck } - if lingerConfig.WorkDir != "" { // 更新完整的工作目录 lingerConfig.WorkDir = filepath.Join(r.Directory(), lingerConfig.WorkDir) @@ -161,8 +165,17 @@ func (s *Server) handle(log *xlog.Logger, ctx context.Context, event *github.Pul for name, fn := range linters.TotalCommentHandlers() { var lingerConfig config.Linter - if v, ok := customLinterConfigs[name]; ok { - lingerConfig = v + //if v, ok := customLinterConfigs[name]; ok { + // lingerConfig = v + //} + + switch name { + case "staticcheck": + lingerConfig = customLinterConfigs.StaticCheck + case "govet": + lingerConfig = customLinterConfigs.GoVet + case "luacheck": + lingerConfig = customLinterConfigs.LuaCheck } if lingerConfig.WorkDir != "" { From 928eaf5271dfae69aa8f09ebb444b538266ae976 Mon Sep 17 00:00:00 2001 From: xwen-winnie <657844267@qq.com> Date: Sun, 4 Feb 2024 09:12:41 +0800 Subject: [PATCH 2/2] fix --- config/config.go | 40 ++++++++++++++------------ config/config.yaml | 6 +++- config/config_test.go | 67 ++++++++++++++++++++++++++++++++++++++----- config/defaults.go | 63 ---------------------------------------- 4 files changed, 87 insertions(+), 89 deletions(-) delete mode 100644 config/defaults.go diff --git a/config/config.go b/config/config.go index 5c4f7f81..bce71803 100644 --- a/config/config.go +++ b/config/config.go @@ -9,45 +9,49 @@ import ( // Config maps org or repo to LinterConfig //type Config map[string]map[string]Linter -type Config map[string]LinterList - -type LinterList struct { - StaticCheck Linter `json:"staticcheck" yaml:"staticcheck" default:"{\"enable\":true}"` - GoVet Linter `json:"govet" yaml:"govet" default:"{\"enable\":true}"` - LuaCheck Linter `json:"luacheck" yaml:"luacheck" default:"{\"enable\":true}"` -} +type Config map[string]map[string]Linter type Linter struct { // Enable is whether to enable this linter, if false, linter still run but not report. - Enable bool `json:"enable" yaml:"enable"` - WorkDir string `json:"workDir" yaml:"workDir"` - Command string `json:"command" yaml:"command"` - Args []string `json:"args" yaml:"args"` + Enable *bool `json:"enable,omitempty" yaml:"enable,omitempty"` + WorkDir string `json:"workDir,omitempty" yaml:"workDir,omitempty"` + Command string `json:"command,omitempty" yaml:"command,omitempty"` + Args []string `json:"args,omitempty" yaml:"args,omitempty"` } // NewConfig returns a new Config. func NewConfig(conf string) (Config, error) { - var c Config f, err := os.ReadFile(conf) if err != nil { return nil, err } + c := Config{} if err = yaml.Unmarshal(f, &c); err != nil { return nil, err } - for k, v := range c { - if err = SetDefaults(&v); err != nil { - return nil, err + defaultEnable := true + for _, v := range c { + for _, val := range v { + if val.Enable == nil { + val.Enable = &defaultEnable + } + } + } + + if len(c) == 0 { + c["qbox"] = map[string]Linter{ + "staticcheck": {Enable: &defaultEnable}, + "govet": {Enable: &defaultEnable}, + "luacheck": {Enable: &defaultEnable}, } - c[k] = v } return c, nil } -func (c Config) CustomLinterConfigs(org, repo string) LinterList { +func (c Config) CustomLinterConfigs(org, repo string) map[string]Linter { if repoConfig, ok := c[org+"/"+repo]; ok { return repoConfig } @@ -56,5 +60,5 @@ func (c Config) CustomLinterConfigs(org, repo string) LinterList { return orgConfig } - return LinterList{} + return nil } diff --git a/config/config.yaml b/config/config.yaml index 9614cab8..5be8328c 100644 --- a/config/config.yaml +++ b/config/config.yaml @@ -23,4 +23,8 @@ qbox/kodo: # github repository name, which will override the settings in qbox or workDir: "src/qiniu.com/kodo" command: staticcheck args: ["-debug.no-compile-errors=true", "./..."] - run_if_changed: ["src/qiniu.com/kodo/.*\\.go$"] # only run if changed files match the regex \ No newline at end of file + run_if_changed: ["src/qiniu.com/kodo/.*\\.go$"] # only run if changed files match the regex + govet: + enable: false + luacheck: + enable: false \ No newline at end of file diff --git a/config/config_test.go b/config/config_test.go index 92a6bef0..f2621895 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -1,19 +1,72 @@ package config import ( - "fmt" + "os" + "path" "testing" + + "github.com/stretchr/testify/assert" ) func TestConfig(t *testing.T) { - repoConfig, err := NewConfig("config.yaml") - if err != nil { - t.Errorf("NewConfig() error = %v", err) - return + defaultTrue := true + defaultFalse := false + case1 := ` +qbox: + staticcheck: + enable: false + luacheck: + enable: true + workDir: "nginx/Lua" + command: luacheck +` + + exp1 := map[string]map[string]Linter{ + "qbox": { + "staticcheck": {Enable: &defaultFalse}, + "luacheck": {Enable: &defaultTrue, WorkDir: "nginx/Lua", Command: "luacheck"}, + }, + } + + case2 := ` +qbox: + luacheck: + enable: false + workDir: "nginx/Lua" + command: luacheck +` + exp2 := map[string]map[string]Linter{ + "qbox": { + "luacheck": {Enable: &defaultFalse, WorkDir: "nginx/Lua", Command: "luacheck"}, + }, + } + + case3 := `` + exp3 := map[string]map[string]Linter{ + "qbox": { + "staticcheck": {Enable: &defaultTrue}, + "govet": {Enable: &defaultTrue}, + "luacheck": {Enable: &defaultTrue}, + }, } - for k, c := range repoConfig { - fmt.Printf("%v: %v \n", k, c) + cs := map[string]map[string]map[string]Linter{ + case1: exp1, + case2: exp2, + case3: exp3, } + dir := os.TempDir() + defer os.RemoveAll(dir) + + for k, v := range cs { + f := path.Join(dir, "t.yaml") + err := os.WriteFile(f, []byte(k), 666) + assert.NoError(t, err) + + res, err := NewConfig(f) + assert.NoError(t, err) + + assert.EqualValues(t, v, res) + } } diff --git a/config/defaults.go b/config/defaults.go deleted file mode 100644 index 7c155e43..00000000 --- a/config/defaults.go +++ /dev/null @@ -1,63 +0,0 @@ -package config - -import ( - "fmt" - "reflect" - - "github.com/creasty/defaults" -) - -// SetDefaults set default values -func SetDefaults(ptr interface{}) error { - err := defaults.Set(ptr) - if err != nil { - return fmt.Errorf("%v: %s", ptr, err.Error()) - } - - v := reflect.ValueOf(ptr).Elem() - t := v.Type() - - for i := 0; i < t.NumField(); i++ { - tf := t.Field(i) - vf := v.Field(i) - if tf.Type.Kind() == reflect.Slice { - for j := 0; j < vf.Len(); j++ { - item := vf.Index(j) - if item.Kind() != reflect.Struct { - continue - } - err := setDefaults(item) - if err != nil { - return err - } - } - } - if tf.Type.Kind() == reflect.Map { - for _, k := range vf.MapKeys() { - item := vf.MapIndex(k) - if item.Kind() != reflect.Struct { - continue - } - tmp := reflect.New(item.Type()) - tmp.Elem().Set(item) - err := setDefaults(tmp.Elem()) - vf.SetMapIndex(k, tmp.Elem()) - if err != nil { - return err - } - } - } - } - return nil -} - -func setDefaults(v reflect.Value) error { - tmp := reflect.New(v.Type()) - tmp.Elem().Set(v) - err := SetDefaults(tmp.Interface()) - if err != nil { - return err - } - v.Set(tmp.Elem()) - return nil -}