From da086e3a36f9955a44dd466477cd1e6d8ad47ad8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Miri=C4=87?= Date: Tue, 3 Sep 2019 09:33:15 +0200 Subject: [PATCH 1/4] feat(ui): add optional 'count' column to CLI summary output This adds an optional count column for Trend metrics in the CLI summary output, while refactoring parts of ui/summary.go for code style (avoiding globals, explicit validation, etc.) and negligible performance improvements (using a map for column lookups). To enable it run e.g. `k6 run --summary-trend-stats 'avg,p(99.99),count' test.js`. Closes #1087 --- cmd/config.go | 17 ++- cmd/config_consolidation_test.go | 14 ++ cmd/options.go | 26 ++-- cmd/run.go | 18 ++- lib/options.go | 4 + ui/summary.go | 210 +++++++++++++++------------ ui/summary_test.go | 234 ++++++++++++++++--------------- 7 files changed, 295 insertions(+), 228 deletions(-) diff --git a/cmd/config.go b/cmd/config.go index 3d07b68e1a0..019c90c274f 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -44,6 +44,7 @@ import ( "github.com/loadimpact/k6/stats/influxdb" "github.com/loadimpact/k6/stats/kafka" "github.com/loadimpact/k6/stats/statsd/common" + "github.com/loadimpact/k6/ui" ) // configFlagSet returns a FlagSet with the default run configuration flags. @@ -309,17 +310,29 @@ func getConsolidatedConfig(fs afero.Fs, cliConf Config, runner lib.Runner) (conf conf = conf.Apply(envConf).Apply(cliConf) conf = applyDefault(conf) + // TODO(imiric): Move this validation where it makes sense in the configuration + // refactor of #883. This repeats the trend stats validation already done + // for CLI flags in cmd.getOptions, in case other configuration sources + // (e.g. env vars) overrode our default value. This is not done in + // lib.Options.Validate to avoid circular imports. + if err = ui.ValidateSummary(conf.SummaryTrendStats); err != nil { + return conf, err + } + return conf, nil } -// applyDefault applys default options value if it is not specified by any mechenisms. This happens with types -// which does not support by "gopkg.in/guregu/null.v3". +// applyDefault applies the default options value if it is not specified. +// This happens with types which are not supported by "gopkg.in/guregu/null.v3". // // Note that if you add option default value here, also add it in command line argument help text. func applyDefault(conf Config) Config { if conf.Options.SystemTags == nil { conf = conf.Apply(Config{Options: lib.Options{SystemTags: &stats.DefaultSystemTagSet}}) } + if len(conf.Options.SummaryTrendStats) == 0 { + conf = conf.Apply(Config{Options: lib.Options{SummaryTrendStats: lib.DefaultSummaryTrendStats}}) + } return conf } diff --git a/cmd/config_consolidation_test.go b/cmd/config_consolidation_test.go index 6614ee14de3..74db5b5fb26 100644 --- a/cmd/config_consolidation_test.go +++ b/cmd/config_consolidation_test.go @@ -412,6 +412,20 @@ func getConfigConsolidationTestCases() []configConsolidationTestCase { ) }, }, + // Test summary trend stats + {opts{}, exp{}, func(t *testing.T, c Config) { + assert.Equal(t, lib.DefaultSummaryTrendStats, c.Options.SummaryTrendStats) + }}, + {opts{cli: []string{"--summary-trend-stats", `""`}}, exp{}, func(t *testing.T, c Config) { + assert.Equal(t, lib.DefaultSummaryTrendStats, c.Options.SummaryTrendStats) + }}, + { + opts{runner: &lib.Options{SummaryTrendStats: []string{"avg", "p(90)", "count"}}}, + exp{}, + func(t *testing.T, c Config) { + assert.Equal(t, []string{"avg", "p(90)", "count"}, c.Options.SummaryTrendStats) + }, + }, //TODO: test for differences between flagsets //TODO: more tests in general, especially ones not related to execution parameters... } diff --git a/cmd/options.go b/cmd/options.go index dd88971e42f..38225930323 100644 --- a/cmd/options.go +++ b/cmd/options.go @@ -63,7 +63,14 @@ func optionFlagSet() *pflag.FlagSet { flags.Duration("min-iteration-duration", 0, "minimum amount of time k6 will take executing a single iteration") flags.BoolP("throw", "w", false, "throw warnings (like failed http requests) as errors") flags.StringSlice("blacklist-ip", nil, "blacklist an `ip range` from being called") - flags.StringSlice("summary-trend-stats", nil, "define `stats` for trend metrics (response times), one or more as 'avg,p(95),...'") + + // The comment about system-tags also applies for summary-trend-stats. The default values + // are set in applyDefault(). + sumTrendStatsHelp := fmt.Sprintf( + "define `stats` for trend metrics (response times), one or more as 'avg,p(95),...' (default '%s')", + strings.Join(lib.DefaultSummaryTrendStats, ","), + ) + flags.StringSlice("summary-trend-stats", nil, sumTrendStatsHelp) flags.String("summary-time-unit", "", "define the time unit used to display the trend stats. Possible units are: 's', 'ms' and 'us'") // system-tags must have a default value, but we can't specify it here, otherwiese, it will always override others. // set it to nil here, and add the default in applyDefault() instead. @@ -143,16 +150,15 @@ func getOptions(flags *pflag.FlagSet) (lib.Options, error) { opts.BlacklistIPs = append(opts.BlacklistIPs, net) } - trendStatStrings, err := flags.GetStringSlice("summary-trend-stats") - if err != nil { - return opts, err - } - for _, s := range trendStatStrings { - if err := ui.VerifyTrendColumnStat(s); err != nil { - return opts, errors.Wrapf(err, "stat '%s'", s) + if flags.Changed("summary-trend-stats") { + trendStats, errSts := flags.GetStringSlice("summary-trend-stats") + if errSts != nil { + return opts, errSts } - - opts.SummaryTrendStats = append(opts.SummaryTrendStats, s) + if errSts = ui.ValidateSummary(trendStats); err != nil { + return opts, errSts + } + opts.SummaryTrendStats = trendStats } summaryTimeUnit, err := flags.GetString("summary-time-unit") diff --git a/cmd/run.go b/cmd/run.go index 6245e1c1a0d..3f447cde4f8 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -176,11 +176,6 @@ a commandline interface for interacting with it.`, return ExitCode{cerr, invalidConfigErrorCode} } - // If summary trend stats are defined, update the UI to reflect them - if len(conf.SummaryTrendStats) > 0 { - ui.UpdateTrendColumns(conf.SummaryTrendStats) - } - // Write options back to the runner too. if err = r.SetOptions(conf.Options); err != nil { return err @@ -448,12 +443,15 @@ a commandline interface for interacting with it.`, // Print the end-of-test summary. if !conf.NoSummary.Bool { fprintf(stdout, "\n") - ui.Summarize(stdout, "", ui.SummaryData{ - Opts: conf.Options, - Root: engine.Executor.GetRunner().GetDefaultGroup(), - Metrics: engine.Metrics, - Time: engine.Executor.GetTime(), + + s := ui.NewSummary(conf.SummaryTrendStats) + s.SummarizeMetrics(stdout, "", ui.SummaryData{ + Metrics: engine.Metrics, + RootGroup: engine.Executor.GetRunner().GetDefaultGroup(), + Time: engine.Executor.GetTime(), + TimeUnit: conf.Options.SummaryTimeUnit.String, }) + fprintf(stdout, "\n") } diff --git a/lib/options.go b/lib/options.go index 0e3a1c659b7..f94341a3ff3 100644 --- a/lib/options.go +++ b/lib/options.go @@ -39,6 +39,10 @@ import ( // iterations+vus, or stages) const DefaultSchedulerName = "default" +// DefaultSummaryTrendStats are the default trend columns shown in the test summary output +// nolint: gochecknoglobals +var DefaultSummaryTrendStats = []string{"avg", "min", "med", "max", "p(90)", "p(95)"} + // Describes a TLS version. Serialised to/from JSON as a string, eg. "tls1.2". type TLSVersion int diff --git a/ui/summary.go b/ui/summary.go index 7c6b7b1a4b0..257d4e068a3 100644 --- a/ui/summary.go +++ b/ui/summary.go @@ -21,7 +21,6 @@ package ui import ( - "errors" "fmt" "io" "sort" @@ -31,99 +30,111 @@ import ( "github.com/loadimpact/k6/lib" "github.com/loadimpact/k6/stats" + "github.com/pkg/errors" "golang.org/x/text/unicode/norm" ) const ( - GroupPrefix = "█" - DetailsPrefix = "↳" + groupPrefix = "█" + detailsPrefix = "↳" - SuccMark = "✓" - FailMark = "✗" + succMark = "✓" + failMark = "✗" ) +//nolint: gochecknoglobals var ( - ErrStatEmptyString = errors.New("invalid stat, empty string") - ErrStatUnknownFormat = errors.New("invalid stat, unknown format") - ErrPercentileStatInvalidValue = errors.New("invalid percentile stat value, accepts a number") + errStatEmptyString = errors.New("invalid stat, empty string") + errStatUnknownFormat = errors.New("invalid stat, unknown format") + errPercentileStatInvalidValue = errors.New("invalid percentile stat value, accepts a number") + staticResolvers = map[string]func(s *stats.TrendSink) interface{}{ + "avg": func(s *stats.TrendSink) interface{} { return s.Avg }, + "min": func(s *stats.TrendSink) interface{} { return s.Min }, + "med": func(s *stats.TrendSink) interface{} { return s.Med }, + "max": func(s *stats.TrendSink) interface{} { return s.Max }, + "count": func(s *stats.TrendSink) interface{} { return s.Count }, + } ) -var TrendColumns = []TrendColumn{ - {"avg", func(s *stats.TrendSink) float64 { return s.Avg }}, - {"min", func(s *stats.TrendSink) float64 { return s.Min }}, - {"med", func(s *stats.TrendSink) float64 { return s.Med }}, - {"max", func(s *stats.TrendSink) float64 { return s.Max }}, - {"p(90)", func(s *stats.TrendSink) float64 { return s.P(0.90) }}, - {"p(95)", func(s *stats.TrendSink) float64 { return s.P(0.95) }}, +// ErrInvalidStat represents an invalid trend column stat +type ErrInvalidStat struct { + name string + err error +} + +func (e ErrInvalidStat) Error() string { + return errors.Wrapf(e.err, "'%s'", e.name).Error() } -type TrendColumn struct { - Key string - Get func(s *stats.TrendSink) float64 +// Summary handles test summary output +type Summary struct { + trendColumns []string + trendValueResolvers map[string]func(s *stats.TrendSink) interface{} } -// VerifyTrendColumnStat checks if stat is a valid trend column -func VerifyTrendColumnStat(stat string) error { - if stat == "" { - return ErrStatEmptyString +// NewSummary returns a new Summary instance, used for writing a +// summary/report of the test metrics data. +func NewSummary(cols []string) *Summary { + s := Summary{trendColumns: cols, trendValueResolvers: staticResolvers} + + customResolvers := s.generateCustomTrendValueResolvers(cols) + for name, res := range customResolvers { + s.trendValueResolvers[name] = res } - for _, col := range TrendColumns { - if col.Key == stat { - return nil + return &s +} + +func (s *Summary) generateCustomTrendValueResolvers(cols []string) map[string]func(s *stats.TrendSink) interface{} { + resolvers := make(map[string]func(s *stats.TrendSink) interface{}) + + for _, stat := range cols { + if _, exists := s.trendValueResolvers[stat]; !exists { + percentile, err := validatePercentile(stat) + if err == nil { + resolvers[stat] = func(s *stats.TrendSink) interface{} { return s.P(percentile / 100) } + } } } - _, err := generatePercentileTrendColumn(stat) - return err + return resolvers } -// UpdateTrendColumns updates the default trend columns with user defined ones -func UpdateTrendColumns(stats []string) { - newTrendColumns := make([]TrendColumn, 0, len(stats)) - - for _, stat := range stats { - percentileTrendColumn, err := generatePercentileTrendColumn(stat) +// ValidateSummary checks if passed trend columns are valid for use in +// the summary output. +func ValidateSummary(trendColumns []string) error { + for _, stat := range trendColumns { + if stat == "" { + return ErrInvalidStat{stat, errStatEmptyString} + } - if err == nil { - newTrendColumns = append(newTrendColumns, TrendColumn{stat, percentileTrendColumn}) + if _, exists := staticResolvers[stat]; exists { continue } - for _, col := range TrendColumns { - if col.Key == stat { - newTrendColumns = append(newTrendColumns, col) - break - } + if _, err := validatePercentile(stat); err != nil { + return ErrInvalidStat{stat, err} } } - if len(newTrendColumns) > 0 { - TrendColumns = newTrendColumns - } + return nil } -func generatePercentileTrendColumn(stat string) (func(s *stats.TrendSink) float64, error) { - if stat == "" { - return nil, ErrStatEmptyString - } - +func validatePercentile(stat string) (float64, error) { if !strings.HasPrefix(stat, "p(") || !strings.HasSuffix(stat, ")") { - return nil, ErrStatUnknownFormat + return 0, errStatUnknownFormat } percentile, err := strconv.ParseFloat(stat[2:len(stat)-1], 64) if err != nil { - return nil, ErrPercentileStatInvalidValue + return 0, errPercentileStatInvalidValue } - percentile = percentile / 100 - - return func(s *stats.TrendSink) float64 { return s.P(percentile) }, nil + return percentile, nil } -// Returns the actual width of the string. +// StrWidth returns the actual width of the string. func StrWidth(s string) (n int) { var it norm.Iter it.InitString(norm.NFKD, s) @@ -157,34 +168,26 @@ func StrWidth(s string) (n int) { return } -// SummaryData represents data passed to Summarize. -type SummaryData struct { - Opts lib.Options - Root *lib.Group - Metrics map[string]*stats.Metric - Time time.Duration -} - -func SummarizeCheck(w io.Writer, indent string, check *lib.Check) { - mark := SuccMark +func summarizeCheck(w io.Writer, indent string, check *lib.Check) { + mark := succMark color := SuccColor if check.Fails > 0 { - mark = FailMark + mark = failMark color = FailColor } _, _ = color.Fprintf(w, "%s%s %s\n", indent, mark, check.Name) if check.Fails > 0 { _, _ = color.Fprintf(w, "%s %s %d%% — %s %d / %s %d\n", - indent, DetailsPrefix, + indent, detailsPrefix, int(100*(float64(check.Passes)/float64(check.Fails+check.Passes))), - SuccMark, check.Passes, FailMark, check.Fails, + succMark, check.Passes, failMark, check.Fails, ) } } -func SummarizeGroup(w io.Writer, indent string, group *lib.Group) { +func summarizeGroup(w io.Writer, indent string, group *lib.Group) { if group.Name != "" { - _, _ = fmt.Fprintf(w, "%s%s %s\n\n", indent, GroupPrefix, group.Name) + _, _ = fmt.Fprintf(w, "%s%s %s\n\n", indent, groupPrefix, group.Name) indent = indent + " " } @@ -193,7 +196,7 @@ func SummarizeGroup(w io.Writer, indent string, group *lib.Group) { checkNames = append(checkNames, check.Name) } for _, name := range checkNames { - SummarizeCheck(w, indent, group.Checks[name]) + summarizeCheck(w, indent, group.Checks[name]) } if len(checkNames) > 0 { _, _ = fmt.Fprintf(w, "\n") @@ -204,11 +207,11 @@ func SummarizeGroup(w io.Writer, indent string, group *lib.Group) { groupNames = append(groupNames, grp.Name) } for _, name := range groupNames { - SummarizeGroup(w, indent, group.Groups[name]) + summarizeGroup(w, indent, group.Groups[name]) } } -func NonTrendMetricValueForSum(t time.Duration, timeUnit string, m *stats.Metric) (data string, extra []string) { +func nonTrendMetricValueForSum(t time.Duration, timeUnit string, m *stats.Metric) (data string, extra []string) { switch sink := m.Sink.(type) { case *stats.CounterSink: value := sink.Value @@ -238,21 +241,23 @@ func NonTrendMetricValueForSum(t time.Duration, timeUnit string, m *stats.Metric } } -func DisplayNameForMetric(m *stats.Metric) string { +func displayNameForMetric(m *stats.Metric) string { if m.Sub.Parent != "" { return "{ " + m.Sub.Suffix + " }" } return m.Name } -func IndentForMetric(m *stats.Metric) string { +func indentForMetric(m *stats.Metric) string { if m.Sub.Parent != "" { return " " } return "" } -func SummarizeMetrics(w io.Writer, indent string, t time.Duration, timeUnit string, metrics map[string]*stats.Metric) { +// nolint:funlen +func (s *Summary) summarizeMetrics(w io.Writer, indent string, t time.Duration, + timeUnit string, metrics map[string]*stats.Metric) { names := []string{} nameLenMax := 0 @@ -262,22 +267,32 @@ func SummarizeMetrics(w io.Writer, indent string, t time.Duration, timeUnit stri extraMaxLens := make([]int, 2) trendCols := make(map[string][]string) - trendColMaxLens := make([]int, len(TrendColumns)) + trendColMaxLens := make([]int, len(s.trendColumns)) for name, m := range metrics { names = append(names, name) // When calculating widths for metrics, account for the indentation on submetrics. - displayName := DisplayNameForMetric(m) + IndentForMetric(m) + displayName := displayNameForMetric(m) + indentForMetric(m) if l := StrWidth(displayName); l > nameLenMax { nameLenMax = l } m.Sink.Calc() if sink, ok := m.Sink.(*stats.TrendSink); ok { - cols := make([]string, len(TrendColumns)) - for i, col := range TrendColumns { - value := m.HumanizeValue(col.Get(sink), timeUnit) + cols := make([]string, len(s.trendColumns)) + + for i, tc := range s.trendColumns { + var value string + + resolver := s.trendValueResolvers[tc] + + switch v := resolver(sink).(type) { + case float64: + value = m.HumanizeValue(v, timeUnit) + case uint64: + value = strconv.FormatUint(v, 10) + } if l := StrWidth(value); l > trendColMaxLens[i] { trendColMaxLens[i] = l } @@ -287,7 +302,7 @@ func SummarizeMetrics(w io.Writer, indent string, t time.Duration, timeUnit stri continue } - value, extra := NonTrendMetricValueForSum(t, timeUnit, m) + value, extra := nonTrendMetricValueForSum(t, timeUnit, m) values[name] = value if l := StrWidth(value); l > valueMaxLen { valueMaxLen = l @@ -303,7 +318,8 @@ func SummarizeMetrics(w io.Writer, indent string, t time.Duration, timeUnit stri } sort.Strings(names) - tmpCols := make([]string, len(TrendColumns)) + + tmpCols := make([]string, len(s.trendColumns)) for _, name := range names { m := metrics[name] @@ -311,22 +327,23 @@ func SummarizeMetrics(w io.Writer, indent string, t time.Duration, timeUnit stri markColor := StdColor if m.Tainted.Valid { if m.Tainted.Bool { - mark = FailMark + mark = failMark markColor = FailColor } else { - mark = SuccMark + mark = succMark markColor = SuccColor } } - fmtName := DisplayNameForMetric(m) - fmtIndent := IndentForMetric(m) + fmtName := displayNameForMetric(m) + fmtIndent := indentForMetric(m) fmtName += GrayColor.Sprint(strings.Repeat(".", nameLenMax-StrWidth(fmtName)-StrWidth(fmtIndent)+3) + ":") var fmtData string if cols := trendCols[name]; cols != nil { for i, val := range cols { - tmpCols[i] = TrendColumns[i].Key + "=" + ValueColor.Sprint(val) + strings.Repeat(" ", trendColMaxLens[i]-StrWidth(val)) + tmpCols[i] = s.trendColumns[i] + "=" + ValueColor.Sprint(val) + + strings.Repeat(" ", trendColMaxLens[i]-StrWidth(val)) } fmtData = strings.Join(tmpCols, " ") } else { @@ -350,10 +367,19 @@ func SummarizeMetrics(w io.Writer, indent string, t time.Duration, timeUnit stri } } -// Summarizes a dataset and returns whether the test run was considered a success. -func Summarize(w io.Writer, indent string, data SummaryData) { - if data.Root != nil { - SummarizeGroup(w, indent+" ", data.Root) +// SummaryData represents data passed to Summary.SummarizeMetrics +type SummaryData struct { + Metrics map[string]*stats.Metric + RootGroup *lib.Group + Time time.Duration + TimeUnit string +} + +// SummarizeMetrics creates a summary of provided metrics and writes it to w. +func (s *Summary) SummarizeMetrics(w io.Writer, indent string, data SummaryData) { + if data.RootGroup != nil { + summarizeGroup(w, indent+" ", data.RootGroup) } - SummarizeMetrics(w, indent+" ", data.Time, data.Opts.SummaryTimeUnit.String, data.Metrics) + + s.summarizeMetrics(w, indent+" ", data.Time, data.TimeUnit, data.Metrics) } diff --git a/ui/summary_test.go b/ui/summary_test.go index 43eb5731bee..bdc26cf1b13 100644 --- a/ui/summary_test.go +++ b/ui/summary_test.go @@ -21,33 +21,113 @@ package ui import ( + "bytes" + "fmt" "testing" + "time" + "github.com/loadimpact/k6/lib" "github.com/loadimpact/k6/stats" "github.com/stretchr/testify/assert" + "gopkg.in/guregu/null.v3" ) -var verifyTests = []struct { - in string - out error -}{ - {"avg", nil}, - {"min", nil}, - {"med", nil}, - {"max", nil}, - {"p(0)", nil}, - {"p(90)", nil}, - {"p(95)", nil}, - {"p(99)", nil}, - {"p(99.9)", nil}, - {"p(99.9999)", nil}, - {"nil", ErrStatUnknownFormat}, - {" avg", ErrStatUnknownFormat}, - {"avg ", ErrStatUnknownFormat}, - {"", ErrStatEmptyString}, +func TestSummary(t *testing.T) { + t.Run("SummarizeMetrics", func(t *testing.T) { + var ( + checksOut = " █ child\n\n ✗ check1\n ↳ 33% — ✓ 5 / ✗ 10\n\n" + + " ✓ checks......: 100.00% ✓ 3 ✗ 0 \n" + countOut = " ✗ http_reqs...: 3 3/s\n" + gaugeOut = " vus.........: 1 min=1 max=1\n" + trendOut = " my_trend....: avg=15ms min=10ms med=15ms max=20ms p(90)=19ms " + + "p(95)=19.5ms p(99.9)=19.99ms\n" + ) + + metrics := createTestMetrics() + testCases := []struct { + stats []string + expected string + }{ + {[]string{"avg", "min", "med", "max", "p(90)", "p(95)", "p(99.9)"}, + checksOut + countOut + trendOut + gaugeOut}, + {[]string{"count"}, checksOut + countOut + " my_trend....: count=3\n" + gaugeOut}, + {[]string{"avg", "count"}, checksOut + countOut + " my_trend....: avg=15ms count=3\n" + gaugeOut}, + } + + rootG, _ := lib.NewGroup("", nil) + childG, _ := rootG.Group("child") + check, _ := lib.NewCheck("check1", childG) + check.Passes = 5 + check.Fails = 10 + childG.Checks["check1"] = check + for _, tc := range testCases { + tc := tc + t.Run(fmt.Sprintf("%v", tc.stats), func(t *testing.T) { + var w bytes.Buffer + s := NewSummary(tc.stats) + + s.SummarizeMetrics(&w, " ", SummaryData{ + Metrics: metrics, + RootGroup: rootG, + Time: time.Second, + TimeUnit: "", + }) + assert.Equal(t, tc.expected, w.String()) + }) + } + }) + + t.Run("generateCustomTrendValueResolvers", func(t *testing.T) { + var customResolversTests = []struct { + stats []string + percentile float64 + }{ + {[]string{"p(99)", "p(err)"}, 0.99}, + {[]string{"p(none", "p(99.9)"}, 0.9990000000000001}, + {[]string{"p(none", "p(99.99)"}, 0.9998999999999999}, + {[]string{"p(none", "p(99.999)"}, 0.9999899999999999}, + } + + sink := createTestTrendSink(100) + + for _, tc := range customResolversTests { + tc := tc + t.Run(fmt.Sprintf("%v", tc.stats), func(t *testing.T) { + s := Summary{trendColumns: tc.stats} + res := s.generateCustomTrendValueResolvers(tc.stats) + assert.Len(t, res, 1) + for k := range res { + assert.Equal(t, sink.P(tc.percentile), res[k](sink)) + } + }) + } + }) } -var defaultTrendColumns = TrendColumns +func TestValidateSummary(t *testing.T) { + var validateTests = []struct { + stats []string + expErr error + }{ + {[]string{}, nil}, + {[]string{"avg", "min", "med", "max", "p(0)", "p(99)", "p(99.999)", "count"}, nil}, + {[]string{"avg", "p(err)"}, ErrInvalidStat{"p(err)", errPercentileStatInvalidValue}}, + {[]string{"nil", "p(err)"}, ErrInvalidStat{"nil", errStatUnknownFormat}}, + {[]string{"p90"}, ErrInvalidStat{"p90", errStatUnknownFormat}}, + {[]string{"p(90"}, ErrInvalidStat{"p(90", errStatUnknownFormat}}, + {[]string{" avg"}, ErrInvalidStat{" avg", errStatUnknownFormat}}, + {[]string{"avg "}, ErrInvalidStat{"avg ", errStatUnknownFormat}}, + {[]string{"", "avg "}, ErrInvalidStat{"", errStatEmptyString}}, + } + + for _, tc := range validateTests { + tc := tc + t.Run(fmt.Sprintf("%v", tc.stats), func(t *testing.T) { + err := ValidateSummary(tc.stats) + assert.Equal(t, tc.expErr, err) + }) + } +} func createTestTrendSink(count int) *stats.TrendSink { sink := stats.TrendSink{} @@ -59,102 +139,28 @@ func createTestTrendSink(count int) *stats.TrendSink { return &sink } -func TestVerifyTrendColumnStat(t *testing.T) { - for _, testCase := range verifyTests { - err := VerifyTrendColumnStat(testCase.in) - assert.Equal(t, testCase.out, err) +func createTestMetrics() map[string]*stats.Metric { + metrics := make(map[string]*stats.Metric) + gaugeMetric := stats.New("vus", stats.Gauge) + gaugeMetric.Sink.Add(stats.Sample{Value: 1}) + + countMetric := stats.New("http_reqs", stats.Counter) + countMetric.Tainted = null.BoolFrom(true) + checksMetric := stats.New("checks", stats.Rate) + checksMetric.Tainted = null.BoolFrom(false) + sink := &stats.TrendSink{} + + samples := []float64{10.0, 15.0, 20.0} + for _, s := range samples { + sink.Add(stats.Sample{Value: s}) + checksMetric.Sink.Add(stats.Sample{Value: 1}) + countMetric.Sink.Add(stats.Sample{Value: 1}) } -} - -func TestUpdateTrendColumns(t *testing.T) { - sink := createTestTrendSink(100) - - t.Run("No stats", func(t *testing.T) { - TrendColumns = defaultTrendColumns - - UpdateTrendColumns(make([]string, 0)) - - assert.Equal(t, defaultTrendColumns, TrendColumns) - }) - - t.Run("One stat", func(t *testing.T) { - TrendColumns = defaultTrendColumns - - UpdateTrendColumns([]string{"avg"}) - - assert.Exactly(t, 1, len(TrendColumns)) - assert.Exactly(t, - sink.Avg, - TrendColumns[0].Get(sink)) - }) - - t.Run("Multiple stats", func(t *testing.T) { - TrendColumns = defaultTrendColumns - - UpdateTrendColumns([]string{"med", "max"}) - - assert.Exactly(t, 2, len(TrendColumns)) - assert.Exactly(t, sink.Med, TrendColumns[0].Get(sink)) - assert.Exactly(t, sink.Max, TrendColumns[1].Get(sink)) - }) - - t.Run("Ignore invalid stats", func(t *testing.T) { - TrendColumns = defaultTrendColumns - UpdateTrendColumns([]string{"med", "max", "invalid"}) + metrics["vus"] = gaugeMetric + metrics["http_reqs"] = countMetric + metrics["checks"] = checksMetric + metrics["my_trend"] = &stats.Metric{Name: "my_trend", Type: stats.Trend, Contains: stats.Time, Sink: sink} - assert.Exactly(t, 2, len(TrendColumns)) - assert.Exactly(t, sink.Med, TrendColumns[0].Get(sink)) - assert.Exactly(t, sink.Max, TrendColumns[1].Get(sink)) - }) - - t.Run("Percentile stats", func(t *testing.T) { - TrendColumns = defaultTrendColumns - - UpdateTrendColumns([]string{"p(99.9999)"}) - - assert.Exactly(t, 1, len(TrendColumns)) - assert.Exactly(t, sink.P(0.999999), TrendColumns[0].Get(sink)) - }) -} - -func TestGeneratePercentileTrendColumn(t *testing.T) { - sink := createTestTrendSink(100) - - t.Run("Happy path", func(t *testing.T) { - colFunc, err := generatePercentileTrendColumn("p(99)") - assert.Nil(t, err) - assert.NotNil(t, colFunc) - assert.Exactly(t, sink.P(0.99), colFunc(sink)) - assert.NotEqual(t, sink.P(0.98), colFunc(sink)) - assert.Nil(t, err) - }) - - t.Run("Empty stat", func(t *testing.T) { - colFunc, err := generatePercentileTrendColumn("") - - assert.Nil(t, colFunc) - assert.Exactly(t, err, ErrStatEmptyString) - }) - - t.Run("Invalid format", func(t *testing.T) { - colFunc, err := generatePercentileTrendColumn("p90") - - assert.Nil(t, colFunc) - assert.Exactly(t, err, ErrStatUnknownFormat) - }) - - t.Run("Invalid format 2", func(t *testing.T) { - colFunc, err := generatePercentileTrendColumn("p(90") - - assert.Nil(t, colFunc) - assert.Exactly(t, err, ErrStatUnknownFormat) - }) - - t.Run("Invalid float", func(t *testing.T) { - colFunc, err := generatePercentileTrendColumn("p(a)") - - assert.Nil(t, colFunc) - assert.Exactly(t, err, ErrPercentileStatInvalidValue) - }) + return metrics } From 16a54493880803c76141942e7f6f6e8279d34807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Miri=C4=87?= Date: Wed, 23 Oct 2019 17:03:41 +0200 Subject: [PATCH 2/4] fix(cmd): respect empty SummaryTrendStats value Resolves: https://github.com/loadimpact/k6/commit/b91229aba4c429d8361561513e37f142f7aae18a#r35621359 This changes current behavior on `master`, but as discussed above, it makes sense to respect whatever the user specifies, so this can be considered a fix. --- cmd/config.go | 2 +- cmd/config_consolidation_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/config.go b/cmd/config.go index 019c90c274f..c73f8373269 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -330,7 +330,7 @@ func applyDefault(conf Config) Config { if conf.Options.SystemTags == nil { conf = conf.Apply(Config{Options: lib.Options{SystemTags: &stats.DefaultSystemTagSet}}) } - if len(conf.Options.SummaryTrendStats) == 0 { + if conf.Options.SummaryTrendStats == nil { conf = conf.Apply(Config{Options: lib.Options{SummaryTrendStats: lib.DefaultSummaryTrendStats}}) } return conf diff --git a/cmd/config_consolidation_test.go b/cmd/config_consolidation_test.go index 74db5b5fb26..d90bda431db 100644 --- a/cmd/config_consolidation_test.go +++ b/cmd/config_consolidation_test.go @@ -417,7 +417,7 @@ func getConfigConsolidationTestCases() []configConsolidationTestCase { assert.Equal(t, lib.DefaultSummaryTrendStats, c.Options.SummaryTrendStats) }}, {opts{cli: []string{"--summary-trend-stats", `""`}}, exp{}, func(t *testing.T, c Config) { - assert.Equal(t, lib.DefaultSummaryTrendStats, c.Options.SummaryTrendStats) + assert.Equal(t, []string{}, c.Options.SummaryTrendStats) }}, { opts{runner: &lib.Options{SummaryTrendStats: []string{"avg", "p(90)", "count"}}}, From 26f932a89ed12f8eb440b3d282c567a26c37ad88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Miri=C4=87?= Date: Thu, 24 Oct 2019 14:29:44 +0200 Subject: [PATCH 3/4] fix(ui): return error on out of range percentiles Also add a few missing summary-trend-stats tests. Resolves: https://github.com/loadimpact/k6/pull/1143#issuecomment-545497594 --- cmd/config_consolidation_test.go | 9 ++++++++- ui/summary.go | 7 ++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/cmd/config_consolidation_test.go b/cmd/config_consolidation_test.go index d90bda431db..d733e4afd11 100644 --- a/cmd/config_consolidation_test.go +++ b/cmd/config_consolidation_test.go @@ -416,9 +416,16 @@ func getConfigConsolidationTestCases() []configConsolidationTestCase { {opts{}, exp{}, func(t *testing.T, c Config) { assert.Equal(t, lib.DefaultSummaryTrendStats, c.Options.SummaryTrendStats) }}, - {opts{cli: []string{"--summary-trend-stats", `""`}}, exp{}, func(t *testing.T, c Config) { + {opts{cli: []string{"--summary-trend-stats", ""}}, exp{}, func(t *testing.T, c Config) { assert.Equal(t, []string{}, c.Options.SummaryTrendStats) }}, + {opts{cli: []string{"--summary-trend-stats", "coun"}}, exp{consolidationError: true}, nil}, + {opts{cli: []string{"--summary-trend-stats", "med,avg,p("}}, exp{consolidationError: true}, nil}, + {opts{cli: []string{"--summary-trend-stats", "med,avg,p(-1)"}}, exp{consolidationError: true}, nil}, + {opts{cli: []string{"--summary-trend-stats", "med,avg,p(101)"}}, exp{consolidationError: true}, nil}, + {opts{cli: []string{"--summary-trend-stats", "med,avg,p(99.999)"}}, exp{}, func(t *testing.T, c Config) { + assert.Equal(t, []string{"med", "avg", "p(99.999)"}, c.Options.SummaryTrendStats) + }}, { opts{runner: &lib.Options{SummaryTrendStats: []string{"avg", "p(90)", "count"}}}, exp{}, diff --git a/ui/summary.go b/ui/summary.go index 257d4e068a3..0f70128e990 100644 --- a/ui/summary.go +++ b/ui/summary.go @@ -46,8 +46,9 @@ const ( var ( errStatEmptyString = errors.New("invalid stat, empty string") errStatUnknownFormat = errors.New("invalid stat, unknown format") - errPercentileStatInvalidValue = errors.New("invalid percentile stat value, accepts a number") - staticResolvers = map[string]func(s *stats.TrendSink) interface{}{ + errPercentileStatInvalidValue = errors.New( + "invalid percentile stat value, accepts a number between 0 and 100") + staticResolvers = map[string]func(s *stats.TrendSink) interface{}{ "avg": func(s *stats.TrendSink) interface{} { return s.Avg }, "min": func(s *stats.TrendSink) interface{} { return s.Min }, "med": func(s *stats.TrendSink) interface{} { return s.Med }, @@ -127,7 +128,7 @@ func validatePercentile(stat string) (float64, error) { percentile, err := strconv.ParseFloat(stat[2:len(stat)-1], 64) - if err != nil { + if err != nil || ((0 > percentile) || (percentile > 100)) { return 0, errPercentileStatInvalidValue } From 51e88e03a1cc1222cea666b94189fd220ee408fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Miri=C4=87?= Date: Wed, 30 Oct 2019 10:07:07 +0100 Subject: [PATCH 4/4] refactor(cmd): simplify applyDefault Resolves: https://github.com/loadimpact/k6/pull/1143#discussion_r340462472 --- cmd/config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/config.go b/cmd/config.go index c73f8373269..9f001e37565 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -328,10 +328,10 @@ func getConsolidatedConfig(fs afero.Fs, cliConf Config, runner lib.Runner) (conf // Note that if you add option default value here, also add it in command line argument help text. func applyDefault(conf Config) Config { if conf.Options.SystemTags == nil { - conf = conf.Apply(Config{Options: lib.Options{SystemTags: &stats.DefaultSystemTagSet}}) + conf.Options.SystemTags = &stats.DefaultSystemTagSet } if conf.Options.SummaryTrendStats == nil { - conf = conf.Apply(Config{Options: lib.Options{SummaryTrendStats: lib.DefaultSummaryTrendStats}}) + conf.Options.SummaryTrendStats = lib.DefaultSummaryTrendStats } return conf }