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

Pact Consumer tests are failing starting with node 18.10 and 19 #1066

Closed RaresAil closed 2 months ago

RaresAil commented 1 year ago

Software versions

Issue Checklist

Please confirm the following:

Expected behaviour

All the tests to pass

Actual behaviour

The tests fail after upgrading from node 18.8.0 to 18.10.0 even on 19.x

Steps to reproduce

Update to node 18.10 or 19 and run the Consumer tests

Relevant log files

● Console

    console.error

      at Pact.Object.<anonymous>.Pact.verify (../node_modules/@pact-foundation/src/httpPact/index.ts:211:15)
      at Object.<anonymous> (../node_modules/jest-pact/dist/pactWith.js:12:45)

    console.error
      Pact verification failed!

      at Pact.Object.<anonymous>.Pact.verify (../node_modules/@pact-foundation/src/httpPact/index.ts:212:15)
      at Object.<anonymous> (../node_modules/jest-pact/dist/pactWith.js:12:45)

    console.error
      Test failed for the following reasons:

        Mock server failed with the following mismatches:

        0) The following request was expected but not received: 
            Method: POST
            Path: ...
            Headers:
              Accept: application/json
              Authorization: Bearer mock
              Content-Type: application/json
            Body: ...

      at Pact.Object.<anonymous>.Pact.verify (../node_modules/@pact-foundation/src/httpPact/index.ts:213:15)
      at Object.<anonymous> (../node_modules/jest-pact/dist/pactWith.js:12:45)

  ● Pact between .... returns the correct response

    socket hang up

      at Function.Object.<anonymous>.AxiosError.from (../node_modules/axios/lib/core/AxiosError.js:89:14)
      at RedirectableRequest.handleRequestError (../node_modules/axios/lib/adapters/http.js:533:25)
      at ClientRequest.eventHandlers.<computed> (../node_modules/follow-redirects/index.js:14:24)

  ● Pact between ... returns the correct response

    Pact verification failed - expected interactions did not match actual.

      at new VerificationError (../node_modules/@pact-foundation/pact/src/errors/verificationError.js:21:42)
      at Pact.Object.<anonymous>.Pact.verify (../node_modules/@pact-foundation/src/httpPact/index.ts:217:13)
      at Object.<anonymous> (../node_modules/jest-pact/dist/pactWith.js:12:45)

Failed to initialise global tracing subscriber - a global default trace dispatcher has already been set
TimothyJones commented 1 year ago

Are you giving the mock server an IP address or just localhost? I know that node 18 uses ipv6 by default, which can cause some confusion around what “localhost” means when communicating externally

RaresAil commented 1 year ago

I get the base url like this:

instance.defaults.baseURL = provider.mockService.baseUrl;

EDIT: The value is 127.0.0.1:port

RaresAil commented 1 year ago

@TimothyJones i tested and if i keep only 1 interaction per file it passes, from the debug logs it looks like pact is doing the cleanup too early

mefellows commented 1 year ago

Can you please provide a reproducible example? We test the builds on node 18 so I don't think the node version is the problem (it might be, but seems unlikely). It smells of incorrectly handled promises, but hard to tell from that output.

RaresAil commented 1 year ago

@mefellows i created a repo with the issue: https://github.com/RaresAil/issue-env-pact

image
TimothyJones commented 1 year ago

Your example works for me using node 18.10.0, but fails in the same way as your test with node 19.5.0.

TimothyJones commented 1 year ago

@mefellows My guess is that the core isn't waiting for the server to start duing addInteraction. I'll leave this for someone more qualified to look into.

Edit: I confirmed that this is probably the case by adding a sleep for 1 second between addInteraction and the request. This caused the broken tests to then pass.

TimothyJones commented 1 year ago

@RaresAil Did you get any further with this? I ran in to the same issue in an unrelated project that uses axios, but does not use Pact. I suspect axios might be at fault

RaresAil commented 1 year ago

@RaresAil Did you get any further with this? I ran in to the same issue in an unrelated project that uses axios, but does not use Pact. I suspect axios might be at fault

I tried and with node-fetch and same issue what i did now as a workaround is the delay of 1 second

