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

Only restore the http/https request when used. #25

Closed
wants to merge 2 commits into from

Conversation

xavierchow
Copy link
Contributor

PR for #24

@codecov-io
Copy link

Current coverage is 92.85%

Merging #25 into master will decrease coverage by -4.81% as of 4f3b8af

@@            master     #25   diff @@
======================================
  Files            3       3       
  Stmts          257     266     +9
  Branches         0      65    +65
  Methods          0       0       
======================================
- Hit            251     247     -4
- Partial          0      13    +13
  Missed           6       6       

Review entire Coverage Diff as of 4f3b8af

Powered by Codecov. Updated on successful CI builds.

@alsotang
Copy link
Member

+1

@@ -260,6 +258,7 @@ function mockRequest() {
* @param {Number} [delay], response delay time, default is 10.
*/
exports.http.request = function (url, data, headers, delay) {
getAgent(http).__sourceRequest = getAgent(http).request;
return _request.call(this, http, url, data, headers, delay);
Copy link
Member

Choose a reason for hiding this comment

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

If test call mock request twice, retore will be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmh... indeed. Although it's meaningless if user calls the mock request twice(because the later mock will override the previous one), unable to restore to the original one is unacceptable. Will update the PR.

@fengmk2
Copy link
Member

fengmk2 commented Nov 30, 2015

Please add a test case for this change.

@fengmk2
Copy link
Member

fengmk2 commented Jan 22, 2017

+1

@fengmk2
Copy link
Member

fengmk2 commented Jan 22, 2017

I will merge this in command line

@fengmk2
Copy link
Member

fengmk2 commented Jan 22, 2017

landed 3c1ed43

@fengmk2 fengmk2 closed this Jan 22, 2017
@fengmk2
Copy link
Member

fengmk2 commented Jan 22, 2017

2.0.1

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

Successfully merging this pull request may close these issues.

4 participants