GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http: throw an error for unexpected agent values by brad-decker · Pull Request #10053 · nodejs/node

Conversation

@brad-decker

@brad-decker brad-decker commented Dec 1, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http, test

Description of change

As per #9069 unexpected things can happen when supplying
an unexpected value to agent. Beings as the docs clearly
state the expected values, this throws an error on an
unexpected value.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the http Issues and PRs related to the http subsystem. label Dec 1, 2016
bnoordhuis
bnoordhuis requested changes Dec 1, 2016
View reviewed changes
Comment thread lib/_http_client.js Outdated

@bnoordhuis bnoordhuis Dec 1, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please remove the console.log.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 1, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

doh, thats embarassing. Will remove.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/_http_client.js Outdated

@bnoordhuis bnoordhuis Dec 1, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

What is the reason for these specific checks vs. a simple } else {?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 1, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We should accept an instance of Agent.Agent. Had to check for the null value as well because the previous if statement also requires options.createConnection to not be a function.

This if block could probably be rewritten slightly to account for all acceptable values and then throw the error in a simple else clause.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/_http_client.js Outdated

@bnoordhuis bnoordhuis Dec 1, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Tiny nit: can you indent by four spaces here? It's allowed to use string templates (backticks) if you think that's easier to read.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 1, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

cool.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 2, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@cjihrig requested moving the + to the previous line, once doing that I got confused by the request for 4 spaces. Should the second line of text be indented another level?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@bnoordhuis bnoordhuis Dec 1, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you test some other values as well?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@bnoordhuis bnoordhuis Dec 1, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please use assert.throws() instead of try/catch. This should fail but won't when http.request doesn't throw an exception.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 1, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍 definitely

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
cjihrig
cjihrig requested changes Dec 1, 2016
View reviewed changes
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@cjihrig cjihrig Dec 1, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you use const instead of var in this file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@cjihrig cjihrig Dec 1, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you move the + to the previous line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@brad-decker

brad-decker commented Dec 2, 2016

Copy link
Copy Markdown
Contributor Author

@cjihrig @bnoordhuis - Thanks for your feedback. I've addressed most of your remarks. I made some further changes to the logic inside of request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brad-decker

brad-decker commented Dec 2, 2016

Copy link
Copy Markdown
Contributor Author

Will squash the commits down prior to merge once approved and passing CI.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig requested changes Dec 3, 2016
View reviewed changes
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@cjihrig cjihrig Dec 3, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you wrap this callback in common.mustCall().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@cjihrig cjihrig Dec 3, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This line, and the following line, should be indented more.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@cjihrig cjihrig Dec 3, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Instead of using a timer (which tends to be flaky), and counting requests received, can you use common.mustCall() with it's second option to define how many times the server request handler is run.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig Dec 3, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

That would make the process.on('exit', ...) handler unnecessary as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 3, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@cjihrig - what is the correct way to .close() the server with this kind of test. Ideally we wouldn't close it just after request x in case for some awful reason x + 1 request would have erroneously occurred which would need to be caught?

I've been looking in some of the other http type tests but all seem to implement something different.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 3, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Also 👍 on using the .mustCall function. I'll read through common.js in the future for test helpers.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig Dec 3, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I guess you might still need the counter to close the server. You might be able to get away with just closing the server after verifying that no exception was thrown, but that probably isn't a good idea. You could also use something like Promise.all() I suppose.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 3, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Got a solution up that i think will work, if you could re-review i would appreciate it. Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mscdex
mscdex reviewed Dec 3, 2016
View reviewed changes
Comment thread lib/_http_client.js Outdated

@mscdex mscdex Dec 3, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unnecessary/unrelated change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 3, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Addressed in a new commit and squashed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mscdex
mscdex reviewed Dec 3, 2016
View reviewed changes
Comment thread lib/_http_client.js Outdated

@mscdex mscdex Dec 3, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unnecessary/unrelated change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 3, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Addressed in a new commit and squashed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@brad-decker
brad-decker force-pushed the handle-agent-input branch from 40b750e to 1bbfa4c Compare December 3, 2016 21:39
cjihrig
cjihrig reviewed Dec 4, 2016
View reviewed changes
Comment thread lib/_http_client.js Outdated

@cjihrig cjihrig Dec 4, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Instead of duplicating all the checks on line 34, couldn't you just add:

} else if (!(agent instanceof Agent.Agent)) {
  throw new TypeError(...);
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 4, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's possible for agent to be null at this point, so I could do an else if here and check that it's not null and that it's not an instance of Agent.Agent.

I went this route as it verifies that we are dealing with appropriate types before this if statement and because the else if logic seemed slightly verbose.

I am okay with putting the extra checks on the else if statement though if it's preferred.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think it's worth mixing into this logic in order to avoid duplicating all the checks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 5, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@cjihrig how about this logic structure:

if (agent === false) {
   // noop
} else if (agent == null) {
   if (typeof options.createConnection !== 'function') {
     // noop
   }
} else if (!(agent instanceof Agent.Agent)) {
  // noop throw error.
}

if agent is undefined or null it'll be cause by the second logic gate, and if createConnection is a function it will not do anything, and will skip the final check. The last else if is exactly as you defined earlier. Obviously the code snippet has implementation removed just for brevity.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig Dec 5, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Seems like that would hit all of the cases.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 6, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just pushed that change, all tests passing locally 👍 thanks for the help @cjihrig

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
cjihrig
cjihrig reviewed Dec 4, 2016
View reviewed changes
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@cjihrig cjihrig Dec 4, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Couldn't you just increment numberOfRequests here, and remove the closeServer variable. Also, can you move server.close() to a new line.

EDIT: You shouldn't really need to track the number of failures, since they should be captured by assert.throws().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 4, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

numberOfRequests is counting the number of requests being initiated. Some of them are actually not actually hitting the server as they fail.

Definitely can move the server.close to new line

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 4, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@cjihrig addressed this in a new commit. Tracking now just the number of responses to close the server after receiving the expected number. Thanks

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@brad-decker
brad-decker force-pushed the handle-agent-input branch from 1bbfa4c to bace99e Compare December 4, 2016 15:33
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 5, 2016
@jasnell

jasnell commented Dec 5, 2016

Copy link
Copy Markdown
Member

Marking semver-major because of the added throw
brad-decker reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brad-decker
brad-decker force-pushed the handle-agent-input branch from bace99e to c0f4d20 Compare December 6, 2016 14:33
@brad-decker

brad-decker commented Dec 6, 2016

Copy link
Copy Markdown
Contributor Author

@bnoordhuis at your convenience would you mind taking a gander at this again?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig approved these changes Dec 6, 2016
View reviewed changes
sam-github
sam-github reviewed Dec 6, 2016
View reviewed changes
Comment thread lib/_http_client.js Outdated

@sam-github sam-github Dec 6, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

unrelated whitespace change

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sam-github
sam-github reviewed Dec 6, 2016
View reviewed changes
Comment thread lib/_http_client.js Outdated

@sam-github sam-github Dec 6, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

unrelated refactor. including purely stylistic changes in the same commit as a functional change makes PRs harder to review, and your commit message doesn't mention or justify these changes. These should be two commits, the addition of the else if (!(agent instanceof Agent.Agent)) { is the functional change, the other commit would be the style changes (though I would drop the addition of the random whitespace line, that's just code churn).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 6, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@sam-github i can actually not change this else if statement/block at all if i also check that agent is != null on my new else if (the functional change you mentioned). Other reviewers said that we should avoid duplicating the checks. What would be best in this case? I can split it into two commits as well just want to make sure i'm following best practices.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sam-github sam-github Dec 6, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I misread the code, I see now, you pulled the check on createConnection out of the conditional, so that if it is a function, no check is made on the type of agent. Code like this is a bit hard to read, I suggest just adding a comment between line 39 and 40 below, making it clear that this case is explicitly being dropped through - that agent can be == null if there is a createConnection function, that would have caused me to not misread.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brad-decker reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@brad-decker brad-decker Dec 7, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@sam-github at your convenience could you check the comment. Wasn't too sure on the wording so it seems a little verbose to me given i was struggling on explaining the case. I'm open to rewrite. Thanks again for your feedback.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sam-github
sam-github reviewed Dec 6, 2016
View reviewed changes
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@sam-github sam-github Dec 6, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

