pact-foundation / pact-net

.NET version of Pact. Enables consumer driven contract testing, providing a mock service and DSL for the consumer project, and interaction playback and verification for the service provider project.
https://pact.io
MIT License
848 stars 234 forks source link

v2 Specification #9

Closed neilcampbell closed 7 years ago

neilcampbell commented 10 years ago

Having this feature will be very useful for us. We are planning on building the regex matching to conform with the v2 spec gist.

kevmee commented 9 years ago

How is this going? Happy to help out.

neilcampbell commented 9 years ago

@kevmee I have done a small PoC, however nothing concrete. I am currently working on finalising the v1 release with getting the test output nice to use. That would be great if you could help out.

kevmee commented 9 years ago

Sweet. Send me over what you have. I'll start looking at the source code today. I'm hoping to roll out PactNet here and this feature would be critical.

neilcampbell commented 9 years ago

Nice!

The details can be found on the branch "feature/regex-response-matching". It's pretty stale, and we haven't fleshed out much, we were just trying to ascertain what the DSL part may look like. It's best to pretty much start again.

I would start with rolling out Pact and seeing if you actually need regex support. You can actually get pretty far with just using the provider state support and inserting deterministic test data (on the provider side). The only reason we haven't built regex support is that we actually don't need it at the moment.

Are you able to share where you are rolling it out?

neilcampbell commented 9 years ago

Some more discussion about spec v2 and regex matching.

https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/pact-dev/KIL2IjeS7RY/MaGi_t7i_gMJ

kevmee commented 9 years ago

I'm working at an insurance company, we're heading down the micro services path so want to get this set up asap.

As you suggested I will be rolling it out, but do see this as a limitation - unless of course I'm missing something that caters for the following scenario:

Consumer -> POST request Service -> returns 201 with {id=XYZ}

I need to know id exists in the body but have no idea what the value will be. I would prefer not to use a provider to insert an expected id as I would not be testing the real API. I would be changing the API behavior when the tests are run, which cheapens the value of the tests.

neilcampbell commented 9 years ago

Yeah the id scenario is a good example.

The only reason you wouldn't know what the id was, is if you could not make the data at the time the test is run, deterministic. Having deterministic state makes integrated testing much easier and way less fragile.

We are not changing the behaviour of the API in this instance, we are making the state the API uses deterministic. This is generally a sign of a good test.

kevmee commented 9 years ago

I understand the benefit of deterministic state when mocking an external service for the sake of an integration test however I'm unsure about altering the internal state of the service that is actually under test, in this case the API.

Anyway I will be doing some work to roll pactnet out over the next week or so. I hope to take a look at this feature at the end of next week.

neilcampbell commented 9 years ago

This is where it gets confusing, the purpose of Pact is to validate the contract between the consumer and provider using a consumer driven approach, not to necessarily provide a testing tool for the provider itself. However as you mention, it is actually testing the provider code to some extent as well. You should still test the provider using normal unit style approaches etc.

The Ruby Pact GitHub wiki is worth checking out as well, as it has more info about these kind of things. https://github.com/realestate-com-au/pact/wiki/FAQ https://github.com/realestate-com-au/pact/wiki/Best-practices

@bethesque What are your thoughts on this? Any specific related doco you can point to?

neilcampbell commented 9 years ago

@kevmee But yeah, I do agree that regex matching is an awesomely useful feature.

bethesque commented 9 years ago

Yes, I agree @neilcampbell, the purpose is to validate the contract, not to be a functional test of the provider, and agree @kevmee that regular expressions are required. An intermediate step is to do "type based" matching, where you can say, this should be a String, this should be an Integer, but I don't care what the value is.

What's the roadmap for types/regexp looking like @neilcampbell?

neilcampbell commented 9 years ago

@bethesque Nothing in the works at the moment, however @kevmee has offered to start implementing it, which is awesome!

bethesque commented 9 years ago

Yay @kevmee! Welcome to team pact. If you haven't already, please read: https://github.com/bethesque/pact-specification/

You're going to need the v2 spec to work from: https://github.com/bethesque/pact-specification/tree/version-2

Specifically, the regular expression specs: https://github.com/bethesque/pact-specification/blob/version-2/testcases/request/body/matches%20with%20regex.json (and the response one)

Be aware that this spec is still in flux! View this as extra incentive to decouple the implementation from the serialisation format ;)

kevmee commented 9 years ago

Thanks @bethesque, for the welcome and the links :)

@neilcampbell I'm hoping to start looking at this next week, so expect to hear a bit more from me then :) As @bethesque mentioned I will look at type based matching as an intermediate step.

Cheers

neilcampbell commented 9 years ago

@kevmee Awesome stuff!

bethesque commented 9 years ago

Here's a type based matching example: https://github.com/bethesque/pact-specification/blob/version-2/testcases/request/body/matches%20with%20type.json

bethesque commented 9 years ago

The idea is that the match type specified for a node is supposed to then carry down for all children, however, that example does not show it well.

neilcampbell commented 9 years ago

@abhayachauhan Just thought I would include you in this conversation about regex matching as I know you guys were also keen to help out with this.

