Suggestions for matchers and matcher generation

[yhrn]

Hi and thanks for a great mocking framework!

I just started using Pegomock and I found a few things related to the experience of using matchers that I would like to discuss. There are three main use cases that I found a bit cumbersome.

1. Matching an argument if it is non-nil - I couldn't find an existing negating or NotEq matcher and if I write one of those I also need to write a matcher function for each type I want to match even if the matcher can handle any type.
2. Matching an argument using a custom matcher - if I have a custom matcher that is applicable for multiple types, e.g. all types implementing some interface, I run into the same inconvenience as described above, i.e. the need to write one matcher function per type matched.
3. Imports in match files are not sorted causing go fmt to modify them (not a big deal but a bit annoying)
4. Matchers that are no longer part of the mocked interfaces are not removed automatically (not a big deal but a bit annoying)
5. I can't seem to find a way to match an arbitrary number of variadic arguments.

I have some suggestions for how this could be addressed:

1. Introduce a new NotEqMatcher and generate a matcher function for it as part of matcher generation
2. Generate a XThatSatisfies(matcher pegomock.Matcher) function as part of matcher generation
3. Format the code after generation? Not sure how easy that is.
4. Maybe generate all matchers in one file.
5. I'd like this to work but I have no idea how to implement it.

What are your thoughts on this?

[petergtz]

Hi @yhrn, thanks for taking the time to open this issue. Let's look at the points one by one:

1. Sounds like a reasonable suggestion. The right place to add this would be: https://github.com/petergtz/pegomock/blob/master/matcher.go https://github.com/petergtz/pegomock/blob/master/mockgen/mockgen.go#L428
2. Not sure if I understand this issue. Can you give an example?
3. Do you have some automatism that formats your generated code? Or where does the go fmt come from that modifies them? I guess it would be possible to run go fmt after generation. I simply never looked at this, because generated code is usually left alone by tools and linters. So before considering a change here and trying to "fix" this, it would interesting to understand where this comes from in your case.
4. Generating all matchers in one file is difficult, because if you have multiple mocks with the same matcher types, you'd end up with multiple functions with the same name in the same package.
5. Hm. Okay, for this one I'll also have to take a deeper look. But it's definitely an issue.

Overall, I think your comments make sense and I'm happy to help. My bandwidth for this project unfortunately is very limited these days, which is why I suggest to people to contribute in form of a PR. I'm happy to review that and merge it. I just can't make the changes myself.

Thanks, Peter

[yhrn]

Hi and thanks for the response!

1. Cool, I should definitely be able to create a PR for this. Just want to discuss the next one a bit more first.
2. The general issue is that if I write my own pegomock.Matcher implementation that is applicable to multiple argument types (or maybe all argument types) I still need to write one "matcher function" for each type I want to match. With the suggested generated method that would not be needed, i.e. I could write the generic code once and then use it everywhere without manually writing any more boilerplate. I'll provide an example below.
3. I guess this came mostly out me being surprised that I ended up with a lot of generated files after running go generate ./... when I just expected a small change to a mock. Running go fmt ./... got rid of that so no big deal but I think this is the first Go code generation tool that I have run into that exhibits this behavior so it stood out a little. We do have some CI automation that will fail builds if go fmt results in any changes but that is not a huge problem because most of the time we remember to format all code before committing anyway.
4. Ok, then this might not be worth doing anything about.
5. :+1:

As for your bandwidth that is very understandable and I'm happy to contribute. 1 & 2, if you think that 2 makes sense after my example below, feels like something that I can easily do. 3 & 4 might not be worth it and 5 is probably a bit too complex for me to find time to take on. I might create a separate issue for that one so that it is tracked cleanly at least.

So the example for 2. I was trying to create an adapter for using Gomega matchers with Pegomock. It turns out that it's not a great fit anyway but it serves as a decent example and there might be some other third party matcher library that would work better. So if I have this code that could live in some reusable "test-util" style package:

// Should returns a Pegomock matcher that delegates to a Gomega matcher.
func Should(gomegaMatcher gomegatypes.GomegaMatcher) pegomock.Matcher {
    return &pegomegaMatcher{gomegaMatcher: gomegaMatcher}
}

// pegomegaMatcher is a suboptimal Gomega Pegomock adapter (with a great name).
type pegomegaMatcher struct {
    gomegaMatcher gomegatypes.GomegaMatcher
    failureMsg    string
    sync.Mutex
}

func (p *pegomegaMatcher) Matches(param pegomock.Param) bool {
    p.Lock()
    defer p.Unlock()
    matches, err := p.gomegaMatcher.Match(param)
    if err != nil {
        p.failureMsg = err.Error()
        return false
    } else if !matches {
        p.failureMsg = p.gomegaMatcher.FailureMessage(param)
        return false
    }
    return true
}

func (p *pegomegaMatcher) FailureMessage() string {
    return p.failureMsg
}

func (p *pegomegaMatcher) String() string {
    // Obviously not great
    return p.gomegaMatcher.FailureMessage("arg")
}

I could then use it like this:

// This struct is not test code but included in the example to make things a bit more obvious
type Request struct {
    MyString string
    MySlice  []string
}

func TestSomething(t *testing.T) {
    Whenever(fake.GetStuff(RequestThat(Should(MatchAllFields(
        Fields{"MyString": ContainSubstring("abcd"), "MySlice": HaveLen(7)}))))).
        ThenReturn("some stuff")

    // ...
}

// It would be nice to not have to write one of these for each argument type 
func RequestThat(matcher pegomock.Matcher) Request {
    pegomock.RegisterMatcher(matcher)
    return Request{}
}

So my suggestion would be to generate the XThat(pegomock.Matcher)/XThatSatisfies(pegomock.Matcher) matcher function.

[petergtz]

Hey @yhrn, sorry for the delay. Thanks a lot for the example for (2). Yes, that makes it a lot easier to understand, and yes, I think that's a good idea. I like it!

So yes, let's go with your proposal to address 1&2, skip 3&4 for now, and move 5 into a separate issue. 👍

Thanks, Peter

[yhrn]

Great, and no stress - it's not like these suggestions are critical. Just wanted to get rid of some more boilerplate. I'll try to get to this later this week.

[petergtz]

BTW, there is now already an issue for the variadic arguments here: https://github.com/petergtz/pegomock/issues/112

I believe this is the same as yours. So no need to open. A separate issue for 5.

[yhrn]

@petergtz would you consider releasing a new version now? At that point I think we could close this one.

We can discuss the Gomega matcher integration separately but I have looked a bit more and I don't think it will work cleanly. Gomega matchers have no description outside of the context of an actual value, only failure messages that compare to an actual value. For pegomock matchers the Stringer description is the important thing, the failure message only seems to be used for invocation count matchers so I can't come up with a generic way of providing a good matcher description, each Gomega matcher type would have to be treated separately.

[yhrn]

Actually, thinking a bit more about custom matchers I feel that this could be simplified further so I created #114. Let me know what you think.

[petergtz]

would you consider releasing a new version now?

We could, yes. It depends how urgently you need this to be on master. If it's not super urgent, I'd like your other PR to be merged as well, and then we can release it as one new version. Otherwise, let me know, and I'll make a new release.

[yhrn]

I agree, I'd rather wait for the other PR to be merged first.

[petergtz]

Just made a new release v2.9.0.

[petergtz]

I'm closing this old issue as I think everything has been addressed. If not, please open a new issue. A lot of things around matchers have changed with the latest version, now that Go has generics.