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: improve child_process test coverage #11278

Closed
wants to merge 1 commit into from

Conversation

chiiia12
Copy link

This PR improves child_process coverage (maybe cover statement 100%).
This is my first contribution, please review.

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

N/A

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 10, 2017
@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Feb 10, 2017
@Trott
Copy link
Member

Trott commented Feb 10, 2017

@nodejs/testing

@cjihrig
Copy link
Contributor

cjihrig commented Feb 10, 2017

I'm not sure that the changes to test-child-process-exec-timeout.js and test-child-process-spawn-typeerror.js actually buy us anything.

@yosuke-furukawa
Copy link
Member

@cjihrig

Currently, we are getting started to learn how to contribute node.js core in my company. I am teaching how-to-contribute and suggest contribution point. So I answered about that.

According to child_process coverage report ,
we found 3 uncovered blocks.

  1. child_process.fork option, stdio: ignore is not passed yet.
  2. child_process throws exception when kill signal is invalid.
  3. child_process.execFile can add arguments array but in current test the array is always empty.

so we just add those 3 tests.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 10, 2017

child_process throws exception when kill signal is invalid.

I don't think the tests in this PR address that. I have a PR for that in #11038. I haven't landed it yet because I think there is an issue somewhere with killing child processes on Windows.

child_process.execFile can add arguments array but in current test the array is always empty.

Again, I don't think this PR addresses that. The changes to test-child-process-spawn-typeerror.js call spawn(), not execFile().

@yosuke-furukawa
Copy link
Member

@cjihrig

I don't think the tests in this PR address that. I have a PR for that in #11038.

we are checking to add console.log at the lines.

  function kill() {
    if (child.stdout)
      child.stdout.destroy();

    if (child.stderr)
      child.stderr.destroy();

    killed = true;
    try {
      child.kill(options.killSignal);
    } catch (e) {
      console.log(`caught on the kill signal: ${options.killSignal} `, e);
      ex = e;
      exithandler();
    }
  }

we have got the message like that. so I think we can pass the blocks.

$ node test/parallel/test-child-process-exec-timeout.js
caught on the kill signal: SIGUNDEF  Error: Unknown signal: SIGUNDEF
    at ChildProcess.kill (internal/child_process.js:375:11)
    at kill (child_process.js:265:13)
    at Timeout.delayedKill [as _onTimeout] (child_process.js:275:7)
    at ontimeout (timers.js:365:14)
    at tryOnTimeout (timers.js:237:5)
    at Timer.listOnTimeout (timers.js:207:5)

However, your PR will also reach here. we don't have an opinion which PR is better. if your PR will be merged before this PR, we will remove this test.

Again, I don't think this PR addresses that. The changes to test-child-process-spawn-typeerror.js call spawn(), not execFile().

hmm. this one is our failure, we need to fix about that. We will fix this soon. thank you.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 11, 2017

we are checking to add console.log at the lines.

Apologies. There is currently no input validation on the kill signal for execFile(). Ideally, it should be using the validateKillSignal() function used by spawnSync(), but that causes some test failures. Some work is needed to improve consistency there.

EDIT: #10423 is relevant to that test, as it would eliminate the code coverage.

@chiiia12 chiiia12 force-pushed the test_improve_child_process branch 3 times, most recently from 6dd71eb to a27130b Compare February 13, 2017 08:44
@chiiia12
Copy link
Author

@cjihrig Thanks.I fixed it!

@yosuke-furukawa
Copy link
Member

@cjihrig
Yes, validateKillSignal for execFile is better. But the function changes would be better to discuss another PR. If we should wait for the function changes and #10423 , we can drop the undefined signal test. If you suggest so, we will drop.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 16, 2017

@yosuke-furukawa I would drop the changes in test-child-process-exec-timeout.js. Those checks will become obsolete in #10423.

The changes in test-child-process-ignore-stdio.js add the missing code coverage, but I don't think it actually tests the code it's covering. For example, I can change the stdio option to a valid option other than 'ignore', and the test still passes.

The changes in test-child-process-spawn-typeerror.js should be moved to a different test file IMO. Because the code you're adding coverage for updates the cmd variable, some assertions should be added for the error object's message and cmd properties at a minimum.

@chiiia12
Copy link
Author

@cjihrig Sounds reasonable.I remove it.

@fhinkel
Copy link
Member

fhinkel commented Mar 26, 2017

@chiiia12 Are you still working on this?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 26, 2017
@fhinkel
Copy link
Member

fhinkel commented May 26, 2017

There hasn't been any activity here. I'm closing this. Feel free to reopen (or ping a collaborator).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants