search

pact-foundation / pact-js

JS version of Pact. Pact is a contract testing framework for HTTP APIs and non-HTTP asynchronous messaging systems.
https://pact.io
Other
1.59k stars 343 forks source link

Multiple interactions per test case are no longer supported #1078

Open martinslota opened 1 year ago

martinslota commented 1 year ago

Software versions

Issue Checklist

Please confirm the following:

Expected behaviour

Multiple interactions can be added for a single test case.

Actual behaviour

The pact server crashes when adding the second interaction.

Steps to reproduce

Run npm ci && npm test in https://github.com/martinslota/pact-js-bug-multiple-interactions.

Relevant log files

failure.log

TimothyJones commented 1 year ago

For others coming to this issue, The error message is:

[21:21:51.872] ERROR (13369): pact-core@13.13.6: !!!!!!!!! PACT CRASHED !!!!!!!!!

The pact consumer core returned false at 'cleanupMockServer'. This
should only happen if the core methods were invoked out of order

This is almost certainly a bug in pact-js-core. It would be great if you could
open a bug report at: https://github.com/pact-foundation/pact-js-core/issues
so that we can fix it.

Can you try using the newer DSL instead of the 9.x one? Generally you shouldn't need more than one request per interaction, so there was probably an accidental assumption made that there would only be one when adding the compatibility layer for the older DSL.

The new DSL has less boilerplate - you don't have to call the lifecycle functions.

What are you doing that needs two requests in one interaction?

mefellows commented 1 year ago

Thanks for digging Tim and the helpful repro @martinslota. I agree using the latter interface is recommended, however the idea was to support migrating from 9.x.x so it would be ideal to support it if we can.

martinslota commented 1 year ago

Can you try using the newer DSL instead of the 9.x one? Generally you shouldn't need more than one request per interaction, so there was probably an accidental assumption made that there would only be one when adding the compatibility layer for the older DSL.

I switched to the newer DSL after I hit this problem, though that has then led to other difficulties, especially https://github.com/pact-foundation/pact_broker-client/issues/131 and then some others which might be more on our side (I'm nowhere near done yet, so it's hard to tell).

The impression I got from the documentation was that the compatibility layer in version 10 (and 11, I suppose) would be almost fully backwards compatible with version 9. Not supporting multiple interactions is a deal breaker for the suite I'm migrating. I think that would be worth either fixing or clearly pointing out in the migration instructions.

mefellows commented 1 year ago

Yes, I think it's a bug.

effervescentia commented 1 year ago

Hey @TimothyJones, I have a similar use-case

some of our code will retry the same request depending on the status that is returned (like a 503)

if we only return the failing response in a test then an exception is thrown after a number of attempts which doesn't really test the relationship between the two requests (which I would argue is part of the contract that is imposed on the provider in this case)

I could also see this being useful for testing a function that polls for a status. maybe it starts and then waits for an external job to complete and the job can go through multiple stages before completing

mefellows commented 1 year ago

If this is in fact necessary to test, I would suggest these are tested as two separate scenarios differentiated by provider states (one for when the service is available and one for when it isn't).

The behaviour you want to test (retry mechanism) is actually the client's behaviour, and not the provider's, so I would further argue this isn't appropriate for a Pact test. The provider should not know how often a client attempts to retry a call.

TimothyJones commented 1 year ago

I agree with Matt.

Making some assumptions about your implementation - the retry call wouldn't be visible to the business layer, so this test would take place in a lower level than the rest of the pact tests. If that's the case, I can see why it might feel appropriate to put it all in one interaction - so that the test boundary doesn't move.

However, there's some parts of the behaviour that would be more appropriate for individual tests. For example, the contract test won't include "does the client respect Retry-After", but if you treated them as separate interactions then this would just be part of the normal unit test.

It feels analogous to creating a resource that is immediately modified - where create and modify are separate interactions. Here, being told to retry later and actually retrying later are probably better off as separate interactions.

kroupacz commented 1 year ago

We also have a use-case where we need to add multiple interactions for one test. We are using "pact tests" (@pact-foundation/pact@^9.x.x) for our GraphQL API and it is not unusual for the GraphQL resolver to call multiple REST API endpoints in the background.

mefellows commented 1 year ago

We also have a use-case where we need to add multiple interactions for one test. We are using "pact tests" (@pact-foundation/pact@^9.x.x) for our GraphQL API and it is not unusual for the GraphQL resolver to call multiple REST API endpoints in the background.

I think the general advice is the same here - i.e. stub the additional resolver calls.

mefellows commented 1 year ago

OK so I thought this was suspicious - you can definitely test multiple interactions (I added it to the ID 10 exists scenario, apologies if that adds confusion).

Here is a change to an example project that shows it working: https://github.com/pactflow/example-consumer/compare/feat/repro-pact-js-1078?expand=1

➜  example-consumer git:(master) ✗ npm t

> consumer@0.1.0 test
> cross-env CI=true react-scripts test

  console.log
    Product { id: '10', name: '28 Degrees', type: 'CREDIT_CARD' }

      at src/api.pact.spec.js:73:17

  console.log
    Product { id: 11, name: '28 Degrees', type: 'CREDIT_CARD' }

      at src/api.pact.spec.js:74:17

...

PASS src/api.pact.spec.js
  API Pact test
    retrieving a product
      ✓ ID 10 exists (186 ms)
      ✓ product does not exist (16 ms)
    retrieving products
      ✓ products exists (7 ms)

Test Suites: 1 passed, 1 total
Tests:       3 passed, 3 total
Snapshots:   0 total
Time:        5.125 s, estimated 6 s
Ran all test suites.

The test also fails if one of the calls wasn't made:

FAIL src/api.pact.spec.js (6.217 s)
  API Pact test
    retrieving a product
      ✕ ID 10 exists (200 ms)
      ✓ product does not exist (20 ms)
    retrieving products
      ✓ products exists (15 ms)

  ● API Pact test › retrieving a product › ID 10 exists

    Test failed for the following reasons:

      Mock server failed with the following mismatches:

        0) The following request was expected but not received:
            Method: GET
            Path: /product/11
            Headers:
              Authorization: Bearer 2019-01-14T11:34:18.045Z

      at PactV3.<anonymous> (node_modules/@pact-foundation/src/v3/pact.ts:226:29)
      at step (node_modules/@pact-foundation/pact/src/v3/pact.js:33:23)
      at Object.next (node_modules/@pact-foundation/pact/src/v3/pact.js:14:53)
      at fulfilled (node_modules/@pact-foundation/pact/src/v3/pact.js:5:58)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 2 passed, 3 total
Snapshots:   0 total
Time:        7.736 s
Ran all test suites.
martinslota commented 1 year ago

I believe that the original problem still stands with the original (not PactV3) provider.

mefellows commented 1 year ago

Ah! You're right, my apologies. I'll re-open

gorabin commented 5 months ago

Any update on this ?

mefellows commented 5 months ago

If there was an update, it would appear here ;)

This interface is quite old, so it's not a priority to go back and add this right now. If you are open to submitting a PR to address it, I could give you some pointers.