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

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

Open
4 tasks done
RaresAil opened this issue Jan 25, 2023 · 21 comments
Open
4 tasks done

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

RaresAil opened this issue Jan 25, 2023 · 21 comments
Labels
enhancement Indicates new feature requests

Comments

@RaresAil
Copy link

RaresAil commented Jan 25, 2023

Software versions

  • OS: MacOS Ventrua
  • Pact Node version: Pact Foundation 10.3.1
  • Node Version: 18.10.x, 19.x

Issue Checklist

Please confirm the following:

  • I have upgraded to the latest
  • I have the read the FAQs in the Readme
  • I have triple checked, that there are no unhandled promises in my code
  • I have set my log level to debug and attached a log file showing the complete request/response cycle

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
      �[31mPact verification failed!�[39m

      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
      �[31mTest 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
@RaresAil RaresAil added the bug Indicates an unexpected problem or unintended behavior label Jan 25, 2023
@TimothyJones
Copy link
Contributor

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
Copy link
Author

RaresAil commented Jan 26, 2023

I get the base url like this:

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

EDIT: The value is 127.0.0.1:port

@RaresAil
Copy link
Author

@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
Copy link
Member

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.

@mefellows mefellows added Awaiting feedback and removed bug Indicates an unexpected problem or unintended behavior labels Jan 26, 2023
@RaresAil
Copy link
Author

RaresAil commented Jan 27, 2023

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

image

@TimothyJones
Copy link
Contributor

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
Copy link
Contributor

TimothyJones commented Jan 27, 2023

@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
Copy link
Contributor

@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
Copy link
Author

RaresAil commented Feb 2, 2023

@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
Copy link
Contributor

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
Copy link
Author

RaresAil commented Feb 2, 2023

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

@mefellows mefellows transferred this issue from pact-foundation/pact-js-core Mar 3, 2023
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

👋 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
Copy link
Member

@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
Copy link
Contributor

TimothyJones commented Mar 3, 2023

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

@baileyandy
Copy link

baileyandy commented Apr 3, 2023

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
Copy link
Contributor

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
Copy link

develmac commented Apr 5, 2023

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
Copy link
Contributor

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

@baileyandy
Copy link

@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
Copy link
Member

See also #1083. We should look into any quality of life improvements related to that also.

@mefellows mefellows added the enhancement Indicates new feature requests label Aug 18, 2023
@AugusteTomaseviciute
Copy link

AugusteTomaseviciute commented Oct 16, 2023

Switching to 127.0.0.1 has fixed my issue also!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates new feature requests
Projects
Status: New Issue
Development

No branches or pull requests

6 participants