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

Contract with matchers #19

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mmadariaga
Copy link

Added usePactIntercept comand that allows to intercept network calls and record response and matching rules into a pact file

@YOU54F
Copy link
Member

YOU54F commented Oct 28, 2022

Oooooh this looks epic!

Can't wait to get some time to check this out.

Just had a quick glance, if we support v2 or v3 matchers, we will probably want to ensure the user only provides one set, and that the output pact specification value matches up with the used matchers

Kicked off the build in the mean time

@YOU54F
Copy link
Member

YOU54F commented Oct 28, 2022

Could we actually use matchers from pact-js to setup the response body, and use the reify method to strip the matchers from the response body used in cy.intercept

@mefellows
Copy link
Contributor

Thanks @mmadariaga! I think it's a good suggestion, but it should be used with caution. Because the matching rules are being setup automatically, we'll have to make sensible suggestions in how they are applied.

In the simplest form, you just need a:

"$.body": {
  "match": "type"
},

And this will recursively apply the type matcher to the body. It gets tricky with arrays and things where you might want to actually apply an eachLike matcher (an array of len >= 1, with all objects conforming to the shape of the supplied object), but this is probably what most people expect.

So I don't think you need a whole extra method to catch this, and could just make it a serialisation option (or a request by request parameter).

@mmadariaga
Copy link
Author

Hi @mefellows ,
This is my first time using both, cypress and pact, and I think I lack the knowledge to properly understand your points.
I hope at least that this PR will encourage someone to implement the feature in a better way.

@dbro04
Copy link

dbro04 commented Nov 16, 2022

I think it would be better to add a set of matching rules to the PactConfigtype and use these rules in the response. In costructInteraction you can check if a rule exist for the property of the response.body. Therefor a rule consists of

  • a name (the property in json, which could be a simple type or an array)
  • the matching rule for example match type or match MinMaxType or any Pact matching Rule

There is no further need for nested types, because they could be handled in the parent type.

@YOU54F YOU54F self-requested a review December 9, 2022 15:36
@YOU54F YOU54F added enhancement New feature or request help wanted Extra attention is needed labels Dec 9, 2022
@YOU54F
Copy link
Member

YOU54F commented Dec 9, 2022

Hey @dbro04 would you be up for collabbing on the PR?

I still think using the PactJS matchers, and reifying the payload before its serialised to a pact might be a sensible idea. We can probably lift parts of the code rather than pulling in the project as a dep

Adding some help wanted labels to try and attract extra eyes

@ishaqibrahimbot
Copy link

Hey @YOU54F! I just started using the pact-cypress-adapter for a work project and realized the adapter doesn't currently support matchers. And then I found this PR.

It's quite important for my use case to make use of matchers - I want to use type-based matching and use stuff like eachLike for arrays and so on.

I'm open to collaborating with you on this PR. Here is my understanding of the approach you're suggesting:

  • Use the matchers on the response body inside cy.intercept
  • Use these to generate the pact file with the matchingRules
  • Reify the body for the network stubbing part of cypress

Let me know if I've understood this correctly. If so, I'll start working on this via a fresh fork + PR.

@YOU54F
Copy link
Member

YOU54F commented Nov 14, 2023

👋🏾 Howdy!

Something like that, I think my main concern would be allowing the user to use pact-js style matchers, rather than having to express json path notation. A pact-js user wouldn't look inside the pact file regularly, so their experience would be with the v2/v3 matchers interface. We should take should of serialising them correctly to the pact file.

We would need to consider the pact specification that gets written to the file, currently it assumes a v2 spec. if we cater for matching rules for both spec versions, then we want to ensure only one format is used, and the correct specification is also serialised.

There are typed schemas for each of the pact specs that may be useful for validation.

https://github.com/pactflow/pact-schemas

whats the use case where you need to use matchers with this adapter. Are you unable to use pact-js in your project? Our general advise if is you are not comparing these contracts against an openapi specification whereby the schema matching rules are applied by the tool (swagger-mock-validator), is to generate the contracts at the lowest layer (your api client) and reuse those pact contracts, as stubs for higher level tests (including those run with cypress).

We use that pattern within PactFlow between its UI/backend app.

@ishaqibrahimbot
Copy link

Thanks for the quick reply @YOU54F!

Let me describe my use case: I'm working on a nextjs project for which we're using cypress to write all the component, integration, and e2e tests. As part of the project's scope, we are also required to set up contract tests via pact and pactflow to ensure no breaking changes are published by the provider (our backend).

Our solution architect suggested we use the cypress-pact plugin to reduce the maintenance burden since the idea was that generating contracts and using them would become a lot more seamless through the plugin. Recently while researching into this I found that the plugin was archived long ago and this adapter was published to work with cypress.

So I've configured the adapter in my project and have written a sample test using it that generates a pact file. My only issue now is that because of the lack of support of matchers, the broker checks the type and the value of a given property inside the response body instead of just making sure the property is of the right type. Only when I manually add matchingRules to the generated pact file does it work as I want.

So either you can give me some advice on what I should do for my use case, or we can somehow work on adding the matchers feature to the adapter.

Let me know if you need more context.

@mefellows
Copy link
Contributor

#19 (comment)

This comment sums it up well. You will probably also want to implement provider states, because these are crucial to data preparation on the provider side.

Yousaf's comments are worth heeding also. One of the problems with using this adapter for pure Pact testing (and not BDCT for which it is intended) is that because you're testing at a higher level, you may end up with more tests than strictly necessary, further burdening the provider. See https://pactflow.io/blog/a-disastrous-tale-of-ui-testing-with-pact/ for more on that topic.

@ishaqibrahimbot
Copy link

Thanks @mefellows, I'll start working on this feature over the weekend.

Regarding Yousaf's comments, I did previously read the article you've linked and understand the pitfalls of overloading the provider with extra and to-some-extent duplicate requests. We're going to restrict the number of pacts we generate in our tests and try to keep them as different as possible. Thanks for the heads up!

@mefellows
Copy link
Contributor

No worries - thanks and good luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants