Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'method' property not available on superagent mocked requests #1558

Closed
mrijke opened this issue May 16, 2019 · 4 comments
Closed

'method' property not available on superagent mocked requests #1558

mrijke opened this issue May 16, 2019 · 4 comments

Comments

@mrijke
Copy link
Contributor

mrijke commented May 16, 2019

What is the expected behavior?
The property 'method' should be available on a request from superagent, similar to here:
https://runkit.com/mrijke/superagent-with-method-on-request

What is the actual behavior?
The 'method' property is undefined

Possible solution
Copy the value from the 'method' property, if available (i.e. the request came from superagent)

How to reproduce the issue

https://runkit.com/mrijke/nock-superagent-undefined-method

Does the bug have a test case?
No, but the superagent specific testcase can be extended, e.g.

test('superagent works', t => {
  const responseText = 'Yay superagent!'
  const headers = { 'Content-Type': 'text/plain' }
  nock('http://example.test')
    .get('/somepath')
    .reply(200, responseText, headers)

  superagent.get('http://example.test/somepath').end(function(err, res) {
    t.equal(res.text, responseText)
    t.equal(res.req.method, "get") // <-- added assert here
    t.end()
  })
})

Versions

Software Version(s)
Nock 10.0.6
Node 10.15.3
@paulmelnikow
Copy link
Member

Hi! Thanks for the report. Does this problem exist in 11.x as well? I'm guessing yes?

@mrijke
Copy link
Contributor Author

mrijke commented May 16, 2019

@paulmelnikow Yes, I tried updating my runkit to 11.0.0-beta.14 and the issue is still there.

@paulmelnikow
Copy link
Member

Awesome. Thanks!

To you, or anyone else: we'd welcome a PR with a fix and an isolated test!

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.

We're also open to contributions on #1427 toward building up and maintaining separate client integration tests which test against the latest development versions of Nock.

@mastermatt
Copy link
Member

This was fixed with #1561

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants