Skip to content
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

Allow type-safe calls to be used in InOrder #78

Merged
merged 5 commits into from
Sep 13, 2023
Merged

Conversation

EstebanOlmedo
Copy link
Collaborator

@EstebanOlmedo EstebanOlmedo commented Sep 6, 2023

This modifies InOrder to receive variadic any instead of *Call, allowing users to use this function with type-safe generated Calls.

This implementation uses a type assertion to check if the provided arguments are *Calls or reflection to get the *Call when the arguments wrap one (generated code). If neither of the two cases are fullfiled then InOrder panics.

Fix #70.

This modifies `InOrder` to receive variadic `any` instead of `*Call`,
allowing users to use this function with type-safe generated Calls.

This implementation uses a type assertion to check if the provided
arguments are `*Call`s or reflection to get the `*Call` when the
arguments wrap one (generated code). If neither of the two cases are
fullfiled then that argument is ignored.

Fix #70.
@marten-seemann
Copy link
Contributor

If neither of the two cases are fullfiled then that argument is ignored.

Would it make sense to panic? Today it's not possible to pass an invalid call to InOrder. It would be nice if I'd be notified right away if I mess up InOrder, otherwise this will be very hard to debug.

gomock/call.go Outdated Show resolved Hide resolved
@marten-seemann
Copy link
Contributor

Alternatively, what about a type-safe(r) way of doing this: Having InOrder accept an interface { Call() *Call }?

This make `InOrder` to panic when any of its arguments isn't either a
`*Call` or a mock generated type which should already embed `*Call`.
gomock/call.go Outdated Show resolved Hide resolved
This adds to the panic message the index and type of the first
invalid type (if any) passed as parameter to `InOrder`.
marten-seemann added a commit to quic-go/quic-go that referenced this pull request Sep 13, 2023
marten-seemann added a commit to quic-go/quic-go that referenced this pull request Sep 13, 2023
@marten-seemann
Copy link
Contributor

Tested in quic-go in quic-go/quic-go#4057. Seems to work. I also tested that it panic if I pass things other than gomock calls to InOrder.

🚢 it!

@sywhang sywhang merged commit dde4646 into main Sep 13, 2023
2 checks passed
@sywhang sywhang deleted the eolmedo/fix-inorder-typed branch September 13, 2023 17:24
@nkostoulas
Copy link

nkostoulas commented Feb 5, 2024

This breaks InOrder() function calls that pass a []*gomock.Call slice 😞. It is therefore making it very hard for us to upgrade to a later version of this lib. Would it be sensible to fix or provide an alternative function with the previous method definition (and maybe using the new method internally instead)?

I can see it was previously proposed using an interface that implements a particular method instead. That would also be sensible. Perhaps this route could be revisited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

type-safe calls can't be used in gomock.InOrder
5 participants