-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add a test description for test/parallel/test-cluster-fork-env.js #16833
Conversation
assert.ok( | ||
checks.overwrite, | ||
'The custom environment did not overwrite the existing environment.'); | ||
'The custom environment did not overwrite the existing environment.' + | ||
`${checks.overwrite}`); |
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 seems we are lacking spaces here and using unnecessary concatenation and redundant templates. What if we use something like this?
assert.ok(
checks.using,
`The worker did not receive the correct env. ${checks.using}`);
assert.ok(
checks.overwrite,
`The custom environment did not overwrite the existing environment. ${
checks.overwrite}`);
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.
Hi @grantgasp! Welcome and thanks for the PR!
Given that we can be certain the values checked are booleans, I'm not sure that adding this info to the messages is helpful. This is an error in the task I curated and not anything you did. I'm terribly sorry about that.
However, there are definitely things that can be done to improve this test.
I'd recommend removing the changes you did and instead adding a comment (below the declaration of common
) that indicates the purpose of the test. Something like "This test checks that arguments provided to cluster.fork()
will create new environment variables and override existing environment variables in the created worker process."
Would you mind doing that? I know it's not much of a change, but it's an improvement, and as a first contribution, it will allow you to see how our process works.
Landed in 707e71c , thanks for the contribution! |
PR-URL: #16833 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #16833 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #16833 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #16833 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #16833 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #16833 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Add value to assertion message to test-cluster-fork-env.js test
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
Test