test: replace port in net immediate finish test

[arturgvieira-zz]

Replaced common.PORT in the following test.
test-net-connect-immediate-finish.js

Ref: #12376

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

test

[refack]

It's a good try, but IMHO it's too arbitrary, you could just use 0.

A (controversial) other way is to copy what we did in 12964 but only do that as an exercise!

[arturgvieira-zz]

@refack Sounds good, I'll have the changes made in a quick update.

[arturgvieira-zz]

All done, updated the port to 0 (zero) and made the same update to the comment.

[addaleax]

I hate to say it, but I don’t think 0 is a better choice than 8080. 0 is not a valid port to connect to, but 8080 and common.PORT are – I think your original approach of using a fixed but valid port is better (or at least more readable) here.

[gibfahn]

Does connecting to an invalid port raise an error? If so that might work well, that way if the connection does happen the test will fail.

[refack]

@arturgvieira after the long discussion we had in #12964 (also about a test that should immediately fail), I think best solution is to keep the use of common.PORT and move the test to /sequential/.

Maybe next PR should actually merge this test with https://github.com/nodejs/node/blob/master/test/parallel/test-net-connect-local-error.js. that one tests for an error in the port and this one in the hostname

[arturgvieira-zz]

@refack Sounds good, I will revert the changes and move this test to sequential.

[refack]

@arturgvieira I just landed #12964 IMHO you can copy the old test code into test/sequential/test-net-connect-local-error.js. Unless the others object.

[refack]

moving to /sequential/

[refack]

Declare a const unfindable_host = '***'

[arturgvieira-zz]

All done, I replaced the host string literal with a constant, reverted the changes so that this test uses common.PORT and moved the test to sequential.

[arturgvieira-zz]

@refack You want to merge this test with test/sequential/test-net-connect-local-error.js correct?

[refack]

I approved the important changes. About merging, let's get a second opinion.
/cc @Trott @addaleax @nodejs/testing

[arturgvieira-zz]

Sounds good.

[Trott]

There is no need to move this to sequential.

[arturgvieira-zz]

Ok, I reverted that last change.

[Trott]

Port 0 is not OK for this test for the reasons outlined by @addaleax.

[Trott]

I think this test may not need any changes at all. It's fine as is. The only change it might need is a comment explaining why using common.PORT here is actually OK. (Or change it to a hard coded value like 8080 and add that comment.)

[arturgvieira-zz]

Sounds good. I'll close the PR, thanks for the review.

[refack]

IMHO you closed too fast, I've revived it in #13062
As we've seen tests fail because of cross-talk (#12964, #12950, #12951, #13055), so since this test needs to not connect, IMHO it's more "sanitary" to run it in /sequential/ (plus it uses common.PORT)