ComparetheMarket / chai-nock

Chai Nock extends Chai with a language for asserting facts about Nock.
MIT License
8 stars 3 forks source link

Using chai-nock with asynchronous functions? #14

Open dcermak opened 4 years ago

dcermak commented 4 years ago

Hi,

I've been trying to get chai-nock to work with asynchronous code but have failed so far unfortunately.

Let's say I have a function that make a PUT request: async function makePost() and I try to verify the payload. When I try to test it with mocha like this:

describe("example", () => {
  it("should work", () => {
  const scope = nock(Url)
      .put(/.*/)
      .reply(200, "foo");

  makePost().should.be.fulfilled.and.eventually.deep.equal("foo");
  scope.should.have.been.requestedWith("my expected payload here");
});
});

Then this never works: I get errors from chai-nock that the request didn't match:

AssertionError: expected Nock to have been requested with exact body 'my expected payload here', but was requested with body undefined

To me that looks like the request is checked before the asynchronous function actually resolves. Unfortunately, I have not been able to properly defer the check. I have tried to use chai-as-promised, declaring the test function in it() as async and awaiting the result of makePost and probably various other things, but they all result in the same outcome (only sometimes it is masked by the fact that the assertion fails in a non-resolved promise that is only sometimes reported as a warning by node).

Is there a way how I could make this work? I really need to support async functions unfortunately.

chrisandrews7 commented 4 years ago

All chai plugins that use some form of promises for their assertions require you to wait for those promises to complete. If you look at the first few examples in the chai-as-promised docs, you are required to either return or await the promise, or use notify(done). The same theory works here, so your example would look like:

describe("example", () => {
  it("should work", async () => {
    const scope = nock(Url)
      .put(/.*/)
      .reply(200, "foo");

    await makePost().should.be.fulfilled.and.eventually.deep.equal("foo");
    await scope.should.have.been.requestedWith("my expected payload here");
  });
});
dcermak commented 4 years ago

Thanks for your prompt reply @chrisandrews7!

Unfortunately the proposed example doesn't work for me, the test case in question simply times out with:

     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

The culprit is the second assertion: removing it makes the test pass (also I have been using chai-as-promised in this form in other parts of my code).

Could the issue here be maybe some weird clash between chai-as-promised and chai-nock itself? Afaik chai-as-promised hooks into all other chai plugins to make them awaitable, but chai-nock performs some promisify itself. Could it be that these two don't play together?

chrisandrews7 commented 4 years ago

Thanks @dcermak, after some testing there looks to be a problem when chai-nock is setup before chai-as-promised. Will get a fix raised. In the meantime if you chai.use on chai-as-promised first and then chai-nock after, it seems to work for me:

const { use } = require('chai');
const chaiNock = require('chai-nock');
const chaiAsPromised = require('chai-as-promised');

use(chaiAsPromised); // This needs to be first
use(chaiNock);
dcermak commented 4 years ago

Sadly, that doesn't fix the issue for me. I already tried completely removing chai-as-promised, as I was suspecting some form of clash between the two, but that did not solve the issue for me.

Honestly, I believe that I've hit a more fundamental problem here that comes from the ordering:

describe("example", () => {
  it("should work", async () => {
    // the nock is setup here for one call
    const scope = nock(Url)
      .put(/.*/)
      .reply(200, "foo");

    // now we make the call, the nock is used here as we await the reply
    await makePost().should.be.fulfilled.and.eventually.deep.equal("foo");
    // now we try to check the expectations
    await scope.should.have.been.requestedWith("my expected payload here");
  });
});

The issue is step 3: await scope.should.have.been.requestedWith("my expected payload here");, since makePost() was awaited, the nock got already "consumed". But the first thing that is done in the requestedWith call, is to setup timeouts for the nock and convert it to a promise. I.e. stuff that must occur before the web request was made. But it is done afterwards, which explains the errors I am seeing: the check is run against a supposed call that should happen after the call to makePost(), but that one never comes (unsurprisingly).

I think that this needs to be fixed differently, maybe by reworking the chaining, so that the nock gets setup before the actual call even for async functions. For instance something like this:

const scope = nock(Url).put(/.*/).reply(200, "foo")
  .should.be.requestedBy(makePost).and.be.requestedWith("my expected payload here");

requestedBy would then setup the nock promise before awaiting makePost() and later it could check the payload.

dcermak commented 4 years ago

So, I've given this a try to create something that would workaround the ordering issue and got relatively far (see: https://gist.github.com/dcermak/38266c88f3cedf7b419e1c32ffc80a80, most of that is copy-pasted from chai-nock itself). It currently looks like this:

describe("example", () => {
  it("should work", async () => {
    const scope = nock(Url)
      .put(/.*/)
      .reply(200, "foo")

    await scope.should.have.been.requestedByWith(makePost, "my expected payload here");
  });
});

This part actually appears to work, the only downside is now that if makePost() returns some data, those are lost unfortunately. I have tried to save the result from makePost() in promisfyNockInterceptor, but unfortunately the nock.on("replied") resolves faster then awaiting the result of makePost and it never makes it out of the function.

dcermak commented 4 years ago

I actually managed to extract the result of makePost() from the call and updated the gist accordingly. The only problem that I am now facing is that I am not able to "return" the result from the chain, so that this would work:

scope.should.have.been
  .requestedByWith(makePost, "my expected payload here")
  .and.deep.equal({status: "ok"});

But then again on can work around this as follows:

let res;
scope.should.have.been
  .requestedByWith(async () => (res = await makePost()), "my expected payload here");
res.should.deep.equal({status: "ok"});

@chrisandrews7 What do you think about all this? If you think that this could become a useful addition to chai-nock, then I'll try to create a pull request.

chrisandrews7 commented 4 years ago

You're quite right @dcermak. The problem is once the nock is consumed it disappears unless its set to persist. So in your example the first assertion consumes the nock using chai-as-promised, and the second assertion using chai-nock doesn't have the nock anymore to assert on. Appreciate the example above but ideally the API should be much simpler and the burden not on the consumer of the library. I think the fundamental way in which the nock is checked needs some careful thought and refactoring. Will adding persist() to your nock work in the meantime?

dcermak commented 4 years ago

Chris Andrews notifications@github.com writes:

You're quite right @dcermak. The problem is once the nock is consumed it disappears unless its set to persist. So in your example the first assertion consumes the nock using chai-as-promised, and the second assertion using chai-nock doesn't have the nock anymore to assert on. Appreciate the example above but ideally the API should be much simpler and the burden not on the consumer of the library. I think the fundamental way in which the nock is checked needs some careful thought and refactoring. Will adding persist() to your nock work in the meantime?

I am not sure (I'll need to check multiple requests after each other), but I'm afraid that my use case requires a custom chai plugin anyway.