Add ability to set provider state #22

SimonBachmaier · 2022-11-16T13:31:54Z

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 · 2022-11-29T09:37:21Z

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:

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)
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 · 2022-11-29T12:36:00Z

src/index.ts

      usePactRequest: (option: AnyObject, alias: string) => Chainable
-      usePactGet: (alias: string, pactConfig: PactConfigType) => Chainable
+      usePactGet: (alias: string, pactConfig: PactConfigType, providerState?: string) => Chainable


I'm not sure pactConfig should have been here. it isn't shown in the examples and wasn't used in line 39 until this change.

YOU54F · 2022-11-29T12:37:01Z

src/index.ts

      })
    })
  }
 }

 const requestDataMap: AnyObject = {}

-const usePactGet = (alias: string) => {
+const usePactGet = (alias: string, pactConfig: PactConfigType, providerState: string = '') => {


pactConfig is a required field, so this would be a breaking interface change for the user.

Also I don't think it should be included?

YOU54F · 2022-11-29T12:37:48Z

README.md

@@ -72,7 +72,7 @@ before(() => {
    cy.setupPact('ui-consumer', 'api-provider')
 })
 ```
-### cy.usePactWait([alias] | alias)
+### cy.usePactWait([alias] | alias, providerState)


All the examples show providerState as non-optional.

it is optional, and will default to '' as per this change.

YOU54F · 2022-11-29T12:40:34Z

test/utils.test.ts

@@ -106,7 +107,8 @@ describe('constructPactFile', () => {
      pactConfig: {
        consumerName: 'ui-consumer',
        providerName: 'todo-api'
-      }
+      },
+      providerState: 'State'


can we add new test cases to show the additional of state, if set, and defaulting to '' if not set please.

YOU54F · 2022-11-29T12:52:04Z

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 pactflow/docs.pactflow.io#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 · 2022-11-29T13:02:28Z

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 · 2022-12-01T12:55:35Z

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 · 2022-12-02T10:00:48Z

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 · 2022-12-09T15:31:55Z

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 · 2023-02-27T13:59:00Z

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 · 2023-03-03T03:37:27Z

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.

SimonBachmaier added 2 commits November 16, 2022 12:58

add provider state functionality

62b86df

update docs

d5b1d99

mefellows requested a review from B3nnyL November 29, 2022 09:32

YOU54F reviewed Nov 29, 2022

View reviewed changes

YOU54F added enhancement New feature or request help wanted Extra attention is needed labels Dec 9, 2022

mefellows closed this Mar 3, 2023

mefellows mentioned this pull request Mar 6, 2023

Allow Provider State to be set dynamically instead of hard code pactflow/pact-msw-adapter#55

Open

2 tasks

gquemener mentioned this pull request Mar 16, 2023

Remove dead code #33

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to set provider state #22

Add ability to set provider state #22

SimonBachmaier commented Nov 16, 2022

mefellows commented Nov 29, 2022

YOU54F Nov 29, 2022

YOU54F Nov 29, 2022

YOU54F Nov 29, 2022

YOU54F Nov 29, 2022

YOU54F commented Nov 29, 2022 •

edited

YOU54F commented Nov 29, 2022

SimonBachmaier commented Dec 1, 2022 •

edited

mefellows commented Dec 2, 2022

YOU54F commented Dec 9, 2022

SimonBachmaier commented Feb 27, 2023 •

edited

mefellows commented Mar 3, 2023

Add ability to set provider state #22

Add ability to set provider state #22

Conversation

SimonBachmaier commented Nov 16, 2022

New

Old

mefellows commented Nov 29, 2022

YOU54F Nov 29, 2022

Choose a reason for hiding this comment

YOU54F Nov 29, 2022

Choose a reason for hiding this comment

YOU54F Nov 29, 2022

Choose a reason for hiding this comment

YOU54F Nov 29, 2022

Choose a reason for hiding this comment

YOU54F commented Nov 29, 2022 • edited

YOU54F commented Nov 29, 2022

SimonBachmaier commented Dec 1, 2022 • edited

mefellows commented Dec 2, 2022

YOU54F commented Dec 9, 2022

SimonBachmaier commented Feb 27, 2023 • edited

mefellows commented Mar 3, 2023

YOU54F commented Nov 29, 2022 •

edited

SimonBachmaier commented Dec 1, 2022 •

edited

SimonBachmaier commented Feb 27, 2023 •

edited