would be good to inject a couple other js types here: Function, Number, Symbol

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 7, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

added those types

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sam-github
sam-github reviewed Dec 6, 2016
View reviewed changes
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@sam-github sam-github Dec 6, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

random whitespace - you have single line between global scope vars everywhere but here

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@brad-decker
brad-decker force-pushed the handle-agent-input branch from c0f4d20 to 81d4f43 Compare December 7, 2016 02:49
jasnell
jasnell reviewed Dec 7, 2016
View reviewed changes
Comment thread lib/_http_client.js Outdated

@jasnell jasnell Dec 7, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please use (agent === null) (strict equals)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sam-github sam-github Dec 7, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@jasnell it looks to me that it is required to be non-strict to match undefined and null

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Dec 7, 2016

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@sam-github @jasnell its non strict to catch both types but i did take the liberty of making that change whereas original version strictly checked for both null and undefined. So i'm flexible on implementation here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell reviewed Dec 7, 2016
View reviewed changes
Comment thread lib/_http_client.js Outdated

@jasnell jasnell Dec 7, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please use TypeError here

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell reviewed Dec 7, 2016
View reviewed changes
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@jasnell jasnell Dec 7, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

instead of a function here, just use a regexp... e.g.

assert.throws(() => createRequest(agent),
  /^TypeError: Agent option must be an instance of http.Agent/);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell reviewed Dec 7, 2016
