Please remove the console.log.

doh, thats embarassing. Will remove.

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

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.

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.

cool.

@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?

Can you test some other values as well?

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

👍 definitely

Can you use const instead of var in this file.

Can you move the + to the previous line.

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

This line, and the following line, should be indented 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.

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

@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.

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

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.

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

Unnecessary/unrelated change.

Addressed in a new commit and squashed

Unnecessary/unrelated change.

Addressed in a new commit and squashed

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

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

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.

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

@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.

Seems like that would hit all of the cases.

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

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().

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

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

unrelated whitespace change

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).

@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.

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.

@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.

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

added those types

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

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

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

@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.

Please use TypeError here

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/);

TypeError

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

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.

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

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

Looks like one too many spaces on this line

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