Skip to content
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

Closed

Conversation

grantgasp
Copy link
Contributor

Add value to assertion message to test-cluster-fork-env.js test

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

Test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 6, 2017
@mscdex mscdex added the cluster Issues and PRs related to the cluster subsystem. label Nov 6, 2017
assert.ok(
checks.overwrite,
'The custom environment did not overwrite the existing environment.');
'The custom environment did not overwrite the existing environment.' +
`${checks.overwrite}`);
Copy link
Contributor

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}`);

@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2017
@vsemozhetbyt
Copy link
Contributor

Copy link
Member

@Trott Trott left a 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.

@gireeshpunathil
Copy link
Member

Landed in 707e71c , thanks for the contribution!

gireeshpunathil pushed a commit that referenced this pull request Nov 13, 2017
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>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
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>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
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>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
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>
@tniessen tniessen changed the title test: add value to assertion message test: add a test description for test/parallel/test-cluster-fork-env.js Nov 18, 2017
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
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>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants