Enhance defaultHWAddrFunc() and tests to hit 100% coverage #57
Conversation
Codecov Report
@@ Coverage Diff @@
## master #57 +/- ##
========================================
+ Coverage 98.92% 100% +1.07%
========================================
Files 4 4
Lines 279 279
========================================
+ Hits 276 279 +3
+ Misses 2 0 -2
+ Partials 1 0 -1
Continue to review full report at Codecov.
|
theckman
force-pushed
the
100%_coverage
branch
from
a78bf44 to
75abe58
Compare
generator_test.go
Outdated
| @@ -376,6 +378,103 @@ func testNewV5DifferentNamespaces(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func Test_defaultHWAddrFunc(t *testing.T) { | |||
There was a problem hiding this comment.
I don't like this underscore very much, because no other top level tests have one. Indeed, TestdefaultHWAttrFunc does not look nice, but I wouldn't mind TestDefaultHWAddrFunc, even though the token after Test does not match the function being tested exactly.
I don't feel strongly about this at all, so we can leave it as-is if you'd like.
There was a problem hiding this comment.
Agree with @acln0 , Let's have TestDefaultHWAddrFunc
| @@ -284,9 +284,11 @@ func newFromHash(h hash.Hash, ns UUID, name string) UUID { | |||
| return u | |||
| } | |||
|
|
|||
| var netInterfaces = net.Interfaces | |||
There was a problem hiding this comment.
At the call site for netInterfaces, we use ifaces, err := netInterfaces() and rely on type inference. I would prefer adding an explicit type here, in the style of these hooks: https://golang.org/pkg/internal/poll/#pkg-variables, to better communicate the signature of what is being hooked, so the reader doesn't have to go through yet another indirection.
Perhaps var netInterfaces func() ([]net.Interface, error) = net.Interfaces.
There was a problem hiding this comment.
I don't really like how it clutters up the code. It would be easier for my editor to give me the function header definition over trying to read that line.
I was trying to hit 100% without making the code too messy.
There was a problem hiding this comment.
I want to push back a little here. I like having the type inline, be it a little bit more cluttered as it may. Editors might help alleviate the extra indirection via highlighting, but I'd like to avoid it altogether.
There was a problem hiding this comment.
Finally getting back around to working on this PR, and this change results in a golint failure.
generator.go:287:19: should omit type func() ([]net.Interface, error) from declaration of var netInterfaces; it will be inferred from the right-hand side (golint)
var netInterfaces func() ([]net.Interface, error) = net.Interfaces
^
As a result I'm just going to omit it at this time.
|
Outside of @acln0's comments this looks good |
|
Sorry for the delay, October was busy for me. Let me do some homework on this one. |
This updates `defaultHWAddrFunc` to use a package-level variable for calling `net.Interfaces()`, so that we can mock it in unit tests. Doing so, and writing the related test, allows us to hit 100% coverage. Signed-off-by: Tim Heckman <[email protected]>
theckman
force-pushed
the
100%_coverage
branch
from
75abe58 to
8b46831
Compare
theckman
dismissed
acln0’s stale review
Changes were made since then; and some of the requested changes are flagged by golint
generator_test.go
Outdated
| @@ -376,6 +378,103 @@ func testNewV5DifferentNamespaces(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func Test_defaultHWAddrFunc(t *testing.T) { | |||
There was a problem hiding this comment.
| func Test_defaultHWAddrFunc(t *testing.T) { | |
| func TestDefaultHWAddrFunc(t *testing.T) { |
Codecov Report
@@ Coverage Diff @@
## master #57 +/- ##
===========================================
+ Coverage 99.02% 100.00% +0.97%
===========================================
Files 4 4
Lines 411 411
===========================================
+ Hits 407 411 +4
+ Misses 3 0 -3
+ Partials 1 0 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This updates
defaultHWAddrFuncto use a package-level variable for callingnet.Interfaces(), so that we can mock it in unit tests.Doing so, and writing the related test, allows us to hit 100% coverage.
Signed-off-by: Tim Heckman [email protected]