algocfg: Fix algocfg get for non-string parameters. #3907
Conversation
winder
force-pushed
the
will/fix-algocfg
branch
from
71eb62c to
681bd65
Compare
winder
changed the title
cce
left a comment
cce
left a comment
There was a problem hiding this comment.
LGTM, there is a partitiontest linter warning though
brianolson
left a comment
brianolson
left a comment
There was a problem hiding this comment.
I think the unit test would be more interesting if it incorporated getObjectProperty. Maybe getObjectProperty changes to encompass Sprintf("%v",...) and return the property stringified, then that function is fully unit testable.
cmd/algocfg/getCommand_test.go
Outdated
|
|
||
| func TestPrint(t *testing.T) { | ||
| testcases := []struct { | ||
| input interface{} |
cmd/algocfg/getCommand_test.go
Outdated
| } | ||
| for i, tc := range testcases { | ||
| t.Run(fmt.Sprintf("test %d", i), func(t *testing.T) { | ||
| var buf bytes.Buffer |
There was a problem hiding this comment.
and around here val, err := getObjectProperty(tc, "Input")
Then Fprintf or Sprintf("%v", val)
There was a problem hiding this comment.
Using the test case as an input to the test function is genius.
brianolson
left a comment
brianolson
left a comment
There was a problem hiding this comment.
One last tweak then good to go
Codecov Report
@@ Coverage Diff @@
## master #3907 +/- ##
==========================================
- Coverage 50.09% 49.96% -0.13%
==========================================
Files 394 400 +6
Lines 68456 68618 +162
==========================================
- Hits 34294 34286 -8
- Misses 30465 30630 +165
- Partials 3697 3702 +5
Continue to review full report at Codecov.
|
4 participants
Summary
algocfg was returning values like
%!s(uint64=16)and%!s(bool=true), with this change those are fixed to return16andtrue.Test Plan
Seemed silly to unit test a built in function, but I went ahead and did it anyway.