eugef / node-mocks-http

Mock 'http' objects for testing Express routing functions
Other
747 stars 131 forks source link

Response does not throw an error when sending twice #262

Closed siavol closed 1 year ago

siavol commented 1 year ago

Node.js does not allow to send response twice, so the following header

function handler(req, res) {
  res.send('You can not send response twice!');
  res.send('This will throw an error');
}

will generate an error:

Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at new NodeError (node:internal/errors:387:5)
    at ServerResponse.setHeader (node:_http_outgoing:603:11)
    ...

But node-mock-http (ver. 1.12.1) is not throwing any error and this unit test fails:

test('handler throws when sending response twice', () => {
    const req = httpMocks.createRequest({
        method: 'GET',
        url: '/'
    });
    const res = httpMocks.createResponse();

    expect(() => handler(req, res)).toThrow();
})
eugef commented 1 year ago

HI @siavol, could you please check if version 1.11.0 has the same behaviour?

siavol commented 1 year ago

@eugef , I confirm that 1.11.0 has the same behaviour.

siavol commented 1 year ago

I took a look at the code and it is pretty straightforward to not allow setting headers after they are sent to the client. But mockResponse.send does not set any headers, so that change will be not enough. Also method docs explicitly says that send can be called multiple times. I wonder what is the reason of this decision?

eugef commented 1 year ago

That code was written almost 8 years ago, so not sure if we can figure out the reason.

The proper behaviour for the mock would be to throw the same type of error. If you can create a PR with the fix - your help will be much appreciated.

siavol commented 1 year ago

Thank you! I am ready to take care of this issue. Going to provide PR in a few days.

siavol commented 1 year ago

I've prepared a pull request: https://github.com/howardabrams/node-mocks-http/pull/263

github-actions[bot] commented 1 year ago

Stale issue message

HartS commented 1 year ago

@eugef do you know why this issue (and corresponding PR) were closed? I realize the PR itself didn't get feedback addressed, but this is still an issue, right?

eugef commented 1 year ago

Github bot regularly closes stale issues and PRs

The issue still exists, you can help fixing it by improving this PR https://github.com/eugef/node-mocks-http/pull/263

HartS commented 1 year ago

Thanks @eugef . I'm kind of just getting familiar of the ergonomics of using this library.

I assume it's also be possible to spy on the res.send function to ensure it's not called more than once, and now I'm wondering if it's better to have the flexibility to allow some tests to send more than once (for example, I have req and res regenerated in a beforeEach, but if I wanted to run through the same middleware I'm testing twice in the same test, then it might be useful that res.send can be called twice)

Feel free to close, and I may re-open with a PR that makes sense to me once I have a better idea of how people might be using this. Perhaps an additional method to enable the error on sending twice is the best approach, since that would make it a non-breaking change for people who might be relying on being able to send multiple times with the same res object.

github-actions[bot] commented 1 year ago

Stale issue message