View reviewed changes
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@jasnell jasnell Dec 7, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

TypeError

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bnoordhuis bnoordhuis Dec 12, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@brad-decker What I mean is that you can simply test assert.doesNotThrow(() => createRequest(agent)), there is no need to check for error types.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell requested changes Dec 7, 2016
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Almost there... just a few bits remaining

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
sam-github
sam-github reviewed Dec 7, 2016
View reviewed changes
Comment thread lib/_http_client.js Outdated

@sam-github sam-github Dec 7, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This line just restates the code, I would drop it, and capitalize the "Explicitly" on the next sentence, so its clear that not handling this case is intentional.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sam-github

sam-github commented Dec 7, 2016

Copy link
Copy Markdown
Contributor

Other than the one line of the comment that I would drop, LGTM
brad-decker reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brad-decker
brad-decker force-pushed the handle-agent-input branch from 81d4f43 to 0229c2d Compare December 8, 2016 01:04
@brad-decker

brad-decker commented Dec 13, 2016

Copy link
Copy Markdown
Contributor Author

@bnoordhuis fixed the last bit of feedback you had, thanks again for reviewing and my apologies for the delay!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis approved these changes Dec 19, 2016
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, thanks. CI: https://ci.nodejs.org/job/node-test-pull-request/5473/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@brad-decker

brad-decker commented Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

@bnoordhuis - Can you point me in the direction of any resources that might help me figure out the CI testing process so i can address issues with the /arm and /linux tests? Would love to become more familiar with this process so i can be self sufficient here. As it stands i'm not quite sure what has failed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Dec 19, 2016

Copy link
Copy Markdown
Contributor

This is the only failure in the CI run - https://ci.nodejs.org/job/node-test-commit-linux/6776/nodes=ubuntu1610-x64/console (search for "not ok"). It doesn't appear to be related to the changes in this PR.
brad-decker reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brad-decker

brad-decker commented Dec 19, 2016

Copy link
Copy Markdown
Contributor Author

Cool. Thanks @cjihrig - ill wait for feedback that specifies otherwise prior to delving into this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex mscdex removed the lts-watch-v6.x label Dec 21, 2016
jasnell
jasnell requested changes Jan 11, 2017
View reviewed changes
Comment thread lib/_http_client.js Outdated

@jasnell jasnell Jan 11, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm really not that fond of the Received type: ... additional bits here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brad-decker brad-decker Jan 14, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@jasnell - sorry for the delay. Addressed your feedback and removed the extra bits.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@brad-decker
brad-decker force-pushed the handle-agent-input branch from 67b9eda to 86f373e Compare January 14, 2017 13:48
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Jan 14, 2017
jasnell
jasnell approved these changes Jan 15, 2017
View reviewed changes
@jasnell

jasnell commented Jan 15, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/5873/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@italoacasas

italoacasas commented Jan 17, 2017

Copy link
Copy Markdown

@brad-decker can you run make lint and fix the error?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brad-decker

brad-decker commented Jan 17, 2017

Copy link
Copy Markdown
Contributor Author

@italoacasas i don't get any errors when running make lint. I also examined the output of the linter failure in CI and it seems to be a build error rather than a true lint error? I may be wrong i'm pretty new to this whole contributing to node thing.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn

gibfahn commented Jan 17, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@brad-decker I'm getting an error when I patch your PR onto master, could you rebase and see if you get an error?

