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

Allow agent to be set on proxy options and passed into Nipple. #1863

Merged
merged 5 commits into from Aug 24, 2014

Conversation

@ndvivedi
Copy link
Contributor

@ndvivedi ndvivedi commented Aug 14, 2014

Changes to support putting an agent on the options argument passed into the proxy handler.

There was an issue created for this previously, that was closed due to inactivity.

#1596

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 15, 2014

Your test isn't actually testing anything...

@ndvivedi
Copy link
Contributor Author

@ndvivedi ndvivedi commented Aug 15, 2014

The test is just asserting that when an agent is set on the options argument to proxy the subsequent call to Nipple.request contains the same object on the agent property of its options argument.

If I remove the change at lines 29-32 in lib/proxy.js the test fails like I would expect.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 16, 2014

Might as well call done() from inside your mocked nipple.request overload. Add { parallel: false } to the test options to indicate you are messing with internals.

@ndvivedi
Copy link
Contributor Author

@ndvivedi ndvivedi commented Aug 17, 2014

The changes to the test are finished.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 19, 2014

Update the doc and it's good.

@hueniverse hueniverse self-assigned this Aug 19, 2014
@Marsup
Copy link
Contributor

@Marsup Marsup commented Aug 19, 2014

Shouldn't he also reword Nipple to Wreck ?

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 19, 2014

Yeah, but I clean up everything anyway... :-) I just don't want to write more docs...

@ndvivedi
Copy link
Contributor Author

@ndvivedi ndvivedi commented Aug 19, 2014

Doc is updated.

One extra thing, the test just before the one i added has the same pattern (modifying Nipple.request to do an assertion). I made the same changes as i did to my test, adding the parallel : false option and moving the done() call, but have not submitted it. Do you want that?

hueniverse pushed a commit that referenced this issue Aug 24, 2014
Allow agent to be set on proxy options and passed into Nipple.
@hueniverse hueniverse merged commit f6655d4 into hapijs:master Aug 24, 2014
1 check passed
@hueniverse hueniverse added this to the 6.6.0 milestone Aug 24, 2014
hueniverse pushed a commit that referenced this issue Aug 24, 2014
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants