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

Problems with Mock Server when running Pact with Jest #1060

Closed subodhgodbole closed 1 year ago

subodhgodbole commented 1 year ago

Software versions

Issue Checklist

Expected Behavior

Mock server should start and respond as per Interactions defined.

Actual Behavior and Steps to reproduce

I am trying to use Pact in my Angular 13 workspace with Jest for writing contract tests. I am using latest version of Pact which is v10.4.1.

However, I am running into problems related to Mock Server. It seems that Mock Server is not receiving any requests. I have added multiple debug logs to check which URL is used by Angular's HttpClient and it appears to point correctly to Mock Server's dynamic URL. See this -

console.log
**** Adding Interaction with path: /users/1
  at src/app/services/user.service.pact.spec.ts:44:15

console.log
**** MockServer:: URL: http://127.0.0.1:50118, ID: unknown
  at src/app/services/user.service.pact.spec.ts:65:17

console.log
**** UserService.get(): http://127.0.0.1:50118/users/1
  at UserService.get (src/app/services/user.service.ts:29:13)

From above -

But still it's not working.

Also, I am not sure why Mock Server Id is coming out as "undefined".

Error I get is as below -

RUNS  src/app/services/user.service.pact.spec.ts
2023-02-08T10:33:00.360413Z DEBUG ThreadId(01) pact_matching::metrics: Could not get the tokio runtime, will not send metrics - there is no reactor running, must be called from the context of a Tokio 1.x runtime
2023-02-08T10:33:00.360795Z DEBUG ThreadId(01) pact_mock_server::server_manager: Shutting down mock server with ID ca85dcf4-01b7-4d4e-af7a-890baaa75559 - MockServerMetrics { requests: 0 }
2023-02-08T10:33:00.363789Z DEBUG ThreadId(01) pact_mock_server::mock_server: Mock server ca85dcf4-01b7-4d4e-af7a-890baaa75559 shutdown - MockServerMetrics { request  console.error
    Unhandled Promise rejection: 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: /users/1 ; Zone: ProxyZone ; Task: Promise.then ; Value: Error: 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: /users/1
        at PactV3.<anonymous> (C:\angular-pact\node_modules\@pact-foundation\src\v3\pact.ts:227:29)
        at step (C:\angular-pact\node_modules\@pact-foundation\pact\src\v3\pact.js:33:23)
        at Object.next (C:\angular-pact\node_modules\@pact-foundation\pact\src\v3\pact.js:14:53)
        at fulfilled (C:\angular-pact\node_modules\@pact-foundation\pact\src\v3\pact.js:5:58)
        at _ZoneDelegate.Object.<anonymous>._ZoneDelegate.invoke (C:\angular-pact\node_modules\zone.js\bundles\zone-testing-bundle.umd.js:409:30)

May be I am missing something very trivial/basic, I would really appreciate if you have any clue / suggestion on what might be wrong in my project?

I have uploaded my Angular project to GitHub here, where this is reproducible. (After cloning, command to run is npm run test:pact).

PS: I raised this on stackoverflow few days back. Also raising it here, in anticipation to get resolution faster.

mefellows commented 1 year ago

Thanks for the detailed report and repro - much appreciated! I'm going to remove the bug label until we can verify it as such.

       Path: /users/1 ; Zone: ProxyZone ; Task: Promise.then ; Value: Error: Test failed for the following reasons:

This makes me think there is a proxy of sorts intercepting requests - probably as part of the Angular setup.

I'm not an Angular developer, but I know it does funky things with the HTTP clients in the test bed (or whatever they call it), so this is where I'd be investigating. Alternatively, there is some promise issue where the promises are firing out of order.

TimothyJones commented 1 year ago

Can you share the test that generates this problem?

mefellows commented 1 year ago

I think it's this one Tim: https://github.com/subodhgodbole/angular-pact/blob/main/src/app/services/user.service.pact.spec.ts

My guess is that it's something to do with Angular proxying/interfering with HTTP requests. It could probably be shown to be the issue by using a different HTTP client (e.g. axios). If it works, you are likely to believe it's something to do with the Angular setup. If it doesn't work, then it would be a bit more digging, but one would also have to suspect some other deeper dependency/configuration is at work (because we have many working examples)

TimothyJones commented 1 year ago

That test isn't right. It's not handling the promise from executeTest so it probably succeeds even when it fails.

    it('should get a User', (done) => {
      provider.executeTest(async(mockServer) => {
        console.log(`**** MockServer:: URL: ${mockServer.url}, ID: ${mockServer.id}`);
        service.setUrlPrefix(mockServer.url);

        service.get(userId).subscribe({
          next: (response) => {
            console.debug(response);
            expect(response).toEqual(expectedUser);
            done();
          },
          error: (error: HttpErrorResponse)=> {
            done();
          }
        });
      });
    });

should be (at least):

    it('should get a User', () => {
      return provider.executeTest(async (mockServer) => {
        console.log(
          `**** MockServer:: URL: ${mockServer.url}, ID: ${mockServer.id}`
        );
        service.setUrlPrefix(mockServer.url);

        service.get(userId).subscribe({
          next: (response) => {
            console.debug(response);
            expect(response).toEqual(expectedUser);
          },
          error: (error: HttpErrorResponse) => {
            throw error;
          },
        });
      });
    });
TimothyJones commented 1 year ago

Yep. This one is entirely unhandled promises at fault - the problem is that the Observable thing in angular doesn't wait for the request to be sent, so the executeTest callback returns immediately, and Pact thinks that the request has been sent before it has.

I don't know what you're supposed to do idomatically in Angular. But, this updated test works:

    it('should get a User', () => {
      return provider.executeTest(async (mockServer) => {
        console.log(
          `**** MockServer:: URL: ${mockServer.url}, ID: ${mockServer.id}`
        );
        service.setUrlPrefix(mockServer.url);

        return new Promise<void>((resolve, reject) => {
          service.get(userId).subscribe({
            next: (response) => {
              console.debug(response);
              expect(response).toEqual(expectedUser);
              resolve();
            },
            error: (error: HttpErrorResponse) => {
              reject(error);
            },
          });
        });
      });
    });

I have triple checked, that there are no unhandled promises in my code and have read the section on intermittent test failures

^ This means that you need to make sure that the executeTest doesn't complete before you have sent your request. This should probably be highlighted clearer in the documentation.

subodhgodbole commented 1 year ago

Thanks @TimothyJones. You pointed it right, test is finishing early and not waiting for executeTest() to finish. IMO, it's probably the Jest and not Angular who is behaving like this here.

Exactly for this async behavior Jest offers done callback for tests. As per Jest documentation, it should wait till done() is called. And if you have noticed, I had used done() and called it only when executeTest() completed. But it did not work. :-(

    it('should get a User', (done) => {
      provider.executeTest(async(mockServer) => {
        console.log(`**** MockServer:: URL: ${mockServer.url}, ID: ${mockServer.id}`);
        service.setUrlPrefix(mockServer.url);

        service.get(userId).subscribe({
          next: (response) => {
            console.debug(response);
            expect(response).toEqual(expectedUser);
            done();
          },
          error: (error: HttpErrorResponse)=> {
            done();
          }
        });
      });
    });

I have used this done() callback at other places as well like beforeAll(), afterEach() and it worked there.

TimothyJones commented 1 year ago

No, that is not right. In that example,done is not correctly used.

Here is the sequence of events:

  1. Set up pact expectation
  2. Send request
  3. Receive response
  4. Verify that the provider received the correct information

In your example, you have written this:

  1. Set up pact expectation
  2. Send request
  3. Receive response
  4. Call done
  5. Verify that the provider received the correct information (this is now ignored by Jest)

In general, it's better to use the promise form instead of the callback form, as it avoids these kind of issues (I think using done is actually a lint error in the recommended jest eslint rules).

subodhgodbole commented 1 year ago

Nope! That does not appear to be the case as per code. In code I am calling done() in next after service returns data. So theoretically it appears right. Mock Server has work to do in between service.get(userId) and .subscribe(). And I am asserting tests in next and calling done() at the end.

TimothyJones commented 1 year ago

You are not using done correctly. See my earlier messages, and try the corrected code I gave you.

subodhgodbole commented 1 year ago

I have pushed one more commit to my Git Repo. In this commit -

Test Suites: 1 passed, 1 total Tests: 1 passed, 1 total Snapshots: 0 total Time: 39.183 s


- Also added standard test case for Jest without Pact - [user.service.jest.spec.ts](https://github.com/subodhgodbole/angular-pact/blob/main/src/app/services/user.service.jest.spec.ts)
 Here I am using `done` callback, exactly the same way I was using it in Pact test earlier.
 This also gets passed correctly.
 Command to run is - `npm run test:jest`

PASS src/app/services/user.service.jest.spec.ts (12.524 s) UserService √ should be created (58 ms) √ should get user (20 ms)

Test Suites: 1 passed, 1 total Tests: 2 passed, 2 total Snapshots: 0 total Time: 36.105 s



Looks like for Pact Tests `promise` is only the way forward, as `done` callback approach supported by Jest is not working.
For me as consumer, I am not sure if it's Jest issue or Pact! But that's fine. I can go ahead and use `Promise` in Pact tests.

I believe we can close this issue as original issue is resolved.
Thanks again for your help for spotting the root cause and providing solution.
TimothyJones commented 1 year ago

I'm glad you've got it working.

However, I think you still haven't understood what I was saying above. Pact is compatible with done - you just have to make sure you only call it after the test has finished. To illustrate this, here's your test but rewritten with done callbacks:

    it('should get a User', (done) => {
      provider
        .executeTest(async (mockServer) => {
          console.log(
            `**** MockServer:: URL: ${mockServer.url}, ID: ${mockServer.id}`
          );
          service.setUrlPrefix(mockServer.url);

          return new Promise<void>((resolve, reject) => {
            service.get(userId).subscribe({
              next: (response) => {
                console.debug(response);
                expect(response).toEqual(expectedUser);
                resolve();
              },
              error: (error: HttpErrorResponse) => {
                reject(error);
              },
            });
          });
        })
        .then(
          () => {
            done();
          },
          (e) => done.fail(e)
        );
    });

As you can see, it's more wordy. If you prefer this way, then you could use this style instead.

Calling done isn't magic, you still need to make sure you understand the flow of the promises in your program, and only call it once the test is complete.

As an aside, this is a really good example of why done callbacks are discouraged by linters - it can be challenging to see the most appropriate place to put them. Personally, I agree with the linters - I would recommend instead returning promises; returning promises is a much more explicit way to write the test, and you don't need to remember to handle the failure case separately.

TimothyJones commented 1 year ago

A second aside - I looked at your other, non-pact test, and you are not using done correctly there either. There could be a race condition that would result in the expect(httpClientSpy.get).toHaveBeenCalledTimes(1); test to either not be run (resulting in a pass that didn't check whether the spy was called), or to be run too early (resulting in a fail before the spy was called).

To fix this, I would recommend moving that expect call inside the callback.

This isn't jest support though, so I'll leave correcting the rest of the uses of done to you and your team.

subodhgodbole commented 1 year ago

Thanks!! But IMO, main point of comparison was that -

Like I said this thread can be closed, as main problem with witting Pact Tests is resolved by using promise.

Thanks.

pdeszynski commented 1 year ago

I'm not sure whether this Issue should be closed. The same happens when jest-pact is being used. Pact fails when there are more than one tests using the same provider instance, for e.g. when running an example test - https://github.com/pact-foundation/pact-js/tree/master/examples/jest - from this repository it fails with

act between Jest-Consumer-Example and Jest-Provider-Example
    with 30000 ms timeout for Pact
      Dogs API
        ✓ returns a successful body (12 ms)
      Cats API
        ✕ returns a successful body (18 ms)

  ● Pact between Jest-Consumer-Example and Jest-Provider-Example › with 30000 ms timeout for Pact › Cats API › returns a successful body

    socket hang up
mefellows commented 1 year ago

Hi @pdeszynski I think that is a separate issue from the one initially reported here.

Could you please open a new ticket with a reproducible example (the issue template should have this info) and we can look into it separately?

TimothyJones commented 1 year ago

@pdeszynski are you using node 19? If yes, see #1066 for further information.