pactflow / pact-cypress-adapter

Cypress Pact Plugin
MIT License
25 stars 13 forks source link

Add ability to set provider state #22

Closed SimonBachmaier closed 1 year ago

SimonBachmaier commented 2 years ago

Similar to issue #16 we had the need to be able to set the provider state when using this plugin.

This pull request adds functionality to pass the state when calling usePactGet and usePactWait

The state is simply passed as a new parameter:

New

after(() => {
    cy.usePactWait(['getAllUsers'], 'providerState')
})
after(() => {
    cy.usePactGet(['getAllUsers'], 'providerState')
})

Old

after(() => {
    cy.usePactWait(['getAllUsers'])
})
after(() => {
    cy.usePactGet(['getAllUsers'])
})
mefellows commented 1 year ago

Thanks Simon 👏 ! Apologies for the delay in getting back to you here.

At first glance, it seems a reasonable suggestion. There are a couple of questions / comments:

  1. Are you using this with a regular Pact verification? This might help you, but is still discouraged (for example, without matching rules the verification is likely still to be quite brittle)
  2. The definition of a Provider State in later contracts expands on this (see https://github.com/pact-foundation/pact-js/blob/master/src/v3/types.ts#L53). We may want to consider this in the change, and providing an option to serialising a V3 compatible pact file

Other than that, the PR is much appreciated and it looks OK to me. I'll unblock the build to ensure it passes all checks.

@B3nnyL @YOU54F anything to add?

YOU54F commented 1 year ago

Thanks @SimonBachmaier for your first contribution 🚀 I think adding provider state and matchers to cypress-pact is valuable but with caveats, which we haven't distinguished well at the moment. At least anecdotally from feedback https://github.com/pactflow/docs.pactflow.io/issues/125

Few comments on the change

It is probably worth a note on the README that this adapter generates pacts in a v2 specification format, so uses providerState rather than v3's providerStates

This adapter wasn't intended for use with regular CDCT, due to lack of provider states and matchers, (and actually if you are able to test your app with Pact and Cypress to generate Pacts, you can easily test at the unit test level, and get quicker feedback and more granular control)

Re-using the pact contracts, or using shared fixtures between your Integration and Unit tests is a good technique for reusability and sharing of test data.

One of our community champions created a video about it here https://www.youtube.com/watch?v=MtJA90VC9g4&ab_channel=TheTestTribe

YOU54F commented 1 year ago

The definition of a Provider State in later contracts expands on this (see https://github.com/pact-foundation/pact-js/blob/master/src/v3/types.ts#L53). We may want to consider this in the change, and providing an option to serialising a V3 compatible pact file

I'd probably consider that as a separate change

@SimonBachmaier just another note, we use conventions commit messages to generate changelogs

https://github.com/pact-foundation/pact-js/blob/master/CONTRIBUTING.md#release-notes

We could do with adding a contributing section to this repo, but for now you may want to update your commit message for the feature to feat: foo and docs: bar 👌🏾

SimonBachmaier commented 1 year ago

Thank you very much for reviewing the PR. I will add the mentioned changes and then come back to you.

By the sound of it, this adapter is built for a completely different use case than what I initially understood. My team wanted to use the adapter to validate the mocked network calls used in our Cypress tests. (We also already use jest-pact) We wanted to upload the generated contracts to a pact broker and have already implemented a simple solution to add matchers.

If this adapter is meant for something else and doesn't aim to generate contracts that can be used with a pact broker, then we would need a different solution anyway.

mefellows commented 1 year ago

By the sound of it, this adapter is built for a completely different use case than what I initially understood. My team wanted to use the adapter to validate the mocked network calls used in our Cypress tests. (We also already use jest-pact)

Yes, it's primary purpose is for generating pact files, but for use with Pactflow's https://docs.pactflow.io/docs/bi-directional-contract-testing/ feature.

We actually use Cypress and Pact at Pactflow, and we are not using this adapter. We would obviously like to avoid the mock drifts, and we actually achieve this is by reusing the payload definitions from our Pact tests in the cypress route stubs. The approach is summarised here: https://stackoverflow.com/a/68492932/1008568

We wanted to upload the generated contracts to a pact broker and have already implemented a simple solution to add matchers.

Having these two features would certainly improve the reliability of the tests and we would be more than open to accepting a PR that allows this. The key thing is that we would like to preserve the simplicity of the interface for those that just want a simple solution to stub network routes, without having to think about matchers etc.

YOU54F commented 1 year ago

Hey @SimonBachmaier if you want to address the couple of PR comments or comment on them, I am still happy to help move this PR along :)

SimonBachmaier commented 1 year ago

Hey @mefellows and @YOU54F
I'm really sorry that I ghosted this thread for so long! We had a lot of internal discussions about the usage of pact and this adapter in December. Over the holidays it slipped my mind to update this pull request... We decided that we won't combine Pact and Cyperss in the near future. As you pointed out this adapter wasn't intended to use for regular Pact verification and we decided that it would be too much work to adapt this adapter or write our own solution.

Thank you for your great input and sorry again for not getting back to this request earlier.

mefellows commented 1 year ago

Thanks Simon, no worries - if there are any changes let us know. I'll close this off for now but we can resurrect in the future if we decide to add it.