From 9e52fc1dc33210bab9f40800751e69685796d528 Mon Sep 17 00:00:00 2001 From: kvisidisi Date: Wed, 16 Jul 2025 16:20:50 +0300 Subject: [PATCH 1/6] test: work in progress for improving test coverage --- internal/config/parse_test.go | 41 +++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/internal/config/parse_test.go b/internal/config/parse_test.go index e1ed8b2..fe74c10 100644 --- a/internal/config/parse_test.go +++ b/internal/config/parse_test.go @@ -124,6 +124,47 @@ metrics: [] wantErr: true, expectedError: "at least one metric must be defined", }, + { + name: "invalid metric name", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "1s" +metrics: + - name: "my-metric" + help: "help" + type: "gauge" + command: "echo 1" +`, + wantErr: true, + expectedError: "metric name is not valid", + }, + { + name: "invalid dynamic label name", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "1s" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" + dynamic_labels: + - name: "1_invalid_label" + field: 0 +`, + wantErr: true, + expectedError: "dynamic_label name: 1_invalid_label is not valid", + }, } for _, tc := range testCases { From 04355121fc9aec26e68e5782f920ec0f6c09ec0e Mon Sep 17 00:00:00 2001 From: kvisidisi Date: Wed, 16 Jul 2025 16:33:57 +0300 Subject: [PATCH 2/6] feat: make global settings optional with defaults --- internal/config/config.go | 8 ++++---- internal/config/parse.go | 15 +++++++++++++++ internal/config/validation.go | 6 +++--- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index e1a5c7e..33eaa22 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,10 +20,10 @@ type Logging struct { } type Global struct { - Timeout time.Duration `yaml:"timeout"` - CacheTTL time.Duration `yaml:"cache_ttl"` - MaxConcurrent int `yaml:"max_concurrent"` - CommandBlacklist []string `yaml:"command_blacklist"` + Timeout time.Duration `yaml:"timeout,omitempty"` + CacheTTL time.Duration `yaml:"cache_ttl,omitempty"` + MaxConcurrent int `yaml:"max_concurrent,omitempty"` + CommandBlacklist []string `yaml:"command_blacklist,omitempty"` } type Metric struct { diff --git a/internal/config/parse.go b/internal/config/parse.go index 14b1bed..19dada4 100644 --- a/internal/config/parse.go +++ b/internal/config/parse.go @@ -4,6 +4,7 @@ import ( "flag" "fmt" "os" + "time" "gopkg.in/yaml.v3" ) @@ -28,6 +29,18 @@ func GetPath() string { return "configs/config.yaml" } +func (c *Config) applyDefaults() { + if c.Global.Timeout == 0 { + c.Global.Timeout = 30 * time.Second + } + if c.Global.CacheTTL == 0 { + c.Global.CacheTTL = 3 * time.Second + } + if c.Global.MaxConcurrent == 0 { + c.Global.MaxConcurrent = 10 + } +} + // Load reads and parses a YAML configuration file into Config struct. // // Returns an error if the file can`t be read or if the YAML is invalid. Panics if cfg is nil. @@ -44,6 +57,8 @@ func Load(path string, cfg *Config) error { return fmt.Errorf("failed to parse YAML from file %s: %w", path, err) } + cfg.applyDefaults() + err = cfg.Validate() if err != nil { return fmt.Errorf("configuration is invalid: %s", err) diff --git a/internal/config/validation.go b/internal/config/validation.go index 29a4022..291a7da 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -77,15 +77,15 @@ func (l *Logging) validate() error { func (g *Global) validate() error { var errs []error - if g.Timeout <= 0 { + if g.Timeout < 0 { errs = append(errs, errors.New("global.timeout must be > 0 (10s)")) } - if g.CacheTTL <= 0 { + if g.CacheTTL < 0 { errs = append(errs, errors.New("global.cache_ttl must be > 0 (5m)")) } - if g.MaxConcurrent <= 0 { + if g.MaxConcurrent < 0 { errs = append(errs, errors.New("global.max_concurrent must be > 0")) } From 6e7896e55337c85461143955285df7635d06d177 Mon Sep 17 00:00:00 2001 From: kvisidisi Date: Wed, 16 Jul 2025 16:44:38 +0300 Subject: [PATCH 3/6] fix(validation): metric, sub-metric names validation --- internal/config/validation.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/config/validation.go b/internal/config/validation.go index 291a7da..1f5cf74 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -97,6 +97,8 @@ func (m *Metric) validate() error { if m.Name == "" { errs = append(errs, errors.New("name is required")) + } else if !metricRegex.MatchString(m.Name) { + errs = append(errs, errors.New("metric name is not valid")) } if m.Help == "" { @@ -149,6 +151,8 @@ func (sm *SubMetric) validate() error { if sm.Name == "" { errs = append(errs, errors.New("name is required")) + } else if !metricRegex.MatchString(sm.Name) { + errs = append(errs, errors.New("sub-metric name is not valid")) } if sm.Help == "" { From 79304e21e67c32ffcebcde54e08c7c78ce0bbae3 Mon Sep 17 00:00:00 2001 From: kvisidisi Date: Wed, 16 Jul 2025 17:16:20 +0300 Subject: [PATCH 4/6] test(config): improved test coverage Added more tests to check for bad values in the config file. --- internal/config/parse_test.go | 255 ++++++++++++++++++++++++++++++++++ 1 file changed, 255 insertions(+) diff --git a/internal/config/parse_test.go b/internal/config/parse_test.go index fe74c10..b8e7488 100644 --- a/internal/config/parse_test.go +++ b/internal/config/parse_test.go @@ -165,6 +165,261 @@ metrics: wantErr: true, expectedError: "dynamic_label name: 1_invalid_label is not valid", }, + { + name: "invalid metrics path", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "metrics" +logging: + level: "info" +global: + timeout: "1s" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" +`, + wantErr: true, + expectedError: "server.metrics_path must start with '/'", + }, + { + name: "invalid logging level", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "warning" +global: + timeout: "1s" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" +`, + wantErr: true, + expectedError: "is not valid. Valid levels: info, debug, error", + }, + { + name: "negative global timeout", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "-5s" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" +`, + wantErr: true, + expectedError: "global.timeout must be > 0", + }, + { + name: "negative global cache_ttl", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "1s" + cache_ttl: "-1m" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" +`, + wantErr: true, + expectedError: "global.cache_ttl must be > 0", + }, + { + name: "negative global max_concurrent", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "1s" + max_concurrent: -5 +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" +`, + wantErr: true, + expectedError: "global.max_concurrent must be > 0", + }, + { + name: "metric with empty command", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "1s" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "" +`, + wantErr: true, + expectedError: "command is required", + }, + { + name: "metric with invalid static label name", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "1s" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" + labels: + "invalid-label": "value" +`, + wantErr: true, + expectedError: "label name invalid-label is not valid", + }, + { + name: "metric with empty dynamic label name", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "1s" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" + dynamic_labels: + - name: "" + field: 0 +`, + wantErr: true, + expectedError: "dynamic_label name is required", + }, + { + name: "sub-metric with invalid name", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "1s" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" + sub_metrics: + - name: "invalid-sub-metric" + help: "help" + type: "gauge" + field: 0 +`, + wantErr: true, + expectedError: "sub-metric name is not valid", + }, + { + name: "sub-metric with empty help", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "1s" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" + sub_metrics: + - name: "my_sub" + help: "" + type: "gauge" + field: 0 +`, + wantErr: true, + expectedError: "help string is required", + }, + { + name: "sub-metric with invalid type", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "1s" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" + sub_metrics: + - name: "my_sub" + help: "a sub metric" + type: "bad_type" + field: 0 +`, + wantErr: true, + expectedError: "type is invalid. valid: gauge, counter", + }, + { + name: "sub-metric with negative field", + yaml: ` +server: + listen_address: ":1234" + metrics_path: "/metrics" +logging: + level: "info" +global: + timeout: "1s" +metrics: + - name: "my_metric" + help: "help" + type: "gauge" + command: "echo 1" + sub_metrics: + - name: "my_sub" + help: "a sub metric" + type: "gauge" + field: -1 +`, + wantErr: true, + expectedError: "field must be >= 0", + }, } for _, tc := range testCases { From 746a06f9e58ba226d161121083d2623c2a6d619f Mon Sep 17 00:00:00 2001 From: kvisidisi Date: Wed, 16 Jul 2025 17:31:24 +0300 Subject: [PATCH 5/6] test(collector): improved test coverage add: - value parsing failure: Checks handling of non-numeric values - field index out of range: Ensures no panic if field index is wrong - sub-metric with non-matching pattern: Verifies match filter --- internal/collector/collector_test.go | 64 ++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/internal/collector/collector_test.go b/internal/collector/collector_test.go index 873e72a..b6eb993 100644 --- a/internal/collector/collector_test.go +++ b/internal/collector/collector_test.go @@ -257,6 +257,70 @@ matched_metric_mem{label_name="label2"} 200 # TYPE connections gauge connections{type="tcp"} 150 connections{type="udp"} 25 +`, + }, + { + name: "value parsing failure", + config: &config.Config{ + Metrics: []config.Metric{ + { + Name: "parse_fail_metric", + Help: "metric that fails to parse.", + Type: "gauge", + Command: "echo 'not_number'", + }, + }, + }, + executor: &mockExecutor{ + output: "not_number", + }, + expectedMetric: ``, + }, + { + name: "field index out of range", + config: &config.Config{ + Metrics: []config.Metric{ + { + Name: "index_metric", + Help: "metric where field index is out of range.", + Type: "gauge", + Command: "echo 'one_value'", + Field: 1, + }, + }, + }, + executor: &mockExecutor{ + output: "one_value", + }, + expectedMetric: ``, + }, + { + name: "sub-metric with no matching pattern", + config: &config.Config{ + Metrics: []config.Metric{ + { + Name: "filter_metric", + Help: "metric with a sub-metric that filters lines.", + Command: "echo -e 'matched_line 100\nunmatched_line 200'", + SubMetrics: []config.SubMetric{ + { + Name: "filtered_sub", + Help: "created for matched lines.", + Type: "gauge", + Field: 1, + Match: "^matched_line", + }, + }, + }, + }, + }, + executor: &mockExecutor{ + output: "matched_line 100\nunmatched_line 200", + }, + expectedMetric: ` +# HELP filter_metric_filtered_sub created for matched lines. +# TYPE filter_metric_filtered_sub gauge +filter_metric_filtered_sub 100 `, }, } From 05a80c400f1b288e78713bb8c6ffc70025e79c40 Mon Sep 17 00:00:00 2001 From: kvisidisi Date: Wed, 16 Jul 2025 17:50:47 +0300 Subject: [PATCH 6/6] test(executor): improved test coverage add: - internal timeout exceeded: Verifies that function's own timeout works --- internal/executor/executor_test.go | 46 +++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/internal/executor/executor_test.go b/internal/executor/executor_test.go index 3720de5..c21f62e 100644 --- a/internal/executor/executor_test.go +++ b/internal/executor/executor_test.go @@ -9,17 +9,20 @@ import ( func TestExecuteCommand(t *testing.T) { testCases := []struct { - name string - command string - timeout time.Duration - wantOutput string - wantErr bool - errContains string + name string + command string + timeout time.Duration + context context.Context + cancelContext bool + wantOutput string + wantErr bool + errContains string }{ { name: "successful execution", command: "echo 'hello world'", timeout: 5 * time.Second, + context: context.Background(), wantOutput: "hello world", wantErr: false, }, @@ -27,26 +30,43 @@ func TestExecuteCommand(t *testing.T) { name: "command fails with stderr", command: "echo 'error message' >&2; exit 1", timeout: 5 * time.Second, + context: context.Background(), wantErr: true, errContains: "error message", }, { - name: "context deadline exceeded", - command: "sleep 2", + name: "context deadline exceeded", + command: "sleep 2", + timeout: 0, + context: func() context.Context { + ctx, _ := context.WithTimeout(context.Background(), 300*time.Millisecond) + return ctx + }(), + cancelContext: false, + wantErr: true, + errContains: "context deadline exceeded", + }, + { + name: "internal timeout exceeded", + command: "sleep 1", timeout: 50 * time.Millisecond, + context: context.Background(), wantErr: true, - errContains: "context deadline exceeded", + errContains: "command timed out", }, } for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), tc.timeout) - defer cancel() + ctx := tc.context + if tc.cancelContext { + var cancel context.CancelFunc + ctx, cancel = context.WithCancel(ctx) + defer cancel() + } executor := &BashExecutor{} - gotOutput, err := executor.ExecuteCommand(ctx, tc.command, 0) + gotOutput, err := executor.ExecuteCommand(ctx, tc.command, tc.timeout) if tc.wantErr { if err == nil {