test: improve net-immediate-finish-test

[refack]

this tests need to fail, it's more "sanitary" to run it in /sequential/
added comment why it's ok to use common.PORT

Ref: #12996
Ref: #12376
Ref: #12950
Ref: #12951
Ref: #13055
Ref: #12964

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added (inline comment in test)
• commit message follows commit guidelines

Affected core subsystem(s)

test,net

[refack]

Continuation of #12996
/cc @arturgvieira @Trott @gibfahn @addaleax

[arturgvieira-zz]

continued.... Thanks for letting me know, I am still working on timing on these PRs, I've only been at it for a week.

[mscdex]

I don't really see the value in the code changes. Moving the test is fine however.

[refack]

I don't really see the value in the code changes. Moving the test is fine however.

Just literal coding, trying to convey the rational.
Actually I'll also add a comment.

[refack]

Stress:
https://ci.nodejs.org/job/node-stress-single-test/nodes=freebsd10-64/1222/
https://ci.nodejs.org/job/node-stress-single-test/1223/nodes=win2016/

[Trott]

Why the move to sequential?

It never actually tries to make a connection to the port or use it in any way.

The hostname lookup fails first and that throws an error, by design.

I'm not sure what is meant by "sanitary" here unless the idea is that should *** somehow get mapped to localhost, then it will could cause problems with other tests? I don't think that's worth worrying about and could be dealt with by hardcoding the port value to some other arbitrary value. Code could even look like this:

// The port is not used but should be a valid value to avoid port-based errors.
// We want the invalid hostname to trigger an error instead.
const thisPortIsNotActuallyUsed = 6767;

[refack]

@Trott it's a case of "safe than sorry"... Since the test tries to make a connection to a non existing server it reminded me of #12964. I agree that it very very highly unlikely that ***:6767 will be resolvable, but 🤷‍♂️ I donno.
It was your comment #12950 (comment) that made me rethink about even a tiny benefit (what I called hygiene) to be worth the move.

If nobody likes this I'll reduce the change to your snippet.

P.S. the placing of the client.close() gives me pause... Is it even necessary?

[jasnell]

Nit: in general, please capitalize the first word of comments :-)

[refack]

ack

[refack]

Quick sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/6607/
CI: https://ci.nodejs.org/job/node-test-pull-request/8641/

[BridgeAR]

There was no update here for a long time and to me it seems this has no high likelihood of getting merged. Closing it therefore. @refack please reopen if you would like to have another look at this.