feat(requestoverrider): Add method property to mocked requests

[mrijke]

Mocked requests were missing the 'method' property on them.

Added a testcase to the test_request_overrider testsuite.

[gr2m]

Hey Maarten,

thanks for the pull request!

It would be very helpful if you first committed and pushed the test only, so we can directly see that it fails. Once the build failed, push the fix. It takes a little longer, but it’s very helpful for maintainers to review

Example

[paulmelnikow]

Hi! Thanks for the PR. I may not have understood #1558.

Is adding method to req a behavior that is specific to superagent, or is it something that node's http module does? In general, Nock should do the standard thing, not implement workarounds and special behaviors for individual libraries.

As I mentioned at #1558:

A note on the testing strategy: we're moving away from preventing regression of integration problems using wide-bracket integration tests (#1427). So we should instead add a test which isolates the specific client behavior, and be added to the client-facing tests in https://github.com/nock/nock/blob/beta/tests/test_request_overrider.js.

The integration test can (and even should) be used to confirm the fix at the time of the change, but won't be kept in the main nock codebase.

So this will also need a regression test which isolates the specific client behavior, that can be kept in place of the one added here based on superagent.

[mrijke]

@gr2m OK no problem, I'll rebase and split the commits.

@paulmelnikow: I'm not extremely familiar with node's http module, but as far as I can see this method property is something explicitly added by superagent (see their types).
I'm using this in my app to handle error responses differently, based on the method that was used. I honestly wasn't aware that it wasn't a default thing until that part of my code didn't work in a testcase in which I used nock.
I've added a regression testcase in my commit, as suggested by @gr2m I will split it in two commits. Is this what you meant by this, or am I misunderstanding something? You mention isolating client specific behavior, and since I think superagent is the only client doing this (at least not node's http module), I assume this test case is enough?

[paulmelnikow]

Not sure I understand why those type defs are written the way they are, though fortunately, it doesn't seem like the method property is specific to superagent:

> require('http').request('http://www.google.com').method
'GET'

Added: And not so with Nock:

> const nock = require('.')
undefined
> const scope = nock('http://google.com').get('/').reply()
undefined
> require('http').request('http://google.com/').method
undefined

[mrijke]

@paulmelnikow you're right... I was trying it out in runkit while nock was still mocking things, so I didn't see it...
How would you like me to proceed? I can turn this PR into something generic for http instead, removing all mentions of superagent, and keep the testcase but use http instead.

[paulmelnikow]

This works for me! Left a couple minor tweaks. Thanks so much!

[paulmelnikow]

🙌🏻

[nockbot]

🎉 This PR is included in version 11.0.0-beta.16 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[nockbot]

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