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: skip process-kill-null if cat unavailable #16228

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Oct 16, 2017

If the cat command is not available test-process-kill-null.js will
report the following error:

internal/process.js:160
      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'pid', 'Number');
      ^

TypeError [ERR_INVALID_ARG_TYPE]: The "pid" argument must be of type Number
    at process.kill (internal/process.js:160:13)
    at Object.<anonymous> (C:\working\node\test\parallel\test-process-kill-null.js:31:19)
    at Module._compile (module.js:604:30)
    at Object.Module._extensions..js (module.js:615:10)
    at Module.load (module.js:523:32)
    at tryModuleLoad (module.js:486:12)
    at Function.Module._load (module.js:478:3)
    at Function.Module.runMain (module.js:645:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:605:3

I'm not sure if I have my windows environment configured correctly (I'm
using Windows 10 Pro) but I get this error. The CI servers don't report
it but perhaps they have some configuration that I don't. I wanted to
raise this to be sure.

This commit suggests to skip this test if the cat command is not
available on the operating system.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test, windows

If the cat command is not available test-process-kill-null.js will
report the following error:

internal/process.js:160
      throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'pid',
'Number');
      ^

TypeError [ERR_INVALID_ARG_TYPE]: The "pid" argument must be of type
Number
    at process.kill (internal/process.js:160:13)
    at Object.<anonymous>
(C:\working\node\test\parallel\test-process-kill-null.js:31:19)
    at Module._compile (module.js:604:30)
    at Object.Module._extensions..js (module.js:615:10)
    at Module.load (module.js:523:32)
    at tryModuleLoad (module.js:486:12)
    at Function.Module._load (module.js:478:3)
    at Function.Module.runMain (module.js:645:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:605:3

I'm not sure if I have my windows environment configured correctly (I'm
using Windows 10 Pro) but I get this error. The CI servers don't report
it but perhaps they have some configuration that I don't. I wanted to
raise this to be sure.

This commit suggests to skip this test if the cat command is not
available on the operating system.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 16, 2017
@danbev
Copy link
Contributor Author

danbev commented Oct 16, 2017

@joyeecheung
Copy link
Member

Maybe a better way to fix this is to find an alternative to cat on windows?

@danbev
Copy link
Contributor Author

danbev commented Oct 16, 2017

Maybe a better way to fix this is to find an alternative to cat on windows?

Sounds good, but I'm not sure what would be a good replacement command to use on windows. I'll dig around but if anyone has some suggestions they would be welcome?

@vsemozhetbyt
Copy link
Contributor

cat is expected to be available as per our prerequisites:

Basic Unix tools required for some tests, Git for Windows includes Git Bash and tools which can be included in the global PATH.

These 7 tests also use cat without skipping on Windows. Do they throw for you as well?

Links:

const cmd = 'echo bar | cat';

spawnSync('cat', [], options);

const cmd = 'echo bar | cat';

const cat = spawn('cat');

spawn('cat', [], { stdio: 'inherit' });

child = spawnSync('cat', [], options);

const cat = cp.spawn('cat', [filename]);


@vsemozhetbyt
Copy link
Contributor

See also #11469 and #11953

@danbev
Copy link
Contributor Author

danbev commented Oct 16, 2017

cat is expected to be available as per our prerequisites:

Sorry about that, I'd missed adding these to my path. Thanks for pointing them out.

I'm rerunning the test locally now and hopefully I'll close this PR if all goes well. Sorry about the noise.

@danbev
Copy link
Contributor Author

danbev commented Oct 16, 2017

@joyeecheung @vsemozhetbyt Closing this as using git-bash and updating my path did the trick and all test pass except one. I'll take a closer look at it. Thanks for your help!

=== release test-graph.pipeconnect ===
Path: async-hooks/test-graph.pipeconnect
fs.js:913
  return binding.mkdir(pathModule.toNamespacedPath(path),
                 ^

Error: EPERM: operation not permitted, mkdir 'C:\Users\danbev\working\node\test\tmp.0'
    at Object.fs.mkdirSync (fs.js:913:18)
    at Object.exports.refreshTmpDir (C:\Users\danbev\working\node\test\common\index.js:159:6)
    at Object.<anonymous> (C:\Users\danbev\working\node\test\async-hooks\test-graph.pipeconnect.js:9:8)
    at Module._compile (module.js:604:30)
    at Object.Module._extensions..js (module.js:615:10)
    at Module.load (module.js:523:32)
    at tryModuleLoad (module.js:486:12)
    at Function.Module._load (module.js:478:3)
    at Function.Module.runMain (module.js:645:10)
    at startup (bootstrap_node.js:187:16)
Command: C:\Users\danbev\working\node\Release\node.exe C:\Users\danbev\working\node\test\async-hooks\test-graph.pipeconnect.js

@cjihrig
Copy link
Contributor

cjihrig commented Oct 16, 2017

@danbev is it ok to close this then?

@danbev danbev closed this Oct 16, 2017
@danbev
Copy link
Contributor Author

danbev commented Oct 16, 2017

@cjihrig Yeah, I just hit the wrong button 😞 . Thanks

@danbev danbev deleted the process-kill-null-skip branch November 23, 2017 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants