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

strip .exe suffix via case-insensitive replacement (fixes #55 and #56) #57

Closed
wants to merge 4 commits into from
Closed

strip .exe suffix via case-insensitive replacement (fixes #55 and #56) #57

wants to merge 4 commits into from

Conversation

mykmelez
Copy link

Strip a '.exe' extension via a case-insensitive replacement rather than via the case-sensitive 'ext` parameter to path.basename because it might be capitalized ('.EXE'). This fixes #56 for me when testing https://github.com/mozilla/qbrt, which uses https://github.com/tapjs/node-tap, which uses https://github.com/istanbuljs/nyc, which uses this package.

Strip a '.exe' extension via a case-insensitive replacement rather than via the case-sensitive 'ext` parameter to path.basename because it might be capitalized ('.EXE').
@mykmelez
Copy link
Author

I'm not sure why three of the four Linux test runs failed. Each run seems to have failed a single test, but the test that failed is different in each case. Perhaps they're unrelated intermittent failures? I can't reproduce them locally, neither on Node 4.8.3 nor Node 6.11.0 (on my Ubuntu 16.04 system). I also can't reproduce on macOS.

The Windows failures are presumably the unrelated ones from #55.

@popomore
Copy link

I'm also faced the problem when I run coverage with nyc on Windows, see link below

https://ci.appveyor.com/project/eggjs/egg-mock/build/1.0.377/job/cp0k4bjh54qd6l2t

@popomore
Copy link

ping @isaacs

cc @bcoe

popomore added a commit to eggjs/bin that referenced this pull request Jun 20, 2017
popomore added a commit to eggjs/bin that referenced this pull request Jun 20, 2017
popomore added a commit to eggjs/bin that referenced this pull request Jun 20, 2017
@popomore
Copy link

@isaacs can you see this PR?

mykmelez added 3 commits July 2, 2017 20:43
Especially on Windows, where it might contain "Program Files".
Per <https://nodejs.org/api/process.html>, "Sending SIGINT, SIGTERM, and SIGKILL cause the unconditional termination of the target process."  So Node ignores our SIGTERM handler, and the process ends when killed with that signal.
@mykmelez
Copy link
Author

mykmelez commented Jul 3, 2017

@isaacs dbf243a and bfd4bae fix the (unrelated) test failures on Windows that were reported in #55, so this now fixes both #55 and #56.

The current failures are:

  1. On Appveyor, the Node 0.12 test run fails on npm install because that version of Node doesn't support fat arrow functions:
npm install
npm WARN npm npm does not support Node.js v0.12.18
npm WARN npm You should probably upgrade to a newer version of node as we
npm WARN npm can't make any promises that npm will work with this version.
npm WARN npm You can find the latest version at https://nodejs.org/
npm ERR! C:\Users\appveyor\AppData\Roaming\npm\node_modules\npm\lib\install.js:298
npm ERR!         [this, (next) => { computeMetadata(this.idealTree); next() }],
npm ERR!                       ^^
npm ERR! Unexpected token =>
  1. On Travis, the Node 0.12 test run fails one of the two tests in test/exec-flag.js, which looks like an intermittent failure, since the only changes I made to that file's logic are Windows-specific, and I can't reproduce locally on my Linux box.

@mykmelez mykmelez changed the title strip .exe suffix via case-insensitive replacement (fixes #56) strip .exe suffix via case-insensitive replacement (fixes #55 and #56) Jul 3, 2017
@mykmelez
Copy link
Author

It looks like @isaacs decided to fix the issues himself instead of taking this pull request, so I'm closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spawn-wrap fails fails to shim node on Windows (part deux)
3 participants