Variadic Method Improvements (Work In Progress - Do Not Merge)

[dlwyatt]

Description

Proposed improvements for mocking variadic methods. Moving the dialogue from #541 over here so further discussion can be made alongside visible code diffs.

Note - In order to cut down on or eliminate the number of breaking changes this code would introduce, I have a PR open in testify here: stretchr/testify#1348 . I don't know if they will take that PR yet. For now, this PR points at my fork of testify to demonstrate how most of the old tests are working with the new generated code.

Lots more details in the linked issue, but the short version here is that this PR aims to get rid of the --unroll-variadic flag, and have mockery give us the best of both worlds from how it works today when that flag is set to either true or false. Namely, that you can use mock's On, AssertCalled, and AssertNotCalled functions without manually building slice values for the variadic parameters, and you can still use a single mock.Anything value against variadic parameters to match any number of arguments.

Assuming testify takes the PR, there is still one breaking change remaining in the existing code; you can't manually build up a slice value for the variadic parameter anymore, which was a syntax still being tested by certain parts of generator_test.go (around line 1500-1600). It may be possible to still allow that syntax in most cases by having the generated mocks look for situations where a single expected argument was passed for a variadic parameter, and that argument is itself a slice. In that case, pass the slice on unmodified to testify. (This might still cause the odd corner case to fail, when someone really does want a slice containing exactly one other slice for some reason. Hopefully there's not too much Go code in the world that looks that way. 😄 )

• Fixes Suggested improvement around variadic functions #541

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Version of Golang used when building/testing:

• 1.11
• 1.12
• 1.13
• 1.14
• 1.15
• 1.16
• 1.17
• 1.18
• 1.19
• 1.20

How Has This Been Tested?

Run make test

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[dlwyatt]

temporary until we see if testify takes the linked PR.

[codecov]

Codecov Report

Base: 72.59% // Head: 73.72% // Increases project coverage by +1.13% 🎉

Coverage data is based on head (e7a31ca) compared to base (e9ed640).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #550      +/-   ##
==========================================
+ Coverage   72.59%   73.72%   +1.13%     
==========================================
  Files           6        6              
  Lines        1277     1332      +55     
==========================================
+ Hits          927      982      +55     
  Misses        306      306              
  Partials       44       44              

Impacted Files	Coverage Δ
cmd/mockery.go	28.57% <100.00%> (ø)
pkg/generator.go	94.63% <100.00%> (+0.43%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[LandonTClipp]

I'll take a look over this and give you my thoughts in a bit

[LandonTClipp]

You already touch on this but I want to get some clarity on this specific question. Take this production code:

func Foo(s ...any) {}

// case 1
Foo("foo", "bar")
// case 2
Foo("foo")

In the tests I may have this:

mockIntf.On("Foo", mock.Anything)

I may either:

1. Want to assert any number of variadic args of any value, or
2. Want to assert a single argument in the variadic parameter of any value

It's not clear to me in the proposal how one would differentiate between those two cases.

In my opinion, it seems like it was a mistake allowing the syntactic sugar because now these cases I mentioned are ambiguous. With unroll-variadic: False and your PR to testify merged, this becomes simple:

// Asserts any number of arguments of any value
mockIntf.On("Foo", mock.Anything)

// Asserts a single argument of any value
mockIntf.On("Foo", []any{mock.Anything})

[dlwyatt]

"now these cases I mentioned are ambiguous"

That's an interesting point. I can see a few possibilities here for matching exactly one argument, assuming the testify PR gets merged:

• To match exactly one argument, use AnythingOfType
• Make the update I mentioned earlier where you can still pass in a slice of expected values, and have that slice contain a single element of mock.Anything
• Add a new placeholder that is specific to Mockery (VariadicAnything or something like that), which conveys the "match any number of arguments" meaning, while mock.Anything remains used for matching a single argument.

[LandonTClipp]

To match exactly one argument, use AnythingOfType

This feels inconsistent and will be a weird edge case.

Make the update I mentioned earlier where you can still pass in a slice of expected values, and have that slice contain a single element of mock.Anything

This is the best option IMO. But then like you mentioned before, you get into a situation where someone might be passing in a slice to the mock implementation as the first argument to the variadic. Kind of weird but... it could happen! If the code has to disambiguate “is this meant to be a single argument or is it the vararg value itself” then it becomes an intractable problem.

Add a new placeholder that is specific to Mockery (VariadicAnything or something like that), which conveys the "match any number of arguments" meaning, while mock.Anything remains used for matching a single argument.

It's an interesting thought. I would rather not start maintaining our own sentinels as it's this weird edge case wart. In my opinion it really seems like the way that solves all of these problems is getting your testify PR merged and making unroll-variadic: False. No ambiguity at all. Some people may be unhappy with the lack of syntactic sugar but as long as we clearly identify this case in the docs, I don't think it would be an issue. Other solutions to me feel like bandaids and open up lots of edge cases we couldn't account for.

[dlwyatt]

Oh, one other option. Make a custom type alias for []any for when you want to be explicit about the slice. If you pass in a single arg of that type, mockery knows to pass it unmodified to testify. Otherwise, roll it up. That covers all the bases (for these examples I'm using VarSlice as the made up type name):

• mock any number of arguments, pass mock.Anything
• mock a single argument only, but don't care about its value, pass in VarSlice{mock.Anything}
• Want to pass in a single argument that happens to be another slice, just pass in your []whatever value. It's not a VarSlice so it'll get wrapped up in a single element []any for testify.

This does still introduce some new usage and could break existing tests, but handles it all in non ambiguous ways without losing the variadic style syntax for most use cases.

[dlwyatt]

Come to think of it, this could be done in a different way that doesn't require a change to testify. Leave the old unroll behavior mostly as is, and just add a new placeholder to mockery for the "match any number of arguments" case. Old tests continue to work fine. Benefit of --unroll-variadic=false is available with the nicer syntax.

[LandonTClipp]

I am inclined to not fix this and make unroll-variadic: True deprecated. I think your ideas of having mockery-specific sentinels would definitely work but from a stylistic point of view, I want to keep our call signatures as close to testify as possible in this situation. I think the cost of having less-nice syntax is marginal when compared to the maintenance burden of having more sentinels and additional logic to handle them.

I think there are merits to the argument either way but my personal preference is to not "fix" this in mockery, and just deprecate unroll-variadic: True.

[LandonTClipp]

@dlwyatt I'm reaching out to the testify maintainers directly to see if we can get your PR merged.