Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

Implement Matchers for "loose" variadic argument matching: All, Len, Contains #189

Closed
wants to merge 1 commit into from

Conversation

jinnovation
Copy link

@jinnovation jinnovation commented May 23, 2018

This PR provides a middle-ground approach to recording variadic argument conditions, between simply using gomock.Any() (accepting any list of argument values) or matching against exact argument lists. To that effect, we provide two Matchers to allow for "looser" matching of variadic parameters, including:

  • Contains, which allows you to associate expected mock behavior with variadic argument lists simply containing one or more values; and
  • Len, which likewise allows you to associate with lists of certain length.

This PR also provides an All compound Matcher, which, for example, can be used—in conjunction with both Contains and Len—to match against unordered values of a variadic argument slice.

@jinnovation jinnovation changed the title Implement Matchers for variadic parameters: All, Len, Contains Implement Matchers for "loose" variadic argument matching: All, Len, Contains Jun 12, 2018
Copy link
Collaborator

@poy poy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this and sorry for the horrible delay! I would love to get this in if we could get these changes in.

}

// Len returns a Matcher that matches on length. The returned Matcher returns
// false if its input is not a slice.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add support for chan and map?


func (m lenMatcher) Matches(x interface{}) bool {
return reflect.TypeOf(x).Kind() == reflect.Slice &&
m.n == reflect.ValueOf(x).Cap()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be using .Len() instead of .Cap()?

@@ -122,3 +123,86 @@ func Not(x interface{}) Matcher {
func AssignableToTypeOf(x interface{}) Matcher {
return assignableToTypeOfMatcher{reflect.TypeOf(x)}
}

type compositeMatcher struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we rename this to allMatcher to better match the existing convention?

return containsElementsMatcher{vs}
}

type containsElementsMatcher struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we rename this to containsMatcher to match existing convention?

Elements:
for i := 0; i < len(m.Elements) && eq; i++ {
e := m.Elements[i]
for j := 0; j < xv.Cap(); j++ {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be using Len() instead of Cap()?

xv := reflect.ValueOf(x)

eq := true
Elements:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for readability, WDYTA putting the inner loop in a helper function and removing the label?

continue Elements
}
}
eq = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for readability, return false here and remove the condition above?

@poy poy self-assigned this Sep 27, 2019
@codyoss
Copy link
Member

codyoss commented Oct 31, 2019

@jinnovation are you still interested in this PR?

@codyoss codyoss added the status: needs more info This issue need more information from the author. label Oct 31, 2019
@jinnovation
Copy link
Author

@codyoss: Yes, I am. Apologies about the delay; I should be able to address @poy's feedback in the near future.

@codyoss
Copy link
Member

codyoss commented Nov 1, 2019

Awesome, thanks for your PR!

@codyoss codyoss added status: in progress and removed status: needs more info This issue need more information from the author. labels Nov 1, 2019
@codyoss
Copy link
Member

codyoss commented Nov 20, 2019

@jinnovation Still interested?

@jinnovation
Copy link
Author

@codyoss: Apologies for leading your team on. Despite expectations, I unfortunately haven't had a chance to follow up on this PR. Feel free to close this out; I will re-open if I come around to it.

@codyoss
Copy link
Member

codyoss commented Nov 27, 2019

I might pick this up in that case as I think these features would be nice to have.

@codyoss
Copy link
Member

codyoss commented Dec 20, 2019

I am going to close this for now. But I will probably open up individual PRs for these style of matchers in the future. Thank you for your work on this though, we will try to get a similar feature set in!

@codyoss codyoss closed this Dec 20, 2019
@codyoss codyoss mentioned this pull request Dec 20, 2019
codyoss added a commit that referenced this pull request Dec 20, 2019
This matcher is to be used to combine multiple matches into one
statement. Implmentation inspired by #189.
@codyoss codyoss mentioned this pull request Dec 20, 2019
codyoss added a commit that referenced this pull request Dec 23, 2019
This matcher will check the length with any type that allows this
behavior via reflection. Implementation inspired by #189.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants