eugef / node-mocks-http

Mock 'http' objects for testing Express routing functions
Other
753 stars 133 forks source link

Allow for specifying `{ method: undefined }` #195

Closed michielbdejong closed 4 years ago

michielbdejong commented 4 years ago

We want to test how our code responds to a http IncomingMessage where the method is undefined.

eugef commented 4 years ago

Hi @michielbdejong, could you please give an example how the method of the IncomingMessage might be undefined? How are you even able to send such a request?

michielbdejong commented 4 years ago

Ha, you're right, https://nodejs.org/api/http.html#http_class_http_incomingmessage says it's always a string, so it should not be necessary, but the following happens:

  1. @types/node says that IncomingMessage#method is optional.
  2. If we write const method: string = request.method; then TypeScript will complain something to the effect of 'cannot assign <string | undefined> to <string>.
  3. So in order to stop that TypeScript compile error, we change our code to const method: string = request.method || 'GET';
  4. Now the branch coverage of that line is only 50% because jest says our unit tests are not testing the case where request.method is undefined.
  5. we want to write a unit test where request.method is undefined, so that we can achieve 100% branch coverage.

So sounds to me like the ? in https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/http.d.ts#L284 is essentially incorrect here? Maybe I misunderstood, I'm pretty new to branch coverage maximisation for TypeScript code.

michielbdejong commented 4 years ago

Ah I guess the comment above that line explains why the ? is there.

eugef commented 4 years ago

Indeed, that comment explains.

But this module is meant to be used only for testing http.Server requests. In this context - method cannot be undefined.

I suggest a workaround for your case:

const req  = httpMocks.createRequest();
req.method = undefined;