Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove testify/assert from Validator Tests #158

Merged
merged 8 commits into from
Apr 14, 2020
Merged

Remove testify/assert from Validator Tests #158

Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0bee411
ucfg: no assert in TestValidationPass()
alrs Apr 8, 2020
1f50ffa
ucfg: no assert in TestValidationFail()
alrs Apr 8, 2020
ec81b76
ucfg: no assert in TestValidateRequiredFailing()
alrs Apr 8, 2020
51f7b3c
ucfg: no assert in TestValidateNonzeroFailing()
alrs Apr 8, 2020
c489d43
ucfg: no assert in TestValidationFailOnDefaults()
alrs Apr 8, 2020
ef86be1
ucfg: validateErrorTemplate for tests
alrs Apr 10, 2020
58a5451
ucfg: mustUnpack() and mustFailUnpack() in validator tests
alrs Apr 13, 2020
3305326
ucfg: quieter logs in TestValidateRequiredFailing()
alrs Apr 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions validator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/davecgh/go-spew/spew"
)

type myNonzeroInt int
Expand Down Expand Up @@ -64,6 +64,8 @@ var errZeroTest = errors.New("value must not be 0")
var errEmptyTest = errors.New("value must not be empty")
var errMoreTest = errors.New("value must have more than 1 element")

var validateErrorTemplate = "config:%s test:%s error:%v"

func (m myNonzeroInt) Validate() error {
return testZeroErr(int(m))
}
Expand Down Expand Up @@ -310,7 +312,9 @@ func TestValidationPass(t *testing.T) {
for i, test := range tests {
t.Run(fmt.Sprintf("Test config (%v): %#v", i, test), func(t *testing.T) {
err := c.Unpack(test)
assert.NoError(t, err)
if err != nil {
t.Fatalf(validateErrorTemplate, spew.Sdump(c), spew.Sdump(test), err)
}
})
}
}
Expand Down Expand Up @@ -535,7 +539,9 @@ func TestValidationFail(t *testing.T) {
for i, test := range tests {
t.Run(fmt.Sprintf("Test config (%v): %#v", i, test), func(t *testing.T) {
err := c.Unpack(test)
assert.True(t, err != nil)
if err == nil {
t.Fatalf(validateErrorTemplate, spew.Sdump(c), spew.Sdump(test), err)
}
})
}
}
Expand Down Expand Up @@ -632,7 +638,11 @@ func TestValidateRequiredFailing(t *testing.T) {

t.Logf("Unpack returned error: %v", err)
err = err.(Error).Reason()
assert.Equal(t, test.err, err)
logTmpl := "expected:%q got:%q"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we always need to log. Appropos want vs. got, I like to use go-cmp in my own projects. Asserting becomes this then:

if diff := cmp.Diff(want, got); diff != "" {
	t.Errorf("mismatch (-want +got):\n%s", diff)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-error test logging only happens when go test is given the -v option, does that change your concern at all?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We run most tests with -v and the output is quite verbose already. Just noticed that the go-ucfg travis file does not use -c, but I will add it to be consistent with other projects (need to update the travis file because I'm adding go modules support anyways).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

if test.err != err {
t.Fatalf(logTmpl, test.err, err)
}
t.Logf(logTmpl, test.err, err)
})
}
}
Expand Down Expand Up @@ -751,7 +761,11 @@ func TestValidateNonzeroFailing(t *testing.T) {

t.Logf("Unpack returned error: %v", err)
err = err.(Error).Reason()
assert.Equal(t, test.err, err)
logTmpl := "expected:%q got:%q"
if test.err != err {
t.Fatalf(logTmpl, test.err, err)
}
t.Logf(logTmpl, test.err, err)
})
}
}
Expand Down Expand Up @@ -973,7 +987,10 @@ func TestValidationFailOnDefaults(t *testing.T) {
for i, test := range tests {
t.Run(fmt.Sprintf("Test config (%v): %#v", i, test), func(t *testing.T) {
err := c.Unpack(test)
assert.True(t, err != nil)
if err == nil {
t.Fatalf(validateErrorTemplate, spew.Sdump(c), spew.Sdump(test), err)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have this pattern in 3 places now, maybe more? Consider to provide a helper function like this (also guarntees that the message is and keeps unified between tests in the future):

func mustUnpack(t *testing.T, cfg *Config, to interface{}) {
   if err := c.Unpack(to); err != nil {
       ...
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm keeping that in mind, for now I've removed some redundancy with the validateErrorTemplate variable.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the C programmer in me sad. Fortunately Printf is more secure in go. :)

For Printf style function I'd prefer to not use const or vars for 'templates'. Having the format string always with the Printf functions helps linters (e.g. go vet) to detect errors early. This is why I'm prefering a function here (can also be a function for logging/failing only), as this will also fix arguments we accept and force developers to follow up if they do any changes to the format string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this in two functions, mustUnpack() and mustFailUnpack().


})
}
}