-
Notifications
You must be signed in to change notification settings - Fork 401
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.
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
Add option to omit empty/nil variadic slice when unroll-variadic
is false
#624
Conversation
@@ -75,6 +75,7 @@ func NewRootCmd() *cobra.Command { | |||
pFlags.Bool("disable-version-string", false, "Do not insert the version string into the generated mock file.") | |||
pFlags.String("boilerplate-file", "", "File to read a boilerplate text from. Text should be a go block comment, i.e. /* ... */") | |||
pFlags.Bool("unroll-variadic", true, "For functions with variadic arguments, do not unroll the arguments into the underlying testify call. Instead, pass variadic slice as-is.") | |||
pFlags.Bool("omit-empty-rolled-variadic", false, "For functions with variadic arguments, omit passing empty slice to underlying testify call. Used in tandem with unroll-variadic set to false.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super happy with the naming here.
Ideally unroll-variadic
would be called roll-variadic
and defaulted to false. We could then call this new option roll-variadic-omit-empty
which feels better.
Curious - how do we feel about deprecating unroll-variadic
in place of roll-variadic
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecating the variable name is probably a lot of effort. Technically speaking, "unroll" makes more sense as it is "expanding"/"unrolling" the slice into the variadic mock.On(
testify call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A part of me also feels like, if we do add this feature, it doesn’t need to be behind a feature flag. I can’t think of a case where this would break someone relying on the current logic?
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #624 +/- ##
===================================================
+ Coverage 76.20159% 76.32312% +0.12153%
===================================================
Files 7 7
Lines 2143 2154 +11
===================================================
+ Hits 1633 1644 +11
Misses 374 374
Partials 136 136
☔ View full report in Codecov by Sentry. |
unroll-variadic
is falseunroll-variadic
is false
Thanks for the PR, I'll take a look soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider if we have:
type FuncOpt func(c *ConfigStore)
type Fooer interface {
Foo(values ...FuncOpt) error
}
When the mock object is called, we get the expected return value from testify like this (unroll-variadic: false
):
ret := _m.Called(values)
if unroll-variadic: true
:
_va := make([]interface{}, len(values))
for _i := range values {
_va[_i] = values[_i]
}
var _ca []interface{}
_ca = append(_ca, _va...)
ret := _m.Called(_ca...)
To make sure I understand the problem... one might register a call expectation like:
mockFoo.On("Foo").Return(nil)
so testify in this case would not be able to match against that expectation because the mock object calls mock.Called
with an empty variadic slice, where none was specified in the expectation. But presumably, this would work:
mockFoo.On("Foo", []FuncOpts{}).Return(nil)
It also seems if we used the Expecter structs as such, it would work:
mockFoo.EXPECT().Foo().Return(nil)
But I do see that if we try to generate an expecter struct with unroll-variadic: false
, we get an error:
Error: cannot generate a valid expecter for variadic method with unroll-variadic=false
However I don't think there's a good reason this has been disabled, I think it just hasn't been implemented yet. Theoretically, if we allow expecter structs with unroll-variadic: false
, the expectation will pass an empty slice to testify like: mock.On("Foo", values)
So, it seems to me that the real solution is to get the expecter structs to support this. That way, both the expectation, and the call assertion, use empty slices. We would still have the problem that using the raw mockFoo.On("Foo")
call would necessitate specifying an empty slice, but this is fixable by:
- Recommending to use the expecter structs (which is just better anyway) and
- Noting this clearly in the docs, if people do elect to not use expecter structs
My preference is to fix the expecter structs to support unroll-variadic: false
. That way, folks won't have to hack the config for this one really specific case. Please let me know if any of my understanding is flawed here.
@@ -1566,6 +1566,34 @@ func (s *GeneratorSuite) TestGeneratorVariadicArgsAsOneArg() { | |||
) | |||
} | |||
|
|||
func (s *GeneratorSuite) TestGeneratorVariadicArgsOmitEmpty() { | |||
expectedBytes, err := os.ReadFile(getMocksPath("RequesterVariadicOmitEmpty.go")) | |||
s.NoError(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be require.NoError
OmitEmptyRolledVariadic: true, | ||
}, s.getInterfaceFromFile("requester_variadic.go", "RequesterVariadic"), pkg, | ||
) | ||
s.NoError(generator.Generate(s.ctx), "The generator ran without errors.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message to NoError
is what is displayed if there is an error. You probably don't need any message though.
|
||
var actual []byte | ||
actual, fmtErr := format.Source(generator.buf.Bytes()) | ||
s.NoError(fmtErr, "The formatter ran without errors.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@LandonTClipp thank you for the detailed review. I was unaware of the Expecter Structs but after reading I agree time is better spent investing in them over introducing this layered config hack. |
@dillonstreator I'm open to fixing this in a variety of ways. Ultimately, mockery needs to handle empty variadic args in a consistent manner (either by always omitting empty varargs, or always passing it). Your problem comes from the fact that it's just inconsistent right now. I'll close this but feel free to submit another PR that implements something like I suggested. Thanks for the feedback! |
Description
Currently, empty variadic options are passed through to testify as a nil slice when
unroll-variadic
is set to false and no variadic arguments are supplied. With the introduction of functional options testing support being added to testify stretchr/testify#1023, we must haveunroll-variadic
set to false given the expectations of the mocking mechanism. DX would be better if the nil slice could be omitted.This PR introduces a new option
omit-empty-rolled-variadic
which would support that use case.Type of change
Version of Golang used when building/testing:
How Has This Been Tested?
Introduced a new test for the new functionality. No existing tests impacted.
Checklist