Remove testify/assert from Validator Tests #158
Conversation
validator_test.go
Outdated
| @@ -310,7 +310,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("test:%v error:%v", spew.Sdump(test), err) | |||
There was a problem hiding this comment.
The error would be cause because c and test are "incompatible". Can you try to print c as well? Also curious how the output will look like (e.g. add a failing test).
There was a problem hiding this comment.
validator_test.go
Outdated
| @@ -632,7 +636,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" | |||
There was a problem hiding this comment.
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)
}
There was a problem hiding this comment.
Non-error test logging only happens when go test is given the -v option, does that change your concern at all?
There was a problem hiding this comment.
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).
validator_test.go
Outdated
| assert.True(t, err != nil) | ||
| if err == nil { | ||
| t.Fatalf("test:%v error:%v", spew.Sdump(test), err) | ||
| } |
There was a problem hiding this comment.
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 {
...
}
}
There was a problem hiding this comment.
I'm keeping that in mind, for now I've removed some redundancy with the validateErrorTemplate variable.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I did this in two functions, mustUnpack() and mustFailUnpack().
Codecov Report
@@ Coverage Diff @@
## master #158 +/- ##
==========================================
+ Coverage 78.63% 78.73% +0.10%
==========================================
Files 24 24
Lines 2822 2822
==========================================
+ Hits 2219 2222 +3
+ Misses 440 437 -3
Partials 163 163
Continue to review full report at Codecov.
|
alrs
changed the title
alrs
changed the title
ucfg: quieter logs in TestValidateNonzeroFailing()
|
Thank you. LGTM. Can we merge it as is, or is it still in progress from your point of view? |
This can merge, I removed "WIP" from the title. |
|
Merged. Thank you. |
Hi, @urso.
I've dipped my toe in the water of removing
assert, as mentioned in #156.This removes
testify/assertfrom only one test file in theucfgpackage, and it brings ingo-spewto improve the readability of test failures.If this approach looks good to you, I'm happy to keep going, preferably one entire package per PR.