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

Adding NotEqualsMatcher and generated matcher factory #113

Merged
merged 4 commits into from
Dec 4, 2020

Conversation

yhrn
Copy link

@yhrn yhrn commented Nov 27, 2020

This also adds:

  • a generated matcher factory for any pegomock.Matcher
  • static NotEq matcher factories for "basic" types
  • some missing static matcher factories
  • generated matcher factories will follow the style of go fmt

See #110

This also adds:

- a generated matcher factory for any pegomock.Matcher
- static NotEq matcher factories for "basic" types
- some missing static matcher factories
matcher.go Outdated Show resolved Hide resolved
Copy link
Owner

@petergtz petergtz left a comment

Choose a reason for hiding this comment

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

Okay this is super awesome! Thank you so much.

Small suggestion since you asked for the failure message.

And would you mind adding a couple of tests validating that both the NotEq... matchers and the XThat... matchers actually do what they're supposed to do? Those can go into dsl_test.go.

And I was wondering, do you intend to add the pegomegaMatcher matcher as well in a separate PR, or was your plan to just make that possible, but not add it to the library?

matcher.go Outdated Show resolved Hide resolved
matcher_factories.go Show resolved Hide resolved
matcher_factories.go Show resolved Hide resolved
@yhrn
Copy link
Author

yhrn commented Nov 28, 2020

And would you mind adding a couple of tests validating that both the NotEq... matchers and the XThat... matchers actually do what they're supposed to do? Those can go into dsl_test.go.

Will do

And I was wondering, do you intend to add the pegomegaMatcher matcher as well in a separate PR, or was your plan to just make that possible, but not add it to the library?

My initial intention was not to add that into the library, just to make such a matcher straightforward to use without additional boilerplate for each argument type. I meant it as an example, it didn't really feel like library quality with how the String() method works. But if you would like it in the library then I don't mind adding it. The other consideration here is that it takes a dependency on Gomega but I guess that could be avoided by defining an equivalent interface in Pegomock.

Another thing that I just realized is that I should probably add XThat matcher factories for the "basic" types in matcher.go. But thinking about that made me realize that this could make the other factories kind of obsolete if the exiting built in matchers had differently named constructors. For example all the EqX() factories could be replaced with XThat(IsEqualTo()). What do you think about this direction? Obviously removing existing factories is a breaking change so we wouldn't do that but they could be deprecated.

@petergtz
Copy link
Owner

it didn't really feel like library quality with how the String() method works.

TBH, I haven't had a detailed look at how this would come out as an actual message in pegomock. Need to look into this a bit closer.

But if you would like it in the library then I don't mind adding it.

I like Gomega, so why not make interoperability with it easy. Feel free to do that in a separate PR if you want to get this one out first. Whatever works better for you.

The other consideration here is that it takes a dependency on Gomega but I guess that could be avoided by defining an equivalent interface in Pegomock.

Yes, would definitely define an equivalent interface in Pegomock to avoid the dependency.

For example all the EqX() factories could be replaced with XThat(IsEqualTo()). What do you think about this direction?

I considered this briefly too when you originally proposed the XThat..., and it's certainly more modular. I'm concerned a bit about the simplicity that the EqXs have and which then would go away. Also, XThat(IsEqualTo(...)) is certainly not as succinct anymore, even when you shorten it to XThat(Equals(...)) or XThat(IsEq(...)) or similar.

Oh, and there's one more thing :-). How about documenting the new features in the README? This could also go into a separate PR if you prefer. But it would be a shame if these cool new things stay hidden for most.

@yhrn
Copy link
Author

yhrn commented Nov 30, 2020

Sounds reasonable, I'll add the tests and XThat matcher factories in matcher.go as well as update the README a bit. I'll try to get to that in a day or so. And then we can discuss the PegomegaMatcher in a separate PR.

- Added all the missing matcher factories including "XThat()"
- Improved NotEqMatcher's FailureMessage
- Some tests
- Some README additions
Copy link
Author

@yhrn yhrn left a comment

Choose a reason for hiding this comment

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

I was also considering changing "But other interactions with this mock were" in the verification failure message to something like "Actual interactions with this mock were" because the current sentence sounds strange with NotEq matching.

However, I wasn't sure if that always made sense, e.g. is that always the complete list of interactions on the mock or is it only the unmatched ones? What are your thoughts on that?

matcher_factories.go Show resolved Hide resolved
matcher.go Show resolved Hide resolved
Copy link
Owner

@petergtz petergtz left a comment

Choose a reason for hiding this comment

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

However, I wasn't sure if that always made sense, e.g. is that always the complete list of interactions on the mock or is it only the unmatched ones? What are your thoughts on that?

I think you're actually right.

fail(fmt.Sprintf(
	"Mock invocation count for %v(%v) does not match expectation%v.\n\n\t%v\n\n\t%v",
	methodName, paramsOrMatchers, timeoutInfo, invocationCountMatcher.FailureMessage(), formatInteractions(genericMock.allInteractions())))

suggests that these are always all interactions. So your suggestion, "Actual interactions with this mock were", make sense I think.

So this and the little typo and I think we're good to go :-).

README.md Outdated Show resolved Hide resolved
@petergtz petergtz merged commit b9eeff7 into petergtz:develop Dec 4, 2020
@petergtz
Copy link
Owner

petergtz commented Dec 4, 2020

Super cool! Thank you so much, @yhrn.

@yhrn yhrn deleted the more-gen-matcher branch December 5, 2020 17:08
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.

2 participants