➜  curl -L https://github.com/nodejs/node/pull/10053.patch | git am --whitespace=fix"
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   139    0   139    0     0    341      0 --:--:-- --:--:-- --:--:--   342
100  3369    0  3369    0     0   3770      0 --:--:-- --:--:-- --:--:-- 24237
Applying: http: throw an error for unexpected agent values
➜  node git:(master) make lint
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules \
	  benchmark lib test tools

/Users/gib/wrk/com/node/test/parallel/test-http-client-reject-unexpected-agent.js
  52:8  error  Expected indentation of 6 spaces but found 7  indent

✖ 1 problem (1 error, 0 warnings)

make: *** [jslint] Error 1
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gibfahn
gibfahn reviewed Jan 17, 2017
View reviewed changes
Comment thread test/parallel/test-http-client-reject-unexpected-agent.js Outdated

@gibfahn gibfahn Jan 17, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks like one too many spaces on this line

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@brad-decker
http: throw an error for unexpected agent values
25cd753 
As per nodejs#9069 unexpected things can happen
when supplying an unexpected value to agent.
Beings as the docs clearly state the expected
values, this throws an error on an unexpected
value.

Signed-off-by: brad-decker <[email protected]>
@brad-decker

brad-decker commented Jan 17, 2017

Copy link
Copy Markdown
Contributor Author

@italoacasas @gibfahn I see what i did. i rebased on origin and not upstream :P got the lint error after rebasing. Sorry about that. fixed now
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brad-decker
brad-decker force-pushed the handle-agent-input branch from 86f373e to 25cd753 Compare January 17, 2017 19:09
@nodejs-github-bot nodejs-github-bot added the lts-watch-v6.x label Jan 17, 2017
@italoacasas

italoacasas commented Jan 18, 2017

Copy link
Copy Markdown

thanks @brad-decker

new CI: https://ci.nodejs.org/job/node-test-pull-request/5919/console
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn

gibfahn commented Jan 18, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

CI is green (ignore the test/arm reported failure)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@italoacasas

italoacasas commented Jan 18, 2017

Copy link
Copy Markdown

Landed fc7025c
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@italoacasas italoacasas closed this Jan 18, 2017
italoacasas pushed a commit that referenced this pull request Jan 18, 2017
@brad-decker Italo A. Casas
http: throw an error for unexpected agent values
fc7025c 
As per #9069
unexpected things can happen when supplying an
unexpected value to agent. Beings as the docs
clearly state the expected values, this throws
an error on an unexpected value.

PR-URL: #10053
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@brad-decker

brad-decker commented Jan 18, 2017

Copy link
Copy Markdown
Contributor Author

Thanks everyone!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@brad-decker Italo A. Casas
http: throw an error for unexpected agent values
b8527b3 
As per nodejs#9069
unexpected things can happen when supplying an
unexpected value to agent. Beings as the docs
clearly state the expected values, this throws
an error on an unexpected value.

PR-URL: nodejs#10053
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@lance lance mentioned this pull request Jan 19, 2017
http core module issue #9069
Closed
fengmk2
fengmk2 reviewed Feb 26, 2017
View reviewed changes
Comment thread lib/_http_client.js
}
// Explicitly pass through this statement as agent will not be used
// when createConnection is provided.
} else if (!(agent instanceof Agent.Agent)) {

@fengmk2 fengmk2 Feb 26, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

How about userland Agent? Those Agent Class maybe not inherits from Agent.Agent and now will be all fails to use them. e.g.: TunnelingAgent https://github.com/request/tunnel-agent/blob/master/index.js#L47

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@fengmk2 fengmk2 mentioned this pull request Feb 26, 2017
http: should support userland Agent #11567
Closed
2 of 4 tasks
@jasnell jasnell mentioned this pull request Apr 4, 2017
8.0.0 Release Proposal #12220
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes
@jasnell jasnell jasnell approved these changes
@cjihrig cjihrig cjihrig approved these changes
+4 more reviewers
@sam-github sam-github sam-github left review comments
@mscdex mscdex mscdex left review comments
@fengmk2 fengmk2 fengmk2 left review comments
@gibfahn gibfahn gibfahn left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

http Issues and PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@brad-decker @jasnell @sam-github @cjihrig @italoacasas @gibfahn @mscdex @fengmk2 @bnoordhuis @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.