abhayachauhan commented 9 years ago

I've been nagging @neilcampbell about this! Excellent to see some movement on it - we would really love this feature included - but we've worked around it for the moment. I will be looking into this in the coming fortnight as we need it fairly soon!

bethesque commented 9 years ago

@uglyog and I implemented type based matching for arrays on Friday for the JVM and Ruby impls.

kevmee commented 9 years ago

Hey folks, I'll be working on setting up pact with our CI pipeline before getting around to this. Will keep you posted when I get a chance to get started.

neilcampbell commented 9 years ago

@kevmee :+1:

abhayachauhan commented 9 years ago

@neilcampbell @bethesque started working on this (type matching only)

consumer side json updated: https://github.com/abhayachauhan/pact-net/blob/type-matching/Samples/EventApi/Consumer.Tests/pacts/consumer-event_api.json

have I missed anything? on the right track?

bethesque commented 9 years ago

Not quite there. "requestMatchingRules" is now just "matchingRules". "match" : "true" isn't a thing, I think you mean "match" : "type"? Check this example out. https://github.com/bethesque/pact-specification/blob/version-2/testcases/request/body/matches%20with%20type.json. You probably also want to include body[1].event_id and body[2].event_id.

neilcampbell commented 9 years ago

Hey @abhayachauhan! On top of what @bethesque we want to have a DSL on the consumer for generating the matching expressions. @bethesque has a nice gist with what the ruby DSL may look like (https://gist.github.com/bethesque/5a35a3c1cb9fdab6dce7), we could do something similar (however we cant use functions in the same way). We did a small PoC in the feature/regex-response-matching branch (it is way behind and very stale, so only use it as a rough guide) https://github.com/SEEK-Jobs/pact-net/blob/feature/regex-response-matching/Samples/EventApi/Consumer.Tests/EventsApiConsumerTests.cs#L248 (highlighted line). You guys may be able to come up with a nicer approach though.

Also I just noticed that your fork is behind by 44 commits, so please patch your changes on top of the current upstream master. I have done a lot of refactoring whilst getting v1 ready, so it is worth syncing before continuing.

Shout if you need anything else!

abhayachauhan commented 9 years ago

Thanks @neilcampbell and @bethesque I was offline when working so I didn't have your example with me, working from memory. I've realigned my fork and done some refactoring. I have had a good look at your spike and it seems like a great starting point. I've refactored , I'll touch base as I progress.

abhayachauhan commented 9 years ago

@neilcampbell @bethesque I've refactored the consumer side. Neil let me know what you think - it ended up fairly similar to your spike.

Here is the pact - https://gist.github.com/abhayachauhan/68e802898b3d566f8acb

I'm looking at the provider side - I'm starting to see a lot of areas which will need refactoring... especially getting matchers into comparers.

Its all a bit rough at the moment, and will be ironed out once its working - let me know if I've missed anything.

neilcampbell commented 9 years ago

Awesome stuff, looking pretty good! I will take a closer look on the weekend at the branch in your fork.

@bethesque With regards to the spec part, should we be following https://gist.github.com/bethesque/5a35a3c1cb9fdab6dce7 or https://github.com/bethesque/pact-specification/blob/version-2/example.json (I just noticed this one)?

Yeah the provider side is likely where the bulk of the work is. The body comparer is particularly ugly and needs a good refactor as you mention.

abhayachauhan commented 9 years ago

Finished the regex and type matching support end to end. Working well I believe.

Just need to go through and refactor (mainly the provider side as discussed).

Here is an example of the pact: https://gist.github.com/abhayachauhan/70c14395e9f1f8a6d819

Let me know what you think @bethesque @neilcampbell

I've made some assumptions on the type matching side of things. Strings/Ints/Booleans are all straight forward. For example an expected Object matches an actual Object = pass. Properties are not evaluated at all. Same with arrays. We can discuss further if required.

bethesque commented 9 years ago

@neilcampbell Neither example. Those were just pseudo code, thinking on a gist. You should be using the pact-specification version 2 tests https://github.com/bethesque/pact-specification/tree/version-2/testcases. For example, the ruby code has a dynamic test that loops through each test case and checks that it agrees with the 'match' element in the fixture. https://github.com/bethesque/pact-support/blob/master/spec/pact_specification/compliance-2.0.rb

I think you already had one of these tests for version 1?

Btw, the node "responseMatchingRules" should be "matchingRules".

I'm not sure what you mean by "Object matches an actual Object", properties not evaluated @abhayachauhan. I'm don't think I understand what you mean by "properties", maybe it's a .net term. Can you give a JSON example?

abhayachauhan commented 9 years ago

Sounds like I've been working off pseudo code :P Back to the drawing board!

@neilcampbell I'm assuming we don't have any data driven tests which run all these test cases seeing we're not v2 compliant. Do we have any data driven tests like these?

Looking through the tests, it seems to have a lot more functionality than I expected. Need to get my head around some of these...

  1. Matching Rules for request and response (Response I understand, but Request?)
  2. Wildcards in the path (Eg: https://github.com/bethesque/pact-specification/blob/version-2/testcases/request/body/array%20with%20nested%20array%20that%20matches.json) - may need some clarification on how these wildcards work?
  3. Min matching for arrays

Any examples of a v2 pact?

neilcampbell commented 9 years ago

@abhayachauhan Yep we have the specification data driven tests setup and running (if you have the submodule locally). You will just need to update the Git submodule that references the spec repo to point to the version-2 branch on your fork/branch. See https://github.com/SEEK-Jobs/pact-net/tree/master/PactNet.Tests/IntegrationTests/Specification

neilcampbell commented 9 years ago

@abhayachauhan And yep there is a fair number of rules to the matching. Took me a while to get it right :) The readme's on the specification repo are worth have a good read through. If you want to chat through this stuff in a bit more depth, give me a shout.

neilcampbell commented 9 years ago

@bethesque Yep we already have those tests setup. There is a git submodule linked into the pact-net repo.

bethesque commented 9 years ago

@abhayachauhan

  1. Matching Rules for request

Flexible request matching is more useful when Pact is used for higher level tests (eg. driven from the UI). If the matching is exact, the tests are brittle, and the number of interactions you end up needing to verify explodes. Have a read of this: https://github.com/realestate-com-au/pact/wiki/Best-practices#use-pact-for-isolated-unit-tests-think-carefully-about-how-you-use-it-for-integrated-tests

  1. Wildcards in the path

* means "match every element in the array", as distinct from [n] which is, match the nth element.

Any examples of a v2 pact?

The test cases are examples of pacts (or at least, parts of the pact).

bethesque commented 9 years ago

Even the ruby version is not completely v2 compliant, but starting with the regular expressions and type based matching will get you a long way.

abhayachauhan commented 9 years ago

@neilcampbell I've implemented regex/type matching for responses so far. Doesn't handle wildcards yet, nor request matching. Done some refactoring, probably needs another look once all the matching functionality is complete. Could become a very large PR if I try to take the whole thing on. Thoughts?

neilcampbell commented 9 years ago

@abhayachauhan Awesome! Going to hopefully take a look at this today, and will get back to you.

neilcampbell commented 9 years ago

@abhayachauhan I had a look tonight and it's starting to look pretty good. I think it's probably worth teeing up a skype chat/code review session/brainstorm some ideas. Thoughts?

abhayachauhan commented 9 years ago

Yeah it is worth doing that - there are a lot of rough edges. It would be good to chat about it.

I've also started looking at the wildcard matching, I'm finding the JSONPath interpretation seems a little different - having to manipulate JSON paths which is not fun.

Once I check that in (next day or two), I'll drop you a line.

Min matching isn't too far off either.

neilcampbell commented 9 years ago

@abhayachauhan Sounds good, looking forward to the chat.

abhayachauhan commented 9 years ago

@neilcampbell Having some trouble coming up with a nice dsl for wildcards. When are you free to chat? Prefer office hours or after?

neilcampbell commented 9 years ago

@abhayachauhan Sorry we have a hackathon on today and tomorrow, so things are a bit crazy. Are you free on Sat for a chat?

abhayachauhan commented 9 years ago

I'm usually free 8:30pm onwards - after the kids are asleep. Saturday should be fine.

neilcampbell commented 9 years ago

@abhayachauhan Can't do tonight or Sunday night. Let's move this to twitter DMs.

abhayachauhan commented 9 years ago

@neilcampbell Another interesting case that I've encountered with matching -

https://github.com/bethesque/pact-specification/blob/version-2/testcases/response/body/array%20at%20top%20level%20with%20matchers.json

dob field has a regex match - where JsonNet converts to datetime automatically - this will obviously fail. If we switch Datetime parsing off, we lose the ability to do type matching for dates on this. Probably should turn it off and treat it as a string...

neilcampbell commented 9 years ago

@abhayachauhan Yeah I think it should just be treated as a string.

neilcampbell commented 9 years ago

Hey @abhayachauhan. I did a big refactoring of the comparers over the weekend and extracted out the concept of matchers. So far it is looking pretty good.

I also started working on parsing the interaction body to resolve the matchers, then cleansing the body of the matchers (including nested matcher functions). So far so good and will hopefully complete that tonight or tomorrow.

From there we can start building the different types of matchers (which we can probably extract them from the stuff you have been working on), setup the rules for composing them (with weighting/ specificity), start serialising in the pact file and the consumer side is pretty well be sorted.

bethesque commented 9 years ago

Hey guys, don't know if it's just the terms you're using, but you'll find life a whole lot easier down the track if you're working on the concept of a "differ", not a "matcher". The algorithm should take two structures, and return the difference, rather than returning a boolean. This is so that you can then display the differences in a meaningful way to the user. A massive proportion of the usability of the Pact implementations depends on the readability of the diffs.

neilcampbell commented 9 years ago

@bethesque Yep it's the terminology we are using. We are actually returning a list of all paths checked in the object graph and if each check was a pass or fail (which will include the diff). That way we can actually override a pass/fail result from a composed matcher if another result is more specific/has a higher weight. Hopefully that makes sense?