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

Updated the scope.request event emitter to include the request body #1062

Merged
merged 1 commit into from
Jun 29, 2018

Conversation

robballou
Copy link
Contributor

I am working on a testing scenario where I am running a method that then makes several requests. In the tests, I needed to later inspect the request body, but it did not appear to include a simple way to get the request body. If there is another way to do this, no worries, but this was helpful for me.

Example:

const twitter = nock(/twitter\.com/).persist().post(/media\/upload/).reply(200);
const methods = {
  'INIT': [],
  'APPEND': [],
  'FINALIZE': [],
  'STATUS': []
};

twitter.on('request', (req, interceptor, body) => {
  const parsed = querystring.parse(body);
  if (parsed.command) {
    methods[parsed.command].push(body);
  }
});

return myMethod().then(() => {
  expect(methods.INIT).to.have.lengthOf(1);
  // ...
})

Not sure if this would be helpful (or better?) in the replied event? Essentially I wanted to spy on which interceptors had been called.

And thanks for this package, it is extremely helpful in tests! 👍

@aarongreenwald
Copy link

Hate to comment just to say +1, but this is something that I need as well. Did anyone look at this and decide whether it's something they want to merge or not? Or this PR just sitting here all alone and brokenhearted, with nobody noticing it and nobody to love it?

@gr2m gr2m force-pushed the request-events-include-body branch from c47ba14 to 7bd7e84 Compare June 29, 2018 19:51
@gr2m
Copy link
Member

gr2m commented Jun 29, 2018

probably the latter I’m afraid. Good news is that thanks to a sponsorship I was able to hire https://maintainer.io/ to help us turn nock into a vital project again :) stay tuned, and give us a hand if you can. #1077 is a great place to start.

I’ve rebased the PR on latest master, if CI is green I’m okay with merging the PR

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good, thanks @robballou & @aarongreenwald

@gr2m gr2m merged commit e15820a into nock:master Jun 29, 2018
@nockbot
Copy link
Collaborator

nockbot commented Jun 29, 2018

🎉 This PR is included in version 9.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aarongreenwald
Copy link

@gr2m Thanks! I'm glad to hear you're able to get this going, and I'll be using it in my project. Will definitely try to report issues or submit PRs if I'm able. Thanks for developing/maintaining nock, it's a great library and helps me a ton with my testing.

@gr2m
Copy link
Member

gr2m commented Jul 2, 2018

Thank you @aarongreenwald! if you like to help, a great place to start is #1077. It will make the test setup more reliable and hence the experience for all contributors and maintainers better moving forward. See you around :)

@lock
Copy link

lock bot commented Sep 13, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Sep 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants