From 834300183e85eb9cd5a749ed042294b63b8cd275 Mon Sep 17 00:00:00 2001 From: Gregory Albouy Date: Sat, 8 Oct 2022 10:58:26 +0200 Subject: [PATCH 01/11] refactor: implement Config.WithFields Integrate the fields memorizing logics into runner.Config - implement Config.WithFields - update tests accordingly - configparse: remove parsedConfig intermediary --- internal/configparse/json.go | 4 +- internal/configparse/json_test.go | 4 +- internal/configparse/parse.go | 55 +++++++++++---------------- runner/internal/config/config.go | 28 +++++++++++++- runner/internal/config/config_test.go | 4 +- 5 files changed, 54 insertions(+), 41 deletions(-) diff --git a/internal/configparse/json.go b/internal/configparse/json.go index 736c520..7ccab59 100644 --- a/internal/configparse/json.go +++ b/internal/configparse/json.go @@ -13,10 +13,10 @@ func JSON(in []byte) (runner.Config, error) { return runner.Config{}, err } - pconf, err := newParsedConfig(uconf) + cfg, err := newParsedConfig(uconf) if err != nil { return runner.Config{}, err } - return runner.DefaultConfig().Override(pconf.config, pconf.fields...), nil + return runner.DefaultConfig().Override(cfg), nil } diff --git a/internal/configparse/json_test.go b/internal/configparse/json_test.go index 54cec95..0947292 100644 --- a/internal/configparse/json_test.go +++ b/internal/configparse/json_test.go @@ -55,9 +55,7 @@ func TestJSON(t *testing.T) { Runner: runner.RecorderConfig{ Concurrency: 3, }, - }, - "url", - "concurrency", + }.WithFields("url", "concurrency"), ), expError: nil, }, diff --git a/internal/configparse/parse.go b/internal/configparse/parse.go index 3c56655..8437dae 100644 --- a/internal/configparse/parse.go +++ b/internal/configparse/parse.go @@ -48,32 +48,23 @@ type UnmarshaledConfig struct { } `yaml:"tests" json:"tests"` } -// parsedConfig embeds a parsed runner.ConfigGlobal and the list of its set fields. -type parsedConfig struct { - // TODO: do not embed, use field config - config runner.Config - fields []string -} +// newParsedConfig parses an input raw config as a runner.ConfigGlobal and returns +// a parsed Config or the first non-nil error occurring in the process. +func newParsedConfig(uconf UnmarshaledConfig) (runner.Config, error) { //nolint:gocognit // acceptable complexity for a parsing func + cfg := runner.Config{} + fieldsSet := []string{} -// addField adds a field to the list of set fields. -func (pconf *parsedConfig) add(field string) { - pconf.fields = append(pconf.fields, field) -} + addField := func(field string) { + fieldsSet = append(fieldsSet, field) + } -// newParsedConfig parses an input raw config as a runner.ConfigGlobal and returns -// a parsedConfig or the first non-nil error occurring in the process. -func newParsedConfig(uconf UnmarshaledConfig) (parsedConfig, error) { //nolint:gocognit // acceptable complexity for a parsing func - maxFields := len(runner.ConfigFieldsUsage) - pconf := parsedConfig{fields: make([]string, 0, maxFields)} - cfg := &pconf.config - - abort := func(err error) (parsedConfig, error) { - return parsedConfig{}, err + abort := func(err error) (runner.Config, error) { + return runner.Config{}, err } if method := uconf.Request.Method; method != nil { cfg.Request.Method = *method - pconf.add(runner.ConfigFieldMethod) + addField(runner.ConfigFieldMethod) } if rawURL := uconf.Request.URL; rawURL != nil { @@ -82,7 +73,7 @@ func newParsedConfig(uconf UnmarshaledConfig) (parsedConfig, error) { //nolint:g return abort(err) } cfg.Request.URL = parsedURL - pconf.add(runner.ConfigFieldURL) + addField(runner.ConfigFieldURL) } if header := uconf.Request.Header; header != nil { @@ -91,7 +82,7 @@ func newParsedConfig(uconf UnmarshaledConfig) (parsedConfig, error) { //nolint:g httpHeader[key] = val } cfg.Request.Header = httpHeader - pconf.add(runner.ConfigFieldHeader) + addField(runner.ConfigFieldHeader) } if body := uconf.Request.Body; body != nil { @@ -99,17 +90,17 @@ func newParsedConfig(uconf UnmarshaledConfig) (parsedConfig, error) { //nolint:g Type: body.Type, Content: []byte(body.Content), } - pconf.add(runner.ConfigFieldBody) + addField(runner.ConfigFieldBody) } if requests := uconf.Runner.Requests; requests != nil { cfg.Runner.Requests = *requests - pconf.add(runner.ConfigFieldRequests) + addField(runner.ConfigFieldRequests) } if concurrency := uconf.Runner.Concurrency; concurrency != nil { cfg.Runner.Concurrency = *concurrency - pconf.add(runner.ConfigFieldConcurrency) + addField(runner.ConfigFieldConcurrency) } if interval := uconf.Runner.Interval; interval != nil { @@ -118,7 +109,7 @@ func newParsedConfig(uconf UnmarshaledConfig) (parsedConfig, error) { //nolint:g return abort(err) } cfg.Runner.Interval = parsedInterval - pconf.add(runner.ConfigFieldInterval) + addField(runner.ConfigFieldInterval) } if requestTimeout := uconf.Runner.RequestTimeout; requestTimeout != nil { @@ -127,7 +118,7 @@ func newParsedConfig(uconf UnmarshaledConfig) (parsedConfig, error) { //nolint:g return abort(err) } cfg.Runner.RequestTimeout = parsedTimeout - pconf.add(runner.ConfigFieldRequestTimeout) + addField(runner.ConfigFieldRequestTimeout) } if globalTimeout := uconf.Runner.GlobalTimeout; globalTimeout != nil { @@ -136,17 +127,17 @@ func newParsedConfig(uconf UnmarshaledConfig) (parsedConfig, error) { //nolint:g return abort(err) } cfg.Runner.GlobalTimeout = parsedGlobalTimeout - pconf.add(runner.ConfigFieldGlobalTimeout) + addField(runner.ConfigFieldGlobalTimeout) } if silent := uconf.Output.Silent; silent != nil { cfg.Output.Silent = *silent - pconf.add(runner.ConfigFieldSilent) + addField(runner.ConfigFieldSilent) } testSuite := uconf.Tests if len(testSuite) == 0 { - return pconf, nil + return cfg.WithFields(fieldsSet...), nil } cases := make([]runner.TestCase, len(testSuite)) @@ -187,9 +178,9 @@ func newParsedConfig(uconf UnmarshaledConfig) (parsedConfig, error) { //nolint:g } } cfg.Tests = cases - pconf.add(runner.ConfigFieldTests) + addField(runner.ConfigFieldTests) - return pconf, nil + return cfg.WithFields(fieldsSet...), nil } // helpers diff --git a/runner/internal/config/config.go b/runner/internal/config/config.go index debcdd0..9494b78 100644 --- a/runner/internal/config/config.go +++ b/runner/internal/config/config.go @@ -83,6 +83,14 @@ type Output struct { Silent bool } +type set map[string]struct{} + +func (set set) add(values ...string) { + for _, v := range values { + set[v] = struct{}{} + } +} + // Global represents the global configuration of the runner. // It must be validated using Global.Validate before usage. type Global struct { @@ -90,6 +98,19 @@ type Global struct { Runner Runner Output Output Tests []tests.Case + + fieldsSet set +} + +// WithField returns a new Global with the input fields marked as set. +func (cfg Global) WithFields(fields ...string) Global { + fieldsSet := cfg.fieldsSet + if fieldsSet == nil { + fieldsSet = set{} + } + fieldsSet.add(fields...) + cfg.fieldsSet = fieldsSet + return cfg } // String returns an indented JSON representation of Config @@ -102,8 +123,11 @@ func (cfg Global) String() string { // Override returns a new Config based on cfg with overridden values from c. // Only fields specified in options are replaced. Accepted options are limited // to existing Fields, other values are silently ignored. -func (cfg Global) Override(c Global, fields ...string) Global { - for _, field := range fields { +func (cfg Global) Override(c Global) Global { + if len(c.fieldsSet) == 0 { + return cfg + } + for field := range c.fieldsSet { switch field { case FieldMethod: cfg.Request.Method = c.Request.Method diff --git a/runner/internal/config/config_test.go b/runner/internal/config/config_test.go index b4ce0ef..56e62ac 100644 --- a/runner/internal/config/config_test.go +++ b/runner/internal/config/config_test.go @@ -120,7 +120,7 @@ func TestGlobal_Override(t *testing.T) { config.FieldSilent, } - if gotCfg := baseCfg.Override(newCfg, fields...); !reflect.DeepEqual(gotCfg, newCfg) { + if gotCfg := baseCfg.Override(newCfg.WithFields(fields...)); !reflect.DeepEqual(gotCfg, newCfg) { t.Errorf("did not override expected fields:\nexp %v\ngot %v", baseCfg, gotCfg) t.Log(fields) } @@ -204,7 +204,7 @@ func TestGlobal_Override(t *testing.T) { }, } - gotCfg := oldCfg.Override(newCfg, config.FieldHeader) + gotCfg := oldCfg.Override(newCfg.WithFields(config.FieldHeader)) if gotHeader := gotCfg.Request.Header; !reflect.DeepEqual(gotHeader, tc.expHeader) { t.Errorf("\nexp %#v\ngot %#v", tc.expHeader, gotHeader) From cd823ba2367f6ce66ee0409975fb72963951c4ab Mon Sep 17 00:00:00 2001 From: Gregory Albouy Date: Sat, 8 Oct 2022 11:04:28 +0200 Subject: [PATCH 02/11] refactor(configparse) rename UnmarshaledConfig -> Representation --- internal/configparse/json.go | 6 ++-- internal/configparse/parse.go | 30 ++++++++++---------- internal/configparse/parser.go | 4 +-- internal/configparse/parser_internal_test.go | 4 +-- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/internal/configparse/json.go b/internal/configparse/json.go index 7ccab59..83f5bf3 100644 --- a/internal/configparse/json.go +++ b/internal/configparse/json.go @@ -8,12 +8,12 @@ import ( func JSON(in []byte) (runner.Config, error) { parser := jsonParser{} - var uconf UnmarshaledConfig - if err := parser.parse(in, &uconf); err != nil { + var repr Representation + if err := parser.parse(in, &repr); err != nil { return runner.Config{}, err } - cfg, err := newParsedConfig(uconf) + cfg, err := newParsedConfig(repr) if err != nil { return runner.Config{}, err } diff --git a/internal/configparse/parse.go b/internal/configparse/parse.go index 8437dae..f25780f 100644 --- a/internal/configparse/parse.go +++ b/internal/configparse/parse.go @@ -10,11 +10,11 @@ import ( "github.com/benchttp/engine/runner" ) -// UnmarshaledConfig is a raw data model for runner config files. +// Representation is a raw data model for runner config files. // It serves as a receiver for unmarshaling processes and for that reason // its types are kept simple (certain types are incompatible with certain // unmarshalers). -type UnmarshaledConfig struct { +type Representation struct { Extends *string `yaml:"extends" json:"extends"` Request struct { @@ -50,7 +50,7 @@ type UnmarshaledConfig struct { // newParsedConfig parses an input raw config as a runner.ConfigGlobal and returns // a parsed Config or the first non-nil error occurring in the process. -func newParsedConfig(uconf UnmarshaledConfig) (runner.Config, error) { //nolint:gocognit // acceptable complexity for a parsing func +func newParsedConfig(repr Representation) (runner.Config, error) { //nolint:gocognit // acceptable complexity for a parsing func cfg := runner.Config{} fieldsSet := []string{} @@ -62,13 +62,13 @@ func newParsedConfig(uconf UnmarshaledConfig) (runner.Config, error) { //nolint: return runner.Config{}, err } - if method := uconf.Request.Method; method != nil { + if method := repr.Request.Method; method != nil { cfg.Request.Method = *method addField(runner.ConfigFieldMethod) } - if rawURL := uconf.Request.URL; rawURL != nil { - parsedURL, err := parseAndBuildURL(*uconf.Request.URL, uconf.Request.QueryParams) + if rawURL := repr.Request.URL; rawURL != nil { + parsedURL, err := parseAndBuildURL(*repr.Request.URL, repr.Request.QueryParams) if err != nil { return abort(err) } @@ -76,7 +76,7 @@ func newParsedConfig(uconf UnmarshaledConfig) (runner.Config, error) { //nolint: addField(runner.ConfigFieldURL) } - if header := uconf.Request.Header; header != nil { + if header := repr.Request.Header; header != nil { httpHeader := http.Header{} for key, val := range header { httpHeader[key] = val @@ -85,7 +85,7 @@ func newParsedConfig(uconf UnmarshaledConfig) (runner.Config, error) { //nolint: addField(runner.ConfigFieldHeader) } - if body := uconf.Request.Body; body != nil { + if body := repr.Request.Body; body != nil { cfg.Request.Body = runner.RequestBody{ Type: body.Type, Content: []byte(body.Content), @@ -93,17 +93,17 @@ func newParsedConfig(uconf UnmarshaledConfig) (runner.Config, error) { //nolint: addField(runner.ConfigFieldBody) } - if requests := uconf.Runner.Requests; requests != nil { + if requests := repr.Runner.Requests; requests != nil { cfg.Runner.Requests = *requests addField(runner.ConfigFieldRequests) } - if concurrency := uconf.Runner.Concurrency; concurrency != nil { + if concurrency := repr.Runner.Concurrency; concurrency != nil { cfg.Runner.Concurrency = *concurrency addField(runner.ConfigFieldConcurrency) } - if interval := uconf.Runner.Interval; interval != nil { + if interval := repr.Runner.Interval; interval != nil { parsedInterval, err := parseOptionalDuration(*interval) if err != nil { return abort(err) @@ -112,7 +112,7 @@ func newParsedConfig(uconf UnmarshaledConfig) (runner.Config, error) { //nolint: addField(runner.ConfigFieldInterval) } - if requestTimeout := uconf.Runner.RequestTimeout; requestTimeout != nil { + if requestTimeout := repr.Runner.RequestTimeout; requestTimeout != nil { parsedTimeout, err := parseOptionalDuration(*requestTimeout) if err != nil { return abort(err) @@ -121,7 +121,7 @@ func newParsedConfig(uconf UnmarshaledConfig) (runner.Config, error) { //nolint: addField(runner.ConfigFieldRequestTimeout) } - if globalTimeout := uconf.Runner.GlobalTimeout; globalTimeout != nil { + if globalTimeout := repr.Runner.GlobalTimeout; globalTimeout != nil { parsedGlobalTimeout, err := parseOptionalDuration(*globalTimeout) if err != nil { return abort(err) @@ -130,12 +130,12 @@ func newParsedConfig(uconf UnmarshaledConfig) (runner.Config, error) { //nolint: addField(runner.ConfigFieldGlobalTimeout) } - if silent := uconf.Output.Silent; silent != nil { + if silent := repr.Output.Silent; silent != nil { cfg.Output.Silent = *silent addField(runner.ConfigFieldSilent) } - testSuite := uconf.Tests + testSuite := repr.Tests if len(testSuite) == 0 { return cfg.WithFields(fieldsSet...), nil } diff --git a/internal/configparse/parser.go b/internal/configparse/parser.go index 0bae47e..2e7865b 100644 --- a/internal/configparse/parser.go +++ b/internal/configparse/parser.go @@ -15,7 +15,7 @@ type yamlParser struct{} // parse decodes a raw yaml input in strict mode (unknown fields disallowed) // and stores the resulting value into dst. -func (p yamlParser) parse(in []byte, dst *UnmarshaledConfig) error { +func (p yamlParser) parse(in []byte, dst *Representation) error { decoder := yaml.NewDecoder(bytes.NewReader(in)) decoder.KnownFields(true) return p.handleError(decoder.Decode(dst)) @@ -102,7 +102,7 @@ type jsonParser struct{} // parse decodes a raw JSON input in strict mode (unknown fields disallowed) // and stores the resulting value into dst. -func (p jsonParser) parse(in []byte, dst *UnmarshaledConfig) error { +func (p jsonParser) parse(in []byte, dst *Representation) error { decoder := json.NewDecoder(bytes.NewReader(in)) decoder.DisallowUnknownFields() return p.handleError(decoder.Decode(dst)) diff --git a/internal/configparse/parser_internal_test.go b/internal/configparse/parser_internal_test.go index 29aeccd..15a930d 100644 --- a/internal/configparse/parser_internal_test.go +++ b/internal/configparse/parser_internal_test.go @@ -64,7 +64,7 @@ func TestYAMLParser(t *testing.T) { t.Run(tc.label, func(t *testing.T) { var ( parser yamlParser - rawcfg UnmarshaledConfig + rawcfg Representation yamlErr *yaml.TypeError ) @@ -122,7 +122,7 @@ func TestJSONParser(t *testing.T) { t.Run(tc.label, func(t *testing.T) { var ( parser jsonParser - rawcfg UnmarshaledConfig + rawcfg Representation ) gotErr := parser.parse(tc.in, &rawcfg) From ad045f04285963861e929e6482c257fa8dd8ad85 Mon Sep 17 00:00:00 2001 From: Gregory Albouy Date: Sat, 8 Oct 2022 11:24:42 +0200 Subject: [PATCH 03/11] feat: expose package configparse: move package - ./internal/configparse -> ./configparse --- cmd/server/main.go | 2 +- {internal/configparse => configparse}/json.go | 0 .../configparse => configparse}/json_test.go | 2 +- .../configparse => configparse}/parse.go | 0 .../parser_internal_test.go | 0 internal/configparse/parser.go | 154 ------------------ 6 files changed, 2 insertions(+), 156 deletions(-) rename {internal/configparse => configparse}/json.go (100%) rename {internal/configparse => configparse}/json_test.go (97%) rename {internal/configparse => configparse}/parse.go (100%) rename {internal/configparse => configparse}/parser_internal_test.go (100%) delete mode 100644 internal/configparse/parser.go diff --git a/cmd/server/main.go b/cmd/server/main.go index a636c70..277091c 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -13,7 +13,7 @@ import ( "github.com/joho/godotenv" "github.com/benchttp/engine/cmd/server/response" - "github.com/benchttp/engine/internal/configparse" + "github.com/benchttp/engine/configparse" "github.com/benchttp/engine/runner" ) diff --git a/internal/configparse/json.go b/configparse/json.go similarity index 100% rename from internal/configparse/json.go rename to configparse/json.go diff --git a/internal/configparse/json_test.go b/configparse/json_test.go similarity index 97% rename from internal/configparse/json_test.go rename to configparse/json_test.go index 0947292..99e6fb4 100644 --- a/internal/configparse/json_test.go +++ b/configparse/json_test.go @@ -7,7 +7,7 @@ import ( "reflect" "testing" - "github.com/benchttp/engine/internal/configparse" + "github.com/benchttp/engine/configparse" "github.com/benchttp/engine/runner" ) diff --git a/internal/configparse/parse.go b/configparse/parse.go similarity index 100% rename from internal/configparse/parse.go rename to configparse/parse.go diff --git a/internal/configparse/parser_internal_test.go b/configparse/parser_internal_test.go similarity index 100% rename from internal/configparse/parser_internal_test.go rename to configparse/parser_internal_test.go diff --git a/internal/configparse/parser.go b/internal/configparse/parser.go deleted file mode 100644 index 2e7865b..0000000 --- a/internal/configparse/parser.go +++ /dev/null @@ -1,154 +0,0 @@ -package configparse - -import ( - "bytes" - "encoding/json" - "errors" - "fmt" - "regexp" - - "gopkg.in/yaml.v3" -) - -// yamlParser implements configParser. -type yamlParser struct{} - -// parse decodes a raw yaml input in strict mode (unknown fields disallowed) -// and stores the resulting value into dst. -func (p yamlParser) parse(in []byte, dst *Representation) error { - decoder := yaml.NewDecoder(bytes.NewReader(in)) - decoder.KnownFields(true) - return p.handleError(decoder.Decode(dst)) -} - -// handleError handles a raw yaml decoder.Decode error, filters it, -// and return the resulting error. -func (p yamlParser) handleError(err error) error { - // yaml.TypeError errors require special handling, other errors - // (nil included) can be returned as is. - var typeError *yaml.TypeError - if !errors.As(err, &typeError) { - return err - } - - // filter out unwanted errors - filtered := &yaml.TypeError{} - for _, msg := range typeError.Errors { - // With decoder.KnownFields set to true, Decode reports any field - // that do not match the destination structure as a non-nil error. - // It is a wanted behavior but prevents the usage of custom aliases. - // To work around this we allow an exception for that rule with fields - // starting with x- (inspired by docker compose api). - if p.isCustomFieldError(msg) { - continue - } - filtered.Errors = append(filtered.Errors, p.prettyErrorMessage(msg)) - } - - if len(filtered.Errors) != 0 { - return filtered - } - - return nil -} - -// isCustomFieldError returns true if the raw error message is due -// to an allowed custom field. -func (p yamlParser) isCustomFieldError(raw string) bool { - customFieldRgx := regexp.MustCompile( - // raw output example: - // line 9: field x-my-alias not found in type struct { ... } - `^line \d+: field (x-\S+) not found in type`, - ) - return customFieldRgx.MatchString(raw) -} - -// prettyErrorMessage transforms a raw Decode error message into a more -// user-friendly one by removing noisy information and returns the resulting -// value. -func (p yamlParser) prettyErrorMessage(raw string) string { - // field not found error - fieldNotFoundRgx := regexp.MustCompile( - // raw output example (type unmarshaledConfig is entirely exposed): - // line 11: field interval not found in type struct { ... } - `^line (\d+): field (\S+) not found in type`, - ) - if matches := fieldNotFoundRgx.FindStringSubmatch(raw); len(matches) >= 3 { - line, field := matches[1], matches[2] - return fmt.Sprintf(`line %s: invalid field ("%s"): does not exist`, line, field) - } - - // wrong field type error - fieldBadValueRgx := regexp.MustCompile( - // raw output examples: - // line 9: cannot unmarshal !!seq into int // unknown input value - // line 10: cannot unmarshal !!str `hello` into int // known input value - `^line (\d+): cannot unmarshal !!\w+(?: ` + "`" + `(\S+)` + "`" + `)? into (\S+)$`, - ) - if matches := fieldBadValueRgx.FindStringSubmatch(raw); len(matches) >= 3 { - line, value, exptype := matches[1], matches[2], matches[3] - if value == "" { - return fmt.Sprintf("line %s: wrong type: want %s", line, exptype) - } - return fmt.Sprintf(`line %s: wrong type ("%s"): want %s`, line, value, exptype) - } - - // we may not have covered all cases, return raw output in this case - return raw -} - -// jsonParser implements configParser. -type jsonParser struct{} - -// parse decodes a raw JSON input in strict mode (unknown fields disallowed) -// and stores the resulting value into dst. -func (p jsonParser) parse(in []byte, dst *Representation) error { - decoder := json.NewDecoder(bytes.NewReader(in)) - decoder.DisallowUnknownFields() - return p.handleError(decoder.Decode(dst)) -} - -// handleError handle a json raw error, transforms it into a user-friendly -// standardized format and returns the resulting error. -func (p jsonParser) handleError(err error) error { - if err == nil { - return nil - } - - // handle syntax error - var errSyntax *json.SyntaxError - if errors.As(err, &errSyntax) { - return fmt.Errorf("syntax error near %d: %w", errSyntax.Offset, err) - } - - // handle type error - var errType *json.UnmarshalTypeError - if errors.As(err, &errType) { - return fmt.Errorf( - "wrong type for field %s: want %s, got %s", - errType.Field, errType.Type, errType.Value, - ) - } - - // handle unknown field error - if field := p.parseUnknownFieldError(err.Error()); field != "" { - return fmt.Errorf(`invalid field ("%s"): does not exist`, field) - } - - return err -} - -// parseUnknownFieldError parses the raw string as a json error -// from an unknown field and returns the field name. -// If the raw string is not an unknown field error, it returns "". -func (p jsonParser) parseUnknownFieldError(raw string) (field string) { - unknownFieldRgx := regexp.MustCompile( - // raw output example: - // json: unknown field "notafield" - `json: unknown field "(\S+)"`, - ) - if matches := unknownFieldRgx.FindStringSubmatch(raw); len(matches) >= 2 { - return matches[1] - } - return "" -} From 7c800ab5653d88977f9a4a72cbcf9a10005e7a9d Mon Sep 17 00:00:00 2001 From: Gregory Albouy Date: Sat, 8 Oct 2022 11:28:15 +0200 Subject: [PATCH 04/11] feat: expose package configparse: expose parsers --- configparse/json.go | 4 +- configparse/parser.go | 154 ++++++++++++++++++ ...parser_internal_test.go => parser_test.go} | 16 +- 3 files changed, 165 insertions(+), 9 deletions(-) create mode 100644 configparse/parser.go rename configparse/{parser_internal_test.go => parser_test.go} (90%) diff --git a/configparse/json.go b/configparse/json.go index 83f5bf3..dbcf0d7 100644 --- a/configparse/json.go +++ b/configparse/json.go @@ -6,10 +6,10 @@ import ( // JSON reads input bytes as JSON and unmarshals it into a runner.ConfigGlobal. func JSON(in []byte) (runner.Config, error) { - parser := jsonParser{} + parser := JSONParser{} var repr Representation - if err := parser.parse(in, &repr); err != nil { + if err := parser.Parser(in, &repr); err != nil { return runner.Config{}, err } diff --git a/configparse/parser.go b/configparse/parser.go new file mode 100644 index 0000000..318d228 --- /dev/null +++ b/configparse/parser.go @@ -0,0 +1,154 @@ +package configparse + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "regexp" + + "gopkg.in/yaml.v3" +) + +// YAMLParser implements configParser. +type YAMLParser struct{} + +// Parse decodes a raw yaml input in strict mode (unknown fields disallowed) +// and stores the resulting value into dst. +func (p YAMLParser) Parse(in []byte, dst *Representation) error { + decoder := yaml.NewDecoder(bytes.NewReader(in)) + decoder.KnownFields(true) + return p.handleError(decoder.Decode(dst)) +} + +// handleError handles a raw yaml decoder.Decode error, filters it, +// and return the resulting error. +func (p YAMLParser) handleError(err error) error { + // yaml.TypeError errors require special handling, other errors + // (nil included) can be returned as is. + var typeError *yaml.TypeError + if !errors.As(err, &typeError) { + return err + } + + // filter out unwanted errors + filtered := &yaml.TypeError{} + for _, msg := range typeError.Errors { + // With decoder.KnownFields set to true, Decode reports any field + // that do not match the destination structure as a non-nil error. + // It is a wanted behavior but prevents the usage of custom aliases. + // To work around this we allow an exception for that rule with fields + // starting with x- (inspired by docker compose api). + if p.isCustomFieldError(msg) { + continue + } + filtered.Errors = append(filtered.Errors, p.prettyErrorMessage(msg)) + } + + if len(filtered.Errors) != 0 { + return filtered + } + + return nil +} + +// isCustomFieldError returns true if the raw error message is due +// to an allowed custom field. +func (p YAMLParser) isCustomFieldError(raw string) bool { + customFieldRgx := regexp.MustCompile( + // raw output example: + // line 9: field x-my-alias not found in type struct { ... } + `^line \d+: field (x-\S+) not found in type`, + ) + return customFieldRgx.MatchString(raw) +} + +// prettyErrorMessage transforms a raw Decode error message into a more +// user-friendly one by removing noisy information and returns the resulting +// value. +func (p YAMLParser) prettyErrorMessage(raw string) string { + // field not found error + fieldNotFoundRgx := regexp.MustCompile( + // raw output example (type unmarshaledConfig is entirely exposed): + // line 11: field interval not found in type struct { ... } + `^line (\d+): field (\S+) not found in type`, + ) + if matches := fieldNotFoundRgx.FindStringSubmatch(raw); len(matches) >= 3 { + line, field := matches[1], matches[2] + return fmt.Sprintf(`line %s: invalid field ("%s"): does not exist`, line, field) + } + + // wrong field type error + fieldBadValueRgx := regexp.MustCompile( + // raw output examples: + // line 9: cannot unmarshal !!seq into int // unknown input value + // line 10: cannot unmarshal !!str `hello` into int // known input value + `^line (\d+): cannot unmarshal !!\w+(?: ` + "`" + `(\S+)` + "`" + `)? into (\S+)$`, + ) + if matches := fieldBadValueRgx.FindStringSubmatch(raw); len(matches) >= 3 { + line, value, exptype := matches[1], matches[2], matches[3] + if value == "" { + return fmt.Sprintf("line %s: wrong type: want %s", line, exptype) + } + return fmt.Sprintf(`line %s: wrong type ("%s"): want %s`, line, value, exptype) + } + + // we may not have covered all cases, return raw output in this case + return raw +} + +// JSONParser implements configParser. +type JSONParser struct{} + +// Parser decodes a raw JSON input in strict mode (unknown fields disallowed) +// and stores the resulting value into dst. +func (p JSONParser) Parser(in []byte, dst *Representation) error { + decoder := json.NewDecoder(bytes.NewReader(in)) + decoder.DisallowUnknownFields() + return p.handleError(decoder.Decode(dst)) +} + +// handleError handle a json raw error, transforms it into a user-friendly +// standardized format and returns the resulting error. +func (p JSONParser) handleError(err error) error { + if err == nil { + return nil + } + + // handle syntax error + var errSyntax *json.SyntaxError + if errors.As(err, &errSyntax) { + return fmt.Errorf("syntax error near %d: %w", errSyntax.Offset, err) + } + + // handle type error + var errType *json.UnmarshalTypeError + if errors.As(err, &errType) { + return fmt.Errorf( + "wrong type for field %s: want %s, got %s", + errType.Field, errType.Type, errType.Value, + ) + } + + // handle unknown field error + if field := p.parseUnknownFieldError(err.Error()); field != "" { + return fmt.Errorf(`invalid field ("%s"): does not exist`, field) + } + + return err +} + +// parseUnknownFieldError parses the raw string as a json error +// from an unknown field and returns the field name. +// If the raw string is not an unknown field error, it returns "". +func (p JSONParser) parseUnknownFieldError(raw string) (field string) { + unknownFieldRgx := regexp.MustCompile( + // raw output example: + // json: unknown field "notafield" + `json: unknown field "(\S+)"`, + ) + if matches := unknownFieldRgx.FindStringSubmatch(raw); len(matches) >= 2 { + return matches[1] + } + return "" +} diff --git a/configparse/parser_internal_test.go b/configparse/parser_test.go similarity index 90% rename from configparse/parser_internal_test.go rename to configparse/parser_test.go index 15a930d..2160998 100644 --- a/configparse/parser_internal_test.go +++ b/configparse/parser_test.go @@ -1,4 +1,4 @@ -package configparse +package configparse_test import ( "errors" @@ -6,6 +6,8 @@ import ( "testing" "gopkg.in/yaml.v3" + + "github.com/benchttp/engine/configparse" ) func TestYAMLParser(t *testing.T) { @@ -63,12 +65,12 @@ func TestYAMLParser(t *testing.T) { for _, tc := range testcases { t.Run(tc.label, func(t *testing.T) { var ( - parser yamlParser - rawcfg Representation + parser configparse.YAMLParser + rawcfg configparse.Representation yamlErr *yaml.TypeError ) - gotErr := parser.parse(tc.in, &rawcfg) + gotErr := parser.Parse(tc.in, &rawcfg) if tc.expErr == nil { if gotErr != nil { @@ -121,11 +123,11 @@ func TestJSONParser(t *testing.T) { for _, tc := range testcases { t.Run(tc.label, func(t *testing.T) { var ( - parser jsonParser - rawcfg Representation + parser configparse.JSONParser + rawcfg configparse.Representation ) - gotErr := parser.parse(tc.in, &rawcfg) + gotErr := parser.Parser(tc.in, &rawcfg) if tc.exp == "" { if gotErr != nil { From 95ac3347436720b63c8c062ae2bbc8608f4c02c0 Mon Sep 17 00:00:00 2001 From: Gregory Albouy Date: Sat, 8 Oct 2022 11:30:40 +0200 Subject: [PATCH 05/11] refactor(configfile): split parsers into dedicated files --- configparse/parser_json.go | 65 +++++++++++++++++++ configparse/parser_json_test.go | 63 ++++++++++++++++++ configparse/{parser.go => parser_yaml.go} | 57 ---------------- .../{parser_test.go => parser_yaml_test.go} | 56 ---------------- 4 files changed, 128 insertions(+), 113 deletions(-) create mode 100644 configparse/parser_json.go create mode 100644 configparse/parser_json_test.go rename configparse/{parser.go => parser_yaml.go} (65%) rename configparse/{parser_test.go => parser_yaml_test.go} (61%) diff --git a/configparse/parser_json.go b/configparse/parser_json.go new file mode 100644 index 0000000..5accc75 --- /dev/null +++ b/configparse/parser_json.go @@ -0,0 +1,65 @@ +package configparse + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "regexp" +) + +// JSONParser implements configParser. +type JSONParser struct{} + +// Parser decodes a raw JSON input in strict mode (unknown fields disallowed) +// and stores the resulting value into dst. +func (p JSONParser) Parser(in []byte, dst *Representation) error { + decoder := json.NewDecoder(bytes.NewReader(in)) + decoder.DisallowUnknownFields() + return p.handleError(decoder.Decode(dst)) +} + +// handleError handle a json raw error, transforms it into a user-friendly +// standardized format and returns the resulting error. +func (p JSONParser) handleError(err error) error { + if err == nil { + return nil + } + + // handle syntax error + var errSyntax *json.SyntaxError + if errors.As(err, &errSyntax) { + return fmt.Errorf("syntax error near %d: %w", errSyntax.Offset, err) + } + + // handle type error + var errType *json.UnmarshalTypeError + if errors.As(err, &errType) { + return fmt.Errorf( + "wrong type for field %s: want %s, got %s", + errType.Field, errType.Type, errType.Value, + ) + } + + // handle unknown field error + if field := p.parseUnknownFieldError(err.Error()); field != "" { + return fmt.Errorf(`invalid field ("%s"): does not exist`, field) + } + + return err +} + +// parseUnknownFieldError parses the raw string as a json error +// from an unknown field and returns the field name. +// If the raw string is not an unknown field error, it returns "". +func (p JSONParser) parseUnknownFieldError(raw string) (field string) { + unknownFieldRgx := regexp.MustCompile( + // raw output example: + // json: unknown field "notafield" + `json: unknown field "(\S+)"`, + ) + if matches := unknownFieldRgx.FindStringSubmatch(raw); len(matches) >= 2 { + return matches[1] + } + return "" +} diff --git a/configparse/parser_json_test.go b/configparse/parser_json_test.go new file mode 100644 index 0000000..07c5610 --- /dev/null +++ b/configparse/parser_json_test.go @@ -0,0 +1,63 @@ +package configparse_test + +import ( + "testing" + + "github.com/benchttp/engine/configparse" +) + +func TestJSONParser(t *testing.T) { + t.Run("return expected errors", func(t *testing.T) { + testcases := []struct { + label string + in []byte + exp string + }{ + { + label: "syntax error", + in: []byte("{\n \"runner\": {},\n}\n"), + exp: "syntax error near 19: invalid character '}' looking for beginning of object key string", + }, + { + label: "unknown field", + in: []byte("{\n \"notafield\": 123\n}\n"), + exp: `invalid field ("notafield"): does not exist`, + }, + { + label: "wrong type", + in: []byte("{\n \"runner\": {\n \"requests\": [123]\n }\n}\n"), + exp: "wrong type for field runner.requests: want int, got array", + }, + { + label: "valid config", + in: []byte("{\n \"runner\": {\n \"requests\": 123\n }\n}\n"), + exp: "", + }, + } + + for _, tc := range testcases { + t.Run(tc.label, func(t *testing.T) { + var ( + parser configparse.JSONParser + rawcfg configparse.Representation + ) + + gotErr := parser.Parser(tc.in, &rawcfg) + + if tc.exp == "" { + if gotErr != nil { + t.Fatalf("unexpected error: %v", gotErr) + } + return + } + + if gotErr.Error() != tc.exp { + t.Errorf( + "unexpected error messages:\nexp %s\ngot %v", + tc.exp, gotErr, + ) + } + }) + } + }) +} diff --git a/configparse/parser.go b/configparse/parser_yaml.go similarity index 65% rename from configparse/parser.go rename to configparse/parser_yaml.go index 318d228..160cc7b 100644 --- a/configparse/parser.go +++ b/configparse/parser_yaml.go @@ -2,7 +2,6 @@ package configparse import ( "bytes" - "encoding/json" "errors" "fmt" "regexp" @@ -96,59 +95,3 @@ func (p YAMLParser) prettyErrorMessage(raw string) string { // we may not have covered all cases, return raw output in this case return raw } - -// JSONParser implements configParser. -type JSONParser struct{} - -// Parser decodes a raw JSON input in strict mode (unknown fields disallowed) -// and stores the resulting value into dst. -func (p JSONParser) Parser(in []byte, dst *Representation) error { - decoder := json.NewDecoder(bytes.NewReader(in)) - decoder.DisallowUnknownFields() - return p.handleError(decoder.Decode(dst)) -} - -// handleError handle a json raw error, transforms it into a user-friendly -// standardized format and returns the resulting error. -func (p JSONParser) handleError(err error) error { - if err == nil { - return nil - } - - // handle syntax error - var errSyntax *json.SyntaxError - if errors.As(err, &errSyntax) { - return fmt.Errorf("syntax error near %d: %w", errSyntax.Offset, err) - } - - // handle type error - var errType *json.UnmarshalTypeError - if errors.As(err, &errType) { - return fmt.Errorf( - "wrong type for field %s: want %s, got %s", - errType.Field, errType.Type, errType.Value, - ) - } - - // handle unknown field error - if field := p.parseUnknownFieldError(err.Error()); field != "" { - return fmt.Errorf(`invalid field ("%s"): does not exist`, field) - } - - return err -} - -// parseUnknownFieldError parses the raw string as a json error -// from an unknown field and returns the field name. -// If the raw string is not an unknown field error, it returns "". -func (p JSONParser) parseUnknownFieldError(raw string) (field string) { - unknownFieldRgx := regexp.MustCompile( - // raw output example: - // json: unknown field "notafield" - `json: unknown field "(\S+)"`, - ) - if matches := unknownFieldRgx.FindStringSubmatch(raw); len(matches) >= 2 { - return matches[1] - } - return "" -} diff --git a/configparse/parser_test.go b/configparse/parser_yaml_test.go similarity index 61% rename from configparse/parser_test.go rename to configparse/parser_yaml_test.go index 2160998..e80439a 100644 --- a/configparse/parser_test.go +++ b/configparse/parser_yaml_test.go @@ -90,59 +90,3 @@ func TestYAMLParser(t *testing.T) { } }) } - -func TestJSONParser(t *testing.T) { - t.Run("return expected errors", func(t *testing.T) { - testcases := []struct { - label string - in []byte - exp string - }{ - { - label: "syntax error", - in: []byte("{\n \"runner\": {},\n}\n"), - exp: "syntax error near 19: invalid character '}' looking for beginning of object key string", - }, - { - label: "unknown field", - in: []byte("{\n \"notafield\": 123\n}\n"), - exp: `invalid field ("notafield"): does not exist`, - }, - { - label: "wrong type", - in: []byte("{\n \"runner\": {\n \"requests\": [123]\n }\n}\n"), - exp: "wrong type for field runner.requests: want int, got array", - }, - { - label: "valid config", - in: []byte("{\n \"runner\": {\n \"requests\": 123\n }\n}\n"), - exp: "", - }, - } - - for _, tc := range testcases { - t.Run(tc.label, func(t *testing.T) { - var ( - parser configparse.JSONParser - rawcfg configparse.Representation - ) - - gotErr := parser.Parser(tc.in, &rawcfg) - - if tc.exp == "" { - if gotErr != nil { - t.Fatalf("unexpected error: %v", gotErr) - } - return - } - - if gotErr.Error() != tc.exp { - t.Errorf( - "unexpected error messages:\nexp %s\ngot %v", - tc.exp, gotErr, - ) - } - }) - } - }) -} From 8614149f63409ebf314c9f6f2ac1bccaf9105071 Mon Sep 17 00:00:00 2001 From: Gregory Albouy Date: Sat, 8 Oct 2022 11:36:03 +0200 Subject: [PATCH 06/11] fix(configparse): typo --- configparse/json.go | 2 +- configparse/parser_json.go | 4 ++-- configparse/parser_json_test.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/configparse/json.go b/configparse/json.go index dbcf0d7..97936a4 100644 --- a/configparse/json.go +++ b/configparse/json.go @@ -9,7 +9,7 @@ func JSON(in []byte) (runner.Config, error) { parser := JSONParser{} var repr Representation - if err := parser.Parser(in, &repr); err != nil { + if err := parser.Parse(in, &repr); err != nil { return runner.Config{}, err } diff --git a/configparse/parser_json.go b/configparse/parser_json.go index 5accc75..ff7c64b 100644 --- a/configparse/parser_json.go +++ b/configparse/parser_json.go @@ -11,9 +11,9 @@ import ( // JSONParser implements configParser. type JSONParser struct{} -// Parser decodes a raw JSON input in strict mode (unknown fields disallowed) +// Parse decodes a raw JSON input in strict mode (unknown fields disallowed) // and stores the resulting value into dst. -func (p JSONParser) Parser(in []byte, dst *Representation) error { +func (p JSONParser) Parse(in []byte, dst *Representation) error { decoder := json.NewDecoder(bytes.NewReader(in)) decoder.DisallowUnknownFields() return p.handleError(decoder.Decode(dst)) diff --git a/configparse/parser_json_test.go b/configparse/parser_json_test.go index 07c5610..51a4914 100644 --- a/configparse/parser_json_test.go +++ b/configparse/parser_json_test.go @@ -42,7 +42,7 @@ func TestJSONParser(t *testing.T) { rawcfg configparse.Representation ) - gotErr := parser.Parser(tc.in, &rawcfg) + gotErr := parser.Parse(tc.in, &rawcfg) if tc.exp == "" { if gotErr != nil { From 9c50630e25e0c9bcfb67f399fb669d42541a425e Mon Sep 17 00:00:00 2001 From: Gregory Albouy Date: Sat, 8 Oct 2022 11:36:48 +0200 Subject: [PATCH 07/11] feat: expose package configparse: expose ParseRepresentation - rename nesParsedConfig -> ParseRepresentation --- configparse/json.go | 2 +- configparse/parse.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/configparse/json.go b/configparse/json.go index 97936a4..d5fb93d 100644 --- a/configparse/json.go +++ b/configparse/json.go @@ -13,7 +13,7 @@ func JSON(in []byte) (runner.Config, error) { return runner.Config{}, err } - cfg, err := newParsedConfig(repr) + cfg, err := ParseRepresentation(repr) if err != nil { return runner.Config{}, err } diff --git a/configparse/parse.go b/configparse/parse.go index f25780f..416941b 100644 --- a/configparse/parse.go +++ b/configparse/parse.go @@ -48,9 +48,9 @@ type Representation struct { } `yaml:"tests" json:"tests"` } -// newParsedConfig parses an input raw config as a runner.ConfigGlobal and returns +// ParseRepresentation parses an input raw config as a runner.ConfigGlobal and returns // a parsed Config or the first non-nil error occurring in the process. -func newParsedConfig(repr Representation) (runner.Config, error) { //nolint:gocognit // acceptable complexity for a parsing func +func ParseRepresentation(repr Representation) (runner.Config, error) { //nolint:gocognit // acceptable complexity for a parsing func cfg := runner.Config{} fieldsSet := []string{} From 273f08b88b63a4a4262ac1e3d67d793a71c889fb Mon Sep 17 00:00:00 2001 From: Gregory Albouy Date: Sat, 8 Oct 2022 12:33:35 +0200 Subject: [PATCH 08/11] refactor(runner): reverse order of Config.Override - prev.Override(next) -> next.Override(prev) --- configparse/json.go | 2 +- configparse/json_test.go | 20 +++++++------- runner/internal/config/config.go | 29 +++++++++----------- runner/internal/config/config_test.go | 39 ++++++++++++++------------- 4 files changed, 44 insertions(+), 46 deletions(-) diff --git a/configparse/json.go b/configparse/json.go index d5fb93d..ed052c9 100644 --- a/configparse/json.go +++ b/configparse/json.go @@ -18,5 +18,5 @@ func JSON(in []byte) (runner.Config, error) { return runner.Config{}, err } - return runner.DefaultConfig().Override(cfg), nil + return cfg.Override(runner.DefaultConfig()), nil } diff --git a/configparse/json_test.go b/configparse/json_test.go index 99e6fb4..69b44d7 100644 --- a/configparse/json_test.go +++ b/configparse/json_test.go @@ -47,16 +47,16 @@ func TestJSON(t *testing.T) { input: baseInput.assign(object{ "runner": object{"concurrency": 3}, }).json(), - expConfig: runner.DefaultConfig().Override( - runner.Config{ - Request: runner.RequestConfig{ - URL: mustParseURL("https://example.com"), - }, - Runner: runner.RecorderConfig{ - Concurrency: 3, - }, - }.WithFields("url", "concurrency"), - ), + expConfig: runner.Config{ + Request: runner.RequestConfig{ + URL: mustParseURL("https://example.com"), + }, + Runner: runner.RecorderConfig{ + Concurrency: 3, + }, + }. + WithFields("url", "concurrency"). + Override(runner.DefaultConfig()), expError: nil, }, } diff --git a/runner/internal/config/config.go b/runner/internal/config/config.go index 9494b78..fd3adb1 100644 --- a/runner/internal/config/config.go +++ b/runner/internal/config/config.go @@ -124,36 +124,33 @@ func (cfg Global) String() string { // Only fields specified in options are replaced. Accepted options are limited // to existing Fields, other values are silently ignored. func (cfg Global) Override(c Global) Global { - if len(c.fieldsSet) == 0 { - return cfg - } - for field := range c.fieldsSet { + for field := range cfg.fieldsSet { switch field { case FieldMethod: - cfg.Request.Method = c.Request.Method + c.Request.Method = cfg.Request.Method case FieldURL: - cfg.Request.URL = c.Request.URL + c.Request.URL = cfg.Request.URL case FieldHeader: - cfg.overrideHeader(c.Request.Header) + c.overrideHeader(cfg.Request.Header) case FieldBody: - cfg.Request.Body = c.Request.Body + c.Request.Body = cfg.Request.Body case FieldRequests: - cfg.Runner.Requests = c.Runner.Requests + c.Runner.Requests = cfg.Runner.Requests case FieldConcurrency: - cfg.Runner.Concurrency = c.Runner.Concurrency + c.Runner.Concurrency = cfg.Runner.Concurrency case FieldInterval: - cfg.Runner.Interval = c.Runner.Interval + c.Runner.Interval = cfg.Runner.Interval case FieldRequestTimeout: - cfg.Runner.RequestTimeout = c.Runner.RequestTimeout + c.Runner.RequestTimeout = cfg.Runner.RequestTimeout case FieldGlobalTimeout: - cfg.Runner.GlobalTimeout = c.Runner.GlobalTimeout + c.Runner.GlobalTimeout = cfg.Runner.GlobalTimeout case FieldSilent: - cfg.Output.Silent = c.Output.Silent + c.Output.Silent = cfg.Output.Silent case FieldTests: - cfg.Tests = c.Tests + c.Tests = cfg.Tests } } - return cfg + return c } // overrideHeader overrides cfg's Request.Header with the values from newHeader. diff --git a/runner/internal/config/config_test.go b/runner/internal/config/config_test.go index 56e62ac..578f7b8 100644 --- a/runner/internal/config/config_test.go +++ b/runner/internal/config/config_test.go @@ -73,7 +73,7 @@ func TestGlobal_Validate(t *testing.T) { func TestGlobal_Override(t *testing.T) { t.Run("do not override unspecified fields", func(t *testing.T) { baseCfg := config.Global{} - newCfg := config.Global{ + nextCfg := config.Global{ Request: config.Request{ Body: config.RequestBody{}, }.WithURL("http://a.b?p=2"), @@ -88,14 +88,25 @@ func TestGlobal_Override(t *testing.T) { }, } - if gotCfg := baseCfg.Override(newCfg); !reflect.DeepEqual(gotCfg, baseCfg) { + if gotCfg := nextCfg.Override(baseCfg); !reflect.DeepEqual(gotCfg, baseCfg) { t.Errorf("overrode unexpected fields:\nexp %#v\ngot %#v", baseCfg, gotCfg) } }) t.Run("override specified fields", func(t *testing.T) { + fields := []string{ + config.FieldMethod, + config.FieldURL, + config.FieldRequests, + config.FieldConcurrency, + config.FieldRequestTimeout, + config.FieldGlobalTimeout, + config.FieldBody, + config.FieldSilent, + } + baseCfg := config.Global{} - newCfg := config.Global{ + nextCfg := config.Global{ Request: config.Request{ Body: validBody, }.WithURL("http://a.b?p=2"), @@ -108,19 +119,9 @@ func TestGlobal_Override(t *testing.T) { Output: config.Output{ Silent: true, }, - } - fields := []string{ - config.FieldMethod, - config.FieldURL, - config.FieldRequests, - config.FieldConcurrency, - config.FieldRequestTimeout, - config.FieldGlobalTimeout, - config.FieldBody, - config.FieldSilent, - } + }.WithFields(fields...) - if gotCfg := baseCfg.Override(newCfg.WithFields(fields...)); !reflect.DeepEqual(gotCfg, newCfg) { + if gotCfg := nextCfg.Override(baseCfg); !reflect.DeepEqual(gotCfg, nextCfg) { t.Errorf("did not override expected fields:\nexp %v\ngot %v", baseCfg, gotCfg) t.Log(fields) } @@ -192,19 +193,19 @@ func TestGlobal_Override(t *testing.T) { for _, tc := range testcases { t.Run(tc.label, func(t *testing.T) { - oldCfg := config.Global{ + baseCfg := config.Global{ Request: config.Request{ Header: tc.oldHeader, }, } - newCfg := config.Global{ + nextCfg := config.Global{ Request: config.Request{ Header: tc.newHeader, }, - } + }.WithFields(config.FieldHeader) - gotCfg := oldCfg.Override(newCfg.WithFields(config.FieldHeader)) + gotCfg := nextCfg.Override(baseCfg) if gotHeader := gotCfg.Request.Header; !reflect.DeepEqual(gotHeader, tc.expHeader) { t.Errorf("\nexp %#v\ngot %#v", tc.expHeader, gotHeader) From 467ea5a3926c4dfbc004d2890cd08eab135c992e Mon Sep 17 00:00:00 2001 From: Gregory Albouy Date: Sat, 8 Oct 2022 12:39:16 +0200 Subject: [PATCH 09/11] fix(runner): use dedicated method for config comparison We cannot rely on reflect.DeepEqual anymore as private field config.fieldsSet can vary for 2 identical configs. - implement Config.Equal that ignores Config.fieldsSet - write unit tests for Config.Equal - use Config.Equal in tests --- configparse/json_test.go | 3 +- runner/internal/config/config.go | 8 +++++ runner/internal/config/config_test.go | 43 +++++++++++++++++++++++++-- 3 files changed, 49 insertions(+), 5 deletions(-) diff --git a/configparse/json_test.go b/configparse/json_test.go index 69b44d7..142c98d 100644 --- a/configparse/json_test.go +++ b/configparse/json_test.go @@ -4,7 +4,6 @@ import ( "encoding/json" "errors" "net/url" - "reflect" "testing" "github.com/benchttp/engine/configparse" @@ -64,7 +63,7 @@ func TestJSON(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { gotConfig, gotError := configparse.JSON(tc.input) - if !reflect.DeepEqual(gotConfig, tc.expConfig) { + if !gotConfig.Equal(tc.expConfig) { t.Errorf("unexpected config:\nexp %+v\ngot %+v", tc.expConfig, gotConfig) } diff --git a/runner/internal/config/config.go b/runner/internal/config/config.go index fd3adb1..bcf788d 100644 --- a/runner/internal/config/config.go +++ b/runner/internal/config/config.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "net/url" + "reflect" "time" "github.com/benchttp/engine/runner/internal/tests" @@ -120,6 +121,13 @@ func (cfg Global) String() string { return string(b) } +// Equal returns true if cfg and c are equal configurations. +func (cfg Global) Equal(c Global) bool { + cfg.fieldsSet = nil + c.fieldsSet = nil + return reflect.DeepEqual(cfg, c) +} + // Override returns a new Config based on cfg with overridden values from c. // Only fields specified in options are replaced. Accepted options are limited // to existing Fields, other values are silently ignored. diff --git a/runner/internal/config/config_test.go b/runner/internal/config/config_test.go index 578f7b8..92cc3ee 100644 --- a/runner/internal/config/config_test.go +++ b/runner/internal/config/config_test.go @@ -88,7 +88,7 @@ func TestGlobal_Override(t *testing.T) { }, } - if gotCfg := nextCfg.Override(baseCfg); !reflect.DeepEqual(gotCfg, baseCfg) { + if gotCfg := nextCfg.Override(baseCfg); !gotCfg.Equal(baseCfg) { t.Errorf("overrode unexpected fields:\nexp %#v\ngot %#v", baseCfg, gotCfg) } }) @@ -121,8 +121,8 @@ func TestGlobal_Override(t *testing.T) { }, }.WithFields(fields...) - if gotCfg := nextCfg.Override(baseCfg); !reflect.DeepEqual(gotCfg, nextCfg) { - t.Errorf("did not override expected fields:\nexp %v\ngot %v", baseCfg, gotCfg) + if gotCfg := nextCfg.Override(baseCfg); !gotCfg.Equal(nextCfg) { + t.Errorf("did not override expected fields:\nexp %v\ngot %v", nextCfg, gotCfg) t.Log(fields) } }) @@ -304,6 +304,43 @@ func TestRequest_Value(t *testing.T) { }) } +func TestGlobal_Equal(t *testing.T) { + t.Run("returns false for different configs", func(t *testing.T) { + base := config.Default() + diff := base + diff.Runner.Requests = base.Runner.Requests + 1 + + if base.Equal(diff) { + t.Error("exp unequal configs") + } + }) + + t.Run("ignores set fields", func(t *testing.T) { + base := config.Default() + same := base.WithFields(config.FieldRequests) + + if !base.Equal(same) { + t.Error("exp equal configs") + } + }) + + t.Run("does not alter configs", func(t *testing.T) { + baseA := config.Default().WithFields(config.FieldRequests) + copyA := config.Default().WithFields(config.FieldRequests) + baseB := config.Default().WithFields(config.FieldURL) + copyB := config.Default().WithFields(config.FieldURL) + + baseA.Equal(baseB) + + if !reflect.DeepEqual(baseA, copyA) { + t.Error("altered receiver config") + } + if !reflect.DeepEqual(baseB, copyB) { + t.Error("altered parameter config") + } + }) +} + // helpers // findErrorOrFail fails t if no error in src matches msg. From 733675ba825fd16965b48391f94d35a313f11864 Mon Sep 17 00:00:00 2001 From: Gregory Albouy Date: Sat, 8 Oct 2022 13:40:24 +0200 Subject: [PATCH 10/11] chore: remove output and silent from runner config --- configparse/parse.go | 9 --------- examples/config/default.yml | 3 --- examples/config/full.yml | 3 --- runner/internal/config/config.go | 11 ++--------- runner/internal/config/config_test.go | 7 ------- runner/internal/config/default.go | 3 --- runner/internal/config/field.go | 2 -- runner/internal/config/field_test.go | 1 - runner/runner.go | 2 -- 9 files changed, 2 insertions(+), 39 deletions(-) diff --git a/configparse/parse.go b/configparse/parse.go index 416941b..8e95508 100644 --- a/configparse/parse.go +++ b/configparse/parse.go @@ -36,10 +36,6 @@ type Representation struct { GlobalTimeout *string `yaml:"globalTimeout" json:"globalTimeout"` } `yaml:"runner" json:"runner"` - Output struct { - Silent *bool `yaml:"silent" json:"silent"` - } `yaml:"output" json:"output"` - Tests []struct { Name *string `yaml:"name" json:"name"` Field *string `yaml:"field" json:"field"` @@ -130,11 +126,6 @@ func ParseRepresentation(repr Representation) (runner.Config, error) { //nolint: addField(runner.ConfigFieldGlobalTimeout) } - if silent := repr.Output.Silent; silent != nil { - cfg.Output.Silent = *silent - addField(runner.ConfigFieldSilent) - } - testSuite := repr.Tests if len(testSuite) == 0 { return cfg.WithFields(fieldsSet...), nil diff --git a/examples/config/default.yml b/examples/config/default.yml index 07e6bea..53369ae 100644 --- a/examples/config/default.yml +++ b/examples/config/default.yml @@ -8,6 +8,3 @@ runner: interval: 0ms requestTimeout: 5s globalTimeout: 30s - -output: - silent: false diff --git a/examples/config/full.yml b/examples/config/full.yml index c20fb5e..c83d434 100644 --- a/examples/config/full.yml +++ b/examples/config/full.yml @@ -17,6 +17,3 @@ runner: interval: 50ms requestTimeout: 2s globalTimeout: 60s - -output: - silent: true diff --git a/runner/internal/config/config.go b/runner/internal/config/config.go index bcf788d..4ecd725 100644 --- a/runner/internal/config/config.go +++ b/runner/internal/config/config.go @@ -79,11 +79,6 @@ type Runner struct { GlobalTimeout time.Duration } -// Output contains options relative to the output. -type Output struct { - Silent bool -} - type set map[string]struct{} func (set set) add(values ...string) { @@ -97,8 +92,8 @@ func (set set) add(values ...string) { type Global struct { Request Request Runner Runner - Output Output - Tests []tests.Case + + Tests []tests.Case fieldsSet set } @@ -152,8 +147,6 @@ func (cfg Global) Override(c Global) Global { c.Runner.RequestTimeout = cfg.Runner.RequestTimeout case FieldGlobalTimeout: c.Runner.GlobalTimeout = cfg.Runner.GlobalTimeout - case FieldSilent: - c.Output.Silent = cfg.Output.Silent case FieldTests: c.Tests = cfg.Tests } diff --git a/runner/internal/config/config_test.go b/runner/internal/config/config_test.go index 92cc3ee..6d43ebe 100644 --- a/runner/internal/config/config_test.go +++ b/runner/internal/config/config_test.go @@ -83,9 +83,6 @@ func TestGlobal_Override(t *testing.T) { RequestTimeout: 3 * time.Second, GlobalTimeout: 4 * time.Second, }, - Output: config.Output{ - Silent: true, - }, } if gotCfg := nextCfg.Override(baseCfg); !gotCfg.Equal(baseCfg) { @@ -102,7 +99,6 @@ func TestGlobal_Override(t *testing.T) { config.FieldRequestTimeout, config.FieldGlobalTimeout, config.FieldBody, - config.FieldSilent, } baseCfg := config.Global{} @@ -116,9 +112,6 @@ func TestGlobal_Override(t *testing.T) { RequestTimeout: 3 * time.Second, GlobalTimeout: 4 * time.Second, }, - Output: config.Output{ - Silent: true, - }, }.WithFields(fields...) if gotCfg := nextCfg.Override(baseCfg); !gotCfg.Equal(nextCfg) { diff --git a/runner/internal/config/default.go b/runner/internal/config/default.go index b07e90d..b5299de 100644 --- a/runner/internal/config/default.go +++ b/runner/internal/config/default.go @@ -20,9 +20,6 @@ var defaultConfig = Global{ RequestTimeout: 5 * time.Second, GlobalTimeout: 30 * time.Second, }, - Output: Output{ - Silent: false, - }, } // Default returns a default config that is safe to use. diff --git a/runner/internal/config/field.go b/runner/internal/config/field.go index 693ae33..c23c831 100644 --- a/runner/internal/config/field.go +++ b/runner/internal/config/field.go @@ -10,7 +10,6 @@ const ( FieldInterval = "interval" FieldRequestTimeout = "requestTimeout" FieldGlobalTimeout = "globalTimeout" - FieldSilent = "silent" FieldTests = "tests" ) @@ -25,7 +24,6 @@ var FieldsUsage = map[string]string{ FieldInterval: "Minimum duration between two non concurrent requests", FieldRequestTimeout: "Timeout for each HTTP request", FieldGlobalTimeout: "Max duration of test", - FieldSilent: "Silent mode (no write to stdout)", FieldTests: "Test suite", } diff --git a/runner/internal/config/field_test.go b/runner/internal/config/field_test.go index 2ba02c9..ad53a23 100644 --- a/runner/internal/config/field_test.go +++ b/runner/internal/config/field_test.go @@ -19,7 +19,6 @@ func TestIsField(t *testing.T) { {In: config.FieldInterval, Exp: true}, {In: config.FieldRequestTimeout, Exp: true}, {In: config.FieldGlobalTimeout, Exp: true}, - {In: config.FieldSilent, Exp: true}, {In: "notafield", Exp: false}, }).Run(t) } diff --git a/runner/runner.go b/runner/runner.go index c705c78..f49067d 100644 --- a/runner/runner.go +++ b/runner/runner.go @@ -16,7 +16,6 @@ type ( RequestConfig = config.Request RequestBody = config.RequestBody RecorderConfig = config.Runner - OutputConfig = config.Output RecordingProgress = recorder.Progress RecordingStatus = recorder.Status @@ -51,7 +50,6 @@ const ( ConfigFieldInterval = config.FieldInterval ConfigFieldRequestTimeout = config.FieldRequestTimeout ConfigFieldGlobalTimeout = config.FieldGlobalTimeout - ConfigFieldSilent = config.FieldSilent ConfigFieldTests = config.FieldTests ) From c733c2ae191fe66b72b544a597e948b51a477739 Mon Sep 17 00:00:00 2001 From: Gregory Albouy Date: Sat, 8 Oct 2022 18:43:56 +0200 Subject: [PATCH 11/11] refactor(runner): misc improements - improve documentation of Config.Override - rename Config.fieldsSet -> Config.assignedFields - refactor Config.WithFields --- configparse/parse.go | 8 ++-- runner/internal/config/config.go | 66 +++++++++++++++++++------------- 2 files changed, 44 insertions(+), 30 deletions(-) diff --git a/configparse/parse.go b/configparse/parse.go index 8e95508..66761c7 100644 --- a/configparse/parse.go +++ b/configparse/parse.go @@ -48,10 +48,10 @@ type Representation struct { // a parsed Config or the first non-nil error occurring in the process. func ParseRepresentation(repr Representation) (runner.Config, error) { //nolint:gocognit // acceptable complexity for a parsing func cfg := runner.Config{} - fieldsSet := []string{} + assignedFields := []string{} addField := func(field string) { - fieldsSet = append(fieldsSet, field) + assignedFields = append(assignedFields, field) } abort := func(err error) (runner.Config, error) { @@ -128,7 +128,7 @@ func ParseRepresentation(repr Representation) (runner.Config, error) { //nolint: testSuite := repr.Tests if len(testSuite) == 0 { - return cfg.WithFields(fieldsSet...), nil + return cfg.WithFields(assignedFields...), nil } cases := make([]runner.TestCase, len(testSuite)) @@ -171,7 +171,7 @@ func ParseRepresentation(repr Representation) (runner.Config, error) { //nolint: cfg.Tests = cases addField(runner.ConfigFieldTests) - return cfg.WithFields(fieldsSet...), nil + return cfg.WithFields(assignedFields...), nil } // helpers diff --git a/runner/internal/config/config.go b/runner/internal/config/config.go index 4ecd725..837aa2e 100644 --- a/runner/internal/config/config.go +++ b/runner/internal/config/config.go @@ -95,22 +95,22 @@ type Global struct { Tests []tests.Case - fieldsSet set + assignedFields set } // WithField returns a new Global with the input fields marked as set. +// Accepted options are limited to existing Fields, other values are +// silently ignored. func (cfg Global) WithFields(fields ...string) Global { - fieldsSet := cfg.fieldsSet - if fieldsSet == nil { - fieldsSet = set{} + if cfg.assignedFields == nil { + cfg.assignedFields = set{} } - fieldsSet.add(fields...) - cfg.fieldsSet = fieldsSet + cfg.assignedFields.add(fields...) return cfg } -// String returns an indented JSON representation of Config -// for debugging purposes. +// String implements fmt.Stringer. It returns an indented JSON representation +// of Config for debugging purposes. func (cfg Global) String() string { b, _ := json.MarshalIndent(cfg, "", " ") return string(b) @@ -118,40 +118,54 @@ func (cfg Global) String() string { // Equal returns true if cfg and c are equal configurations. func (cfg Global) Equal(c Global) bool { - cfg.fieldsSet = nil - c.fieldsSet = nil + cfg.assignedFields = nil + c.assignedFields = nil return reflect.DeepEqual(cfg, c) } -// Override returns a new Config based on cfg with overridden values from c. -// Only fields specified in options are replaced. Accepted options are limited -// to existing Fields, other values are silently ignored. -func (cfg Global) Override(c Global) Global { - for field := range cfg.fieldsSet { +// Override returns a new Config by overriding the values of base +// with the values from the Config receiver. +// Only fields previously specified by the receiver via Config.WithFields +// are replaced. +// All other values from base are preserved. +// +// The following example is equivalent to defaultConfig with the concurrency +// value from myConfig: +// +// myConfig. +// WithFields(FieldConcurrency). +// Override(defaultConfig) +// +// The following example is equivalent to defaultConfig, as no field as been +// tagged via WithFields by the receiver: +// +// myConfig.Override(defaultConfig) +func (cfg Global) Override(base Global) Global { + for field := range cfg.assignedFields { switch field { case FieldMethod: - c.Request.Method = cfg.Request.Method + base.Request.Method = cfg.Request.Method case FieldURL: - c.Request.URL = cfg.Request.URL + base.Request.URL = cfg.Request.URL case FieldHeader: - c.overrideHeader(cfg.Request.Header) + base.overrideHeader(cfg.Request.Header) case FieldBody: - c.Request.Body = cfg.Request.Body + base.Request.Body = cfg.Request.Body case FieldRequests: - c.Runner.Requests = cfg.Runner.Requests + base.Runner.Requests = cfg.Runner.Requests case FieldConcurrency: - c.Runner.Concurrency = cfg.Runner.Concurrency + base.Runner.Concurrency = cfg.Runner.Concurrency case FieldInterval: - c.Runner.Interval = cfg.Runner.Interval + base.Runner.Interval = cfg.Runner.Interval case FieldRequestTimeout: - c.Runner.RequestTimeout = cfg.Runner.RequestTimeout + base.Runner.RequestTimeout = cfg.Runner.RequestTimeout case FieldGlobalTimeout: - c.Runner.GlobalTimeout = cfg.Runner.GlobalTimeout + base.Runner.GlobalTimeout = cfg.Runner.GlobalTimeout case FieldTests: - c.Tests = cfg.Tests + base.Tests = cfg.Tests } } - return c + return base } // overrideHeader overrides cfg's Request.Header with the values from newHeader.