TimothyJones commented 1 year ago

I tried and with node-fetch and same issue

Aha. That points to the changing node version being the reason for the broken test. With this information, I was able to get to the bottom of it:

I checked the changelog - and one of the breaking changes in 19.x is that Keep-Alive is set to true on http connections. This is a problem on repeated tests, as at some point you will pick up a connection that is due to be closed soon. This also explains why your workaround of a one second pause works - as the default socket lifetime is 1 second.

You confirm this with httpAgent: new http.Agent({ keepAlive: false }), in your axios options (weirdly, if you're creating axios each time, then new httpAgent({keepAlive: true}) will also work, because the connection pool won't be reused).

I think the right fix is to retry on ECONNRESET.

@mefellows This is a thing that could be changed in the proxy to avoid it in the future. If you want to do this, you can just add a bit of middleware that has res.append('Connection', 'close');

RaresAil commented 1 year ago

Thank you, i will also change on my side from the delay of 1s to disable the keepAlive

github-actions[bot] commented 1 year ago

👋 Hi! The 'smartbear-supported' label has just been added to this issue, which will create an internal tracking ticket in PactFlow's Jira (PACT-788). We will use this to prioritise and assign a team member to this task. All activity will be public on this ticket. For now, sit tight and we'll update this ticket once we have more information on the next steps.

See our documentation for more information.

mefellows commented 1 year ago

@mefellows This is a thing that could be changed in the proxy to avoid it in the future. If you want to do this, you can just add a bit of middleware that has res.append('Connection', 'close');

Thanks Tim - awesome digging. That should be easy enough to do.

TimothyJones commented 1 year ago

I think this issue is the same as the one reported at the bottom of #1060 - since the error is socket hang up

baileyandy commented 1 year ago

For some reason, none of the above workarounds are helping my failing tests. I have reverted to node 16 for now.

However, I did discoverer that this has been known in the JVM world for a while.

TimothyJones commented 1 year ago

To be clear, this isn’t a problem with pact. This is pact exposing that your client isn’t handing its cached resources correctly, which leads to an unhandled error in your client. Whether or not this is a real problem depends on how your real provider behaves.

The suggestion I made to Matt was to modify the mock proxy to behave better, which it should (as it is a mock).

If the workarounds above don’t work, then I think you’re experiencing a different issue. You might want to open an issue with some code that reproduces it.

develmac commented 1 year ago

I know I am lat to the game, but having a similar issue with https://vitest.dev and pure fetch API implementation. In node 16 the Pact test works, but with 18 I can see in the console pact_mock_server::server_manager: Shutting down mock server with ID even before the http client code executes.

Test looks like this:

describe("Data contract", () => {
  it("Should get data", async () => {
    provider
      .given("placeholder")
      .uponReceiving("placeholder")
      .withRequest({
        method: "GET",
        path: "/api/resource",
        headers: { Accept: "*/*" },
      })
      .willRespondWith({
        status: 200,
        headers: { "Content-Type": "application/json" },
        body: {},
      })

    return provider.executeTest(async () => {
      const dtos = await getMyDtos()
      expect(dtos).not.toBeNull()
    })
  }, 20000)
})
TimothyJones commented 1 year ago

As above, I recommend reading the breaking changes between those node versions and adjusting accordingly.

baileyandy commented 1 year ago

@TimothyJones many thanks for the tips

I have re-read the Node changes in more detail and my issue was indeed caused by a change to the resolution of localhost. I ended up also finding this stackoverflow question on my quest. I am on OSX which I guess is resolving to IPv6 first. Switching to 127.0.0.1 has fixed my issue.

interestingly all my pacts were running in jsdom jest environment and passed with the localhost -> IP change. Switching to node jest environment caused everything to fail again unless I also added httpAgent: new http.Agent({ keepAlive: false }) to the axios config.

mefellows commented 1 year ago

See also https://github.com/pact-foundation/pact-js/discussions/1083. We should look into any quality of life improvements related to that also.

AugusteTomaseviciute commented 9 months ago

Switching to 127.0.0.1 has fixed my issue also!