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

Fix httpAgent reference assignment #6

Closed
wants to merge 1 commit into from

Conversation

@sandfox
Copy link

sandfox commented Aug 9, 2016

httpAgent stats were not being displayed because local references to the agent(s) was overwritten by a reference to either the global or user supplied httpsAgent(s)

end result was that http agents stats were not visible

httpAgent stats were not being displayed because local references to the agent(s) was overwritten by a reference to either the global or user supplied httpsAgent(s)

end result was that http agents stats were not visible
@arb arb self-assigned this Aug 9, 2016
@arb arb added the bug label Aug 9, 2016
@arb arb added this to the 1.0.3 milestone Aug 9, 2016
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Aug 9, 2016

The fact that this has passing tests with and without this change makes me suspect of the tests. Do you think you could edit some of the existing http/https tests to verify this changes is correct.

(Just eye-balling it, it looks right FYI 👍 )

@sandfox

This comment has been minimized.

Copy link
Author

sandfox commented Aug 10, 2016

Sure thing, will see what I can do. I completely forgot about the tests actually! I can see why the tests happened to pass despite the bug, the count of http and https requests made is the same so the values were inter-changable in the test expectation.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Oct 27, 2016

@sandfox can you take a look at the tests to make sure this change is good?

@sandfox

This comment has been minimized.

Copy link
Author

sandfox commented Oct 31, 2016

@arb sorry, had a quick look and got as far realising the https and http test were quite intertwined and couldn't see an easy way to separate them out. I've been a little caught up with PR's on another project but I can try and have a look sometime this week about fixing up the tests

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Jun 12, 2017

@sandfox do you still have interest in landing this PR? Please check/write/update tests to make sure this doesn't regress in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.