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

add test and docs update for detached fork process #24524

Closed
wants to merge 1 commit into from

Conversation

@nanomosfet
Copy link
Contributor

commented Nov 20, 2018

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

This addresses issue #17592

doc/api/child_process.md Outdated Show resolved Hide resolved
@@ -0,0 +1,44 @@
// Copyright Joyent, Inc. and other Node contributors.

This comment has been minimized.

Copy link
@lpinca

lpinca Nov 20, 2018

Member

AFAIK this copyright is not needed for new tests.

This comment has been minimized.

Copy link
@nanomosfet

nanomosfet Nov 20, 2018

Author Contributor

Will remove it. Thanks for letting me know.

@codegagan
Copy link
Contributor

left a comment

I would suggest to use arrow functions wherever possible in file test/fixtures/parent-process-nonpersistent-fork.js

@lpinca
lpinca approved these changes Nov 20, 2018
Copy link
Member

left a comment

SGTM with nits addressed.

@nanomosfet

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

@codegagan Thanks for the suggestion. I was following how they declared anonymous in other tests.

Also do you mean test/parallel/test-child-process-fork-detached.js? The file you mentioned does not have any functions. I prefer arrow functions as well. Should I change those anonymous functions in my main test file?

@nanomosfet nanomosfet force-pushed the nanomosfet:add-fork-detached-test branch from 2b18e03 to 30c5b2d Nov 20, 2018

@vsemozhetbyt

This comment was marked as resolved.

Copy link
Contributor

commented Nov 20, 2018

@nanomosfet I am sorry I did not notice this at first, but it seems we will have some linting issues with doc format and commit message format.

  1. Long lines in docs are wrapped at 80 characters maximum (with indent preserved).
  2. At least the first commit's title and message need to conform with this guideline.

With these nits fixed, we will be ready to run CI tests.

@nanomosfet nanomosfet force-pushed the nanomosfet:add-fork-detached-test branch from 30c5b2d to d58c8ac Nov 20, 2018

@nanomosfet

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

@vsemozhetbyt Thanks for catching that.

I updated the commit message. I couldn't tell if I should use test or docs for the sub system.

I also changed the docs so that is is less than 80 characters with indent preserved.
Let me know what else I can do. :)

@vsemozhetbyt

This comment has been minimized.

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

cc @nodejs/child_process

@nanomosfet

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

@vsemozhetbyt Looks like two of the checks failed. Not really sure what to make out of the details of the build. What is going on?

@codegagan
Copy link
Contributor

left a comment

@nanomosfet you can look at this test file which has used arrow functions

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

1 Windows job failure may be unrelated. Let's rerun just this job:
https://ci.nodejs.org/job/node-test-commit-windows-fanned/22587/

@nanomosfet

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

@vsemozhetbyt Nice it looks like that one passed. Is that a flaky test?

Thank you BTW

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

Flaky tests are usually marked as flaky in CI logs, but maybe we do not know yet that this one is flaky)

@nanomosfet

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

@vsemozhetbyt I see. What is left to do so we can merge?

I can change the anon. functions to arrow functions but I am afraid to because of rerunning the CI tests again. I can always make another PR to update a few test with arrow functions. I have noticed a lot of PR's where they are updating tests to use arrow functions.

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

You can do this in another PR or just leave it for one of the Code-and-learn sessions (most of these latest PRs are from such session).

Our rules require at least 48 hours for the review process and 2 approvals. So, for now, we just need to wait a bit)

@@ -325,6 +325,8 @@ changes:
* `args` {string[]} List of string arguments.
* `options` {Object}
* `cwd` {string} Current working directory of the child process.
* `detached` {boolean} If `true`, prepares child to run

This comment has been minimized.

Copy link
@Trott

Trott Nov 24, 2018

Member

Probably better to duplicate the text that exists for the detached option under spawn() rather than have two different phrasings like this?

This comment has been minimized.

Copy link
@nanomosfet

nanomosfet Nov 24, 2018

Author Contributor

Yeah I'd agree. It's almost the same though. I think the difference is just the If true. Should I go ahead make that change?

process.on('exit', function() {
assert.notStrictEqual(childId, -1);
// Killing the child process should not throw an error
process.kill(childId);

This comment has been minimized.

Copy link
@Trott

Trott Nov 24, 2018

Member

This strikes me as having significant potential for leaving stray processes around on CI. Not necessarily blocking, but surely something to keep an eye on after this lands....

This comment has been minimized.

Copy link
@nanomosfet

nanomosfet Nov 24, 2018

Author Contributor

Yeah I totally agree. One of my checks was to make sure that no node processes were running after I ran the test individually.

@Trott

This comment has been minimized.

Copy link
Member

commented Nov 24, 2018

I would prefer either leaving the functions as they are, or else changing them here. Changing them to arrow functions in a separate PR is just churn for no good reason. Again, not blocking. Just my own preference.

test: test and docs for detached fork process
This tests child process fork component in detached mode
by spawning a parent process that creates a child process.
We kill the parent process and check if the child is still
running.

Fixes: #17592

@nanomosfet nanomosfet force-pushed the nanomosfet:add-fork-detached-test branch from d58c8ac to f1f4837 Nov 24, 2018

@nanomosfet

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2018

@codegagan Updated PR to use arrow functions
@vsemozhetbyt fixed typo

nit: nonPersistantNode -> nonPersistentNode here and below.

@Trott - fork detached docs now match spwan detached. Both are now

 * `detached` {boolean} Prepare child to run independently of its parent
    process. Specific behavior depends on the platform, see
    [`options.detached`][]).

Re ran test and no stray node processes

Let me know what else I can do. Thank you for your time <3 :)

Edit: also rebased to tip of nodejs/node

@vsemozhetbyt

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Nov 24, 2018

Looks good to me. Can someone in @nodejs/child_process (or someone else knowledgable about potential pitfalls here) take a look?

Trott added a commit to Trott/io.js that referenced this pull request Nov 28, 2018
test: test and docs for detached fork process
This tests child process fork component in detached mode
by spawning a parent process that creates a child process.
We kill the parent process and check if the child is still
running.

Fixes: nodejs#17592

PR-URL: nodejs#24524
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

Landed in f051737.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@Trott Trott closed this Nov 28, 2018

targos added a commit that referenced this pull request Nov 29, 2018
test: test and docs for detached fork process
This tests child process fork component in detached mode
by spawning a parent process that creates a child process.
We kill the parent process and check if the child is still
running.

Fixes: #17592

PR-URL: #24524
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR BridgeAR referenced this pull request Dec 5, 2018
4 of 4 tasks complete
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
test: test and docs for detached fork process
This tests child process fork component in detached mode
by spawning a parent process that creates a child process.
We kill the parent process and check if the child is still
running.

Fixes: nodejs#17592

PR-URL: nodejs#24524
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs added a commit that referenced this pull request Feb 12, 2019
test: test and docs for detached fork process
This tests child process fork component in detached mode
by spawning a parent process that creates a child process.
We kill the parent process and check if the child is still
running.

Fixes: #17592

PR-URL: #24524
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BethGriggs BethGriggs referenced this pull request Feb 12, 2019
rvagg added a commit that referenced this pull request Feb 28, 2019
test: test and docs for detached fork process
This tests child process fork component in detached mode
by spawning a parent process that creates a child process.
We kill the parent process and check if the child is still
running.

Fixes: #17592

PR-URL: #24524
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.