dev : implement proper error handling for user input validation

cmd/runner/main.go

@@ -3,7 +3,6 @@ package main
 import (
 	"errors"
 	"flag"
-	"fmt"
 	"log"
 	"os"
 	"time"
@@ -45,8 +44,10 @@ func parseArgs() {
 func main() {
 	parseArgs()
 
-	cfg := makeRunnerConfig()
-	fmt.Println(cfg)
+	cfg, err := makeRunnerConfig()
+	if err != nil {
+		log.Fatal(err)
+	}
 
 	if err := requester.New(cfg).RunAndReport(serverURL); err != nil {
 		log.Fatal(err)
@@ -55,7 +56,7 @@ func main() {
 
 // makeRunnerConfig returns a config.Config initialized with config file
 // options if found, overridden with CLI options.
-func makeRunnerConfig() config.Config {
+func makeRunnerConfig() (config.Config, error) {
 	fileConfig, err := configfile.Parse(configFile)
 	if err != nil && !errors.Is(err, os.ErrNotExist) {
 		// config file is not mandatory, other errors are critical
@@ -64,5 +65,7 @@ func makeRunnerConfig() config.Config {
 
 	cliConfig := config.New(url, requests, concurrency, timeout, globalTimeout)
 
-	return config.Merge(fileConfig, cliConfig)
+	mergedConfig := config.Merge(fileConfig, cliConfig)
+
+	return mergedConfig, mergedConfig.Validate()
 }

config/config.go

@@ -2,7 +2,7 @@ package config
 
 import (
 	"encoding/json"
-	"errors"
+	"fmt"
 	"net/http"
 	"net/url"
 	"time"
@@ -50,10 +50,10 @@ func (cfg Config) HTTPRequest() (*http.Request, error) {
 // is not guaranteed to be safe: it must be validated using Config.Validate
 // before usage.
 func New(uri string, requests, concurrency int, requestTimeout, globalTimeout time.Duration) Config {
-	var urlURL *url.URL
-	if uri != "" {
-		// ignore err: a Config can be invalid at this point
-		urlURL, _ = url.Parse(uri)
+	// ignore err: a Config can be invalid at this point
+	urlURL, _ := url.ParseRequestURI(uri)
+	if urlURL == nil {
+		urlURL = &url.URL{}
 	}
 	return Config{
 		Request: Request{
@@ -68,6 +68,37 @@ func New(uri string, requests, concurrency int, requestTimeout, globalTimeout ti
 	}
 }
 
+// Validate returns the config and a not nil ErrInvalid if any of the fields provided by the user is not valid
+func (cfg Config) Validate() error { //nolint
+	inputErrors := []error{}
+
+	_, err := url.ParseRequestURI(cfg.Request.URL.String())
+	if err != nil {
+		inputErrors = append(inputErrors, fmt.Errorf("-url: %s is not a valid url", cfg.Request.URL.String()))
+	}
+
+	if cfg.RunnerOptions.Requests < 1 && cfg.RunnerOptions.Requests != -1 {
+		inputErrors = append(inputErrors, fmt.Errorf("-requests: must be >= 0, we got %d", cfg.RunnerOptions.Requests))
+	}
+
+	if cfg.RunnerOptions.Concurrency < 1 && cfg.RunnerOptions.Concurrency != -1 {
+		inputErrors = append(inputErrors, fmt.Errorf("-concurrency: must be > 0, we got %d", cfg.RunnerOptions.Concurrency))
+	}
+
+	if cfg.Request.Timeout < 0 {
+		inputErrors = append(inputErrors, fmt.Errorf("-timeout: must be > 0, we got %d", cfg.Request.Timeout))
+	}
+
+	if cfg.RunnerOptions.GlobalTimeout < 0 {
+		inputErrors = append(inputErrors, fmt.Errorf("-globalTimeout: must be > 0, we got %d", cfg.RunnerOptions.GlobalTimeout))
+	}
+
+	if len(inputErrors) > 0 {
+		return &ErrInvalid{inputErrors}
+	}
+	return nil
+}
+
 // Default returns a default config that is safe to use.
 func Default() Config {
 	return defaultConfig
@@ -105,13 +136,3 @@ func Merge(base, override Config) Config {
 func MergeDefault(override Config) Config {
 	return Merge(Default(), override)
 }
-
-// Validate returns an unimplemented error.
-//
-// Once implemented, Validate will return ErrInvalid if any of its fields
-// does not meet the runner requirements.
-//
-// TODO: https://github.com/benchttp/runner/issues/20
-func (cfg Config) Validate() error {
-	return errors.New("unimplemented")
-}

config/config_test.go

@@ -3,12 +3,47 @@ package config_test
 import (
 	"net/url"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/benchttp/runner/config"
 )
 
+func TestConfigValidation(t *testing.T) {
+	t.Run("test valid configuration", func(t *testing.T) {
+		cfg := config.New("https://github.com/benchttp/", 5, 5, 5, 5)
+		err := cfg.Validate()
+		if err != nil {
+			t.Errorf("valid configuration not considered as such")
+		}
+	})
+
+	t.Run("test invalid configuration returns ErrInvalid error with correct messages", func(t *testing.T) {
+		cfg := config.New("github-com/benchttp/", -5, -5, -5, -5)
+		err := cfg.Validate()
+		if err == nil {
+			t.Errorf("invalid configuration considered valid")
+		} else {
+			if !errorContains(err, "-url: "+cfg.Request.URL.String()+" is not a valid url") {
+				t.Errorf("\n- information about invalid url missing from error message")
+			}
+			if !errorContains(err, "-requests: must be >= 0, we got ") {
+				t.Errorf("\n- information about invalid requests number missing from error message")
+			}
+			if !errorContains(err, "-concurrency: must be > 0, we got ") {
+				t.Errorf("\n- information about invalid concurrency number missing from error message")
+			}
+			if !errorContains(err, "-timeout: must be > 0, we got") {
+				t.Errorf("\n- information about invalid timeout missing from error message")
+			}
+			if !errorContains(err, "-globalTimeout: must be > 0, we got ") {
+				t.Errorf("\n- information about invalid globalTimeout missing from error message")
+			}
+		}
+	})
+}
+
 func TestMerge(t *testing.T) {
 	t.Run("do not override with zero values", func(t *testing.T) {
 		cfgBase := newConfig()
@@ -74,7 +109,7 @@ func TestMerge(t *testing.T) {
 
 func TestNew(t *testing.T) {
 	t.Run("zero value params return empty config", func(t *testing.T) {
-		exp := config.Config{}
+		exp := config.Config{Request: config.Request{URL: &url.URL{}}}
 		if got := config.New("", 0, 0, 0, 0); !reflect.DeepEqual(got, exp) {
 			t.Errorf("returned non-zero config:\nexp %#v\ngot %#v", exp, got)
 		}
@@ -83,7 +118,7 @@ func TestNew(t *testing.T) {
 	t.Run("non-zero params return initialized config", func(t *testing.T) {
 		var (
 			rawURL      = "http://example.com"
-			urlURL, _   = url.Parse(rawURL)
+			urlURL, _   = url.ParseRequestURI(rawURL)
 			requests    = 1
 			concurrency = 2
 			reqTimeout  = 3 * time.Second
@@ -130,3 +165,14 @@ func newConfig() config.Config {
 		},
 	}
 }
+
+// To check that the error message is as expected
+func errorContains(err error, expected string) bool {
+	if err == nil {
+		return expected == ""
+	}
+	if expected == "" {
+		return false
+	}
+	return strings.Contains(err.Error(), expected)
+}

config/error.go

@@ -1,8 +1,13 @@
 package config
 
-import (
-	"errors"
-)
+type ErrInvalid struct {
+	invalidValues []error
+}
 
-// ErrInvalid is returned for any invalid Config value.
-var ErrInvalid = errors.New("invalid config")
+func (e *ErrInvalid) Error() string {
+	message := "Invalid value(s) provided:\n"
+	for _, err := range e.invalidValues {
+		message += err.Error() + "\n"
+	}
+	return message
+}

config/file/parse.go

@@ -67,7 +67,7 @@ func parseRawConfig(in rawConfig) (cfg config.Config, err error) {
 // query parameters. It returns the first non-nil error occurring in the
 // process.
 func parseAndBuildURL(raw string, qp map[string]string) (*url.URL, error) {
-	u, err := url.Parse(raw)
+	u, err := url.ParseRequestURI(raw)
 	if err != nil {
 		return nil, err
 	}

config/file/parse_test.go

@@ -62,7 +62,7 @@ func TestParse(t *testing.T) {
 // newExpConfig returns the expected config.Config result after parsing
 // one of the config files in testdataConfigPath.
 func newExpConfig() config.Config {
-	u, _ := url.Parse(testURL)
+	u, _ := url.ParseRequestURI(testURL)
 	return config.Config{
 		Request: config.Request{
 			Method:  "GET",