doc: Recommend using port 0 instead of common.PORT #8694
Conversation
In the 'writing_tests' guide it is recommended to use common.PORT instead of an arbitrary value, but the recommendation is to use port 0 instead and the docs should reflect that.
|
You beat me to it, was a couple of minute away from sending a PR for this, thanks! |
| using. | ||
| - The use of 0 as the listening port. Always use 0 instead of using an | ||
| arbitrary value, as it allows to run tests in parallel safely, as the | ||
| operating system will assign a random port. |
There was a problem hiding this comment.
We could add something explaining that this is true, unless the test is specifically checking that assigning a specific port works as expected.
|
LGTM with @santigimeno's suggestion. |
|
For example test being modified in PR #8586 is exception to this. As @santigimeno pointed out, some tests need to test assigning specific port. |
| instead of using an arbitrary value, as it allows to run tests in parallel | ||
| safely, as they are not trying to reuse the same port another test is already | ||
| using. | ||
| - The use of 0 as the listening port. Always use 0 instead of using an |
There was a problem hiding this comment.
NIt: The first sentence isn't really necessary. This paragraph can start with Always use...
|
(Also: I agree with @santigimeno's comment!) |
|
LGTM subject to comments already made. |
| arbitrary value, as it allows to run tests in parallel safely, as the | ||
| operating system will assign a random port. | ||
| - If the test doesn't depend on a specific port number then always use 0 instead | ||
| of an arbitrary value, as it allows to run tests in parallel safely, as the |
There was a problem hiding this comment.
as it allows tests to be run in parallel safely reads a little bit cleaner I think.
| of an arbitrary value, as it allows to run tests in parallel safely, as the | ||
| operating system will assign a random port. If the test requires a specific | ||
| port, for example if the test checks that assigning a specific port works as | ||
| expected, then it is ok to assign a specific port number. |
There was a problem hiding this comment.
Shouldn't we suggest using common.PORT in any cases?
|
Could someone provide some background on this? I'm a little confused about it. Should we get rid of |
It was used to ensure that, but it was failing to do so. Sometimes a test would crash and the port would remain in use, causing |
I think this can be landed. I have some nits. (The first sentence in the paragraph that's added is a bit of a run-on sentence. Split it into two sentences, maybe? And the last sentence in the paragraph should probably mention |
|
Landing:
|
In the 'writing_tests' guide it is recommended to use common.PORT instead of an arbitrary value, but the recommendation is to use port 0 instead and the docs should reflect that. PR-URL: nodejs#8694 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
In the 'writing_tests' guide it is recommended to use common.PORT instead of an arbitrary value, but the recommendation is to use port 0 instead and the docs should reflect that. PR-URL: #8694 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
In the 'writing_tests' guide it is recommended to use common.PORT instead of an arbitrary value, but the recommendation is to use port 0 instead and the docs should reflect that. PR-URL: #8694 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Checklist
Affected core subsystem(s)
doc
Description of change
In the writing_tests guide it is recommended to use common.PORT
instead of an arbitrary value, but the recommendation is to use
port 0 instead and the docs should reflect that.