dev : implement proper error handling for user input validation #26

c1-ra · 2022-01-31T00:54:08Z

Description

Add validation of the user input into the runner CLI in Config.Validate() method and related tests.

codecov-commenter · 2022-01-31T13:14:05Z

Codecov Report

Merging #26 (4f0afa3) into main (e03a1aa) will increase coverage by 4.44%.
The diff coverage is 100.00%.

❗ Current head 4f0afa3 differs from pull request most recent head 79eed3c. Consider uploading reports for the commit 79eed3c to get more accurate results

@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
+ Coverage   74.26%   78.70%   +4.44%     
==========================================
  Files           5        6       +1     
  Lines         136      155      +19     
==========================================
+ Hits          101      122      +21     
+ Misses         24       22       -2     
  Partials       11       11

Flag	Coverage Δ
unittests	`78.70% <100.00%> (+4.44%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
config/config.go	`84.48% <100.00%> (+9.48%)`	⬆️
config/error.go	`100.00% <100.00%> (ø)`
config/file/parse.go	`65.95% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e03a1aa...79eed3c. Read the comment docs.

config/config.go

cmd/runner/main.go

GregoryAlbouy

Nice! Just a couple of changes for me and we're good to go!

GregoryAlbouy · 2022-02-01T10:16:18Z

config/config.go

+	if cfg.RunnerOptions.Requests < 0 {
+		if cfg.RunnerOptions.Requests != -1 {


We can avoid that extra nesting using &&, also I think 0 should be an invalid value for requests:

Suggested change

if cfg.RunnerOptions.Requests < 0 {

if cfg.RunnerOptions.Requests != -1 {

if cfg.RunnerOptions.Requests < 1 && cfg.RunnerOptions.Requests != -1 {

I first used &&, but gocognit was complaining that Validate() was too complex :(
https://github.com/benchttp/runner/runs/5018964117?check_suite_focus=true
It seems less complex to me with && but nesting another condition seemed to pass the checks, so I went with it. Do you have another method in mind that could work to pass the checks too?

It is right though that 0 should also be invalid.

Well if gocognit considers a single && more complex than a nested if then it doesn't do its job correctly 🤷‍♂️
I suggest we shut it down and remove it from .golangci.yml.

I have used //nolint on the method for the moment, if we run into this problem again, we can remove gocognit completely...

config/config.go

* Add `ErrInvalid` to `config` package * Add `Config.Validate()` method * Add call to `Config.Validate()` in `makeRunnerConfig` * Add tests for `Config.Validate()` * Reformat with golangci-lint * Modify `config.New` and related test * Modify `url.Parse(url)` to `url.ParseRequestURI(url)` Co-authored-by: C <>

c1-ra linked an issue Jan 31, 2022 that may be closed by this pull request

Implement proper error handling for config object #20

Closed

c1-ra changed the title ~~Dev/implement proper error handling~~ dev : implement proper error handling Jan 31, 2022

c1-ra force-pushed the dev/implement-proper-error-handling branch 5 times, most recently from e7e704f to 6b882b0 Compare January 31, 2022 13:12

c1-ra force-pushed the dev/implement-proper-error-handling branch from 6b882b0 to 2230d58 Compare January 31, 2022 15:45

c1-ra changed the title ~~dev : implement proper error handling~~ dev : implement proper error handling for user input validation Jan 31, 2022

c1-ra marked this pull request as ready for review January 31, 2022 15:48

C added 7 commits January 31, 2022 16:50

Add ErrInvalid to config package

026106e

Add Config.Validate() method

2ff26c2

Add call to Config.Validate() in main

ea08640

Add tests for Config.Validate()

8e54859

Reformat with golangci-lint

9cb98e1

Modify config.New and related test

f0ebbc7

Modify url.Parse(url) to url.ParseRequestURI(url)

dfc11e0

c1-ra force-pushed the dev/implement-proper-error-handling branch from 2230d58 to dfc11e0 Compare January 31, 2022 15:56

c1-ra requested review from GregoryAlbouy and moreirathomas January 31, 2022 15:58

GregoryAlbouy reviewed Jan 31, 2022

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

config/config.go Outdated Show resolved Hide resolved

cmd/runner/main.go Outdated Show resolved Hide resolved

c1-ra requested a review from GregoryAlbouy February 1, 2022 09:27

Modify config.Validate() to return only an error

67e0465

c1-ra force-pushed the dev/implement-proper-error-handling branch from eb8f096 to c1ffab1 Compare February 1, 2022 09:34

Fix Config.Validate() accepted values

d2cd105

c1-ra force-pushed the dev/implement-proper-error-handling branch from c1ffab1 to d2cd105 Compare February 1, 2022 09:48

GregoryAlbouy reviewed Feb 1, 2022

View reviewed changes

Fix check of invalid values

bba6d94

c1-ra requested a review from GregoryAlbouy February 1, 2022 11:41

c1-ra force-pushed the dev/implement-proper-error-handling branch 9 times, most recently from e6b0150 to bba6d94 Compare February 1, 2022 15:38

C added 3 commits February 1, 2022 16:44

Simplify ErrInvalid

77165b4

Fix Validate check values

8712a5d

Move call to Validate() to makeRunnerConfig

0f7994d

GregoryAlbouy approved these changes Feb 1, 2022

View reviewed changes

moreirathomas approved these changes Feb 2, 2022

View reviewed changes

c1-ra merged commit 9656fd0 into main Feb 3, 2022

moreirathomas deleted the dev/implement-proper-error-handling branch February 3, 2022 10:11

GregoryAlbouy restored the dev/implement-proper-error-handling branch February 3, 2022 17:52

GregoryAlbouy deleted the dev/implement-proper-error-handling branch February 3, 2022 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev : implement proper error handling for user input validation #26

dev : implement proper error handling for user input validation #26

Uh oh!

c1-ra commented Jan 31, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Jan 31, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GregoryAlbouy left a comment

Uh oh!

GregoryAlbouy Feb 1, 2022 •

edited

Loading

Uh oh!

c1-ra Feb 1, 2022 •

edited

Loading

Uh oh!

GregoryAlbouy Feb 1, 2022

Uh oh!

c1-ra Feb 1, 2022

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		if cfg.RunnerOptions.Requests < 0 {
		if cfg.RunnerOptions.Requests != -1 {

	if cfg.RunnerOptions.Requests < 0 {
	if cfg.RunnerOptions.Requests != -1 {
	if cfg.RunnerOptions.Requests < 1 && cfg.RunnerOptions.Requests != -1 {

dev : implement proper error handling for user input validation #26

dev : implement proper error handling for user input validation #26

Uh oh!

Conversation

c1-ra commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

codecov-commenter commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GregoryAlbouy left a comment

Choose a reason for hiding this comment

Uh oh!

GregoryAlbouy Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1-ra Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GregoryAlbouy Feb 1, 2022

Choose a reason for hiding this comment

Uh oh!

c1-ra Feb 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

c1-ra commented Jan 31, 2022 •

edited

Loading

codecov-commenter commented Jan 31, 2022 •

edited

Loading

GregoryAlbouy Feb 1, 2022 •

edited

Loading

c1-ra Feb 1, 2022 •

edited

Loading