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

Tests are broken in Windows #1813

Closed
tswaters opened this issue Jul 26, 2015 · 6 comments
Closed

Tests are broken in Windows #1813

tswaters opened this issue Jul 26, 2015 · 6 comments
Labels
type: bug a defect, confirmed by a maintainer

Comments

@tswaters
Copy link

Having read the contributing guidelines, I realized that on a windows machine I could not confirm whether any changes I committed failed tests or not because many of the tests are failing in windows.

Summary of issues and potential fixes:

  • I wasn't able to get the windows make to work properly at all. using cygwin seems to works though.
  • many references to '/tmp' inside acceptance fs/utils - these can be updated to use os.tmpdir()
  • passing bin/mocha to exec fails, "bin is not recognized as an internal or external command" -- updating it to node bin/mocha makes it work.
  • using spawn in integration/helpers doesn't work at all. Updating to use exec, passing args delimited by a space works. (also need to preface with node, similar to above)

The trickiest one relates to the dot symbol used by windows --

Integration/hook tests takes the output of test results and splits by new lines and the dot symbol. Thing is, windows doesn't use the dot symbol (\u2024) - it uses a full stop, so the tests fail because the split doesn't work, and a full stop winds up in some of the titles. Updating the regexp to use a dot in addition to \u2024 makes it fail in a different way (similar to how the tests would fail if the test titles included \u2024).

I think the best way to fix this is to first, change the test to use whatever base reporter exports for the dot symbol, and second, change the windows dot to use \u00B7. \u2024 would work, but using the default cmd font causes a beep every time it is used (which gets exceedingly annoying) - \u00B7 looks pretty close to the dot character, is supported by cmd and likely wouldn't be used in test titles.

I have fixes ready for these and will send a PR... I'm looking for confirmation that these changes (a) are welcome, (b) wouldn't break the tests when run in a unix environment (unsure if spawn vs. exec produces different results and whether prefacing the command with node causes issues)

@jbnicolai
Copy link

Hi @tswaters,

I'm looking for confirmation that these changes (a) are welcome

Fantastic initiative! I must admit I wasn't even aware of the failing tests on windows, but fixing these is more than welcome!

(b) wouldn't break the tests when run in a unix environment (unsure if spawn vs. exec produces different results and whether prefacing the command with node causes issues)

The easiest way for you to automatically test in a unix environment would be via Travis-CI. If you create a PR for these fixes it will run automatically, but you can also enable it for your own for tswaters/mocha so you can experiment before creating a PR.

Currently at least one test fails when running npm test

  34 passing (8s)
  1 failing

  1) options --grep runs specs matching a RegExp:

      Uncaught AssertionError: false == true
      + expected - actual

      -false
      +true

      at test/integration/options.js:117:9
      at test/integration/helpers.js:102:16
      at ChildProcess.<anonymous> (test/integration/helpers.js:169:5)
      at maybeClose (child_process.js:1015:16)
      at Socket.<anonymous> (child_process.js:1183:11)
      at Pipe.close (net.js:485:12)



make: *** [test-integration] Error 1

Going over your individual points:

I wasn't able to get the windows make to work properly at all. using cygwin seems to works though.

I'm not familiar with Windows make at all. Is there a compatible syntax that both versions of make can use? Or would this entail a Windows-specific Makefile? I would like to avoid the latter, if at all possible.

many references to '/tmp' inside acceptance fs/utils - these can be updated to use os.tmpdir()

Good catch.

passing bin/mocha to exec fails, "bin is not recognized as an internal or external command" -- updating it to node bin/mocha makes it work.

That should be the same. As you can see bin/mocha stats with a shebang ``#!/usr/bin/env node, which indeed means as much as 'run this file with the node` version on the user's path'.

using spawn in integration/helpers doesn't work at all. Updating to use exec, passing args delimited by a space works. (also need to preface with node, similar to above)

What's the issue with spawn? As far as I know this should be supported on Windows. The risk with using exec is that the max buffer size could potentially be exceeded, as we actually parse the stdout of the child_process. On the other hand, this probably won't actually be an issue as the output is relatively small.

The trickiest one relates to the dot symbol used by windows --
I think the best way to fix this is to first, change the test to use whatever base reporter exports for the dot symbol,

👍

and second, change the windows dot to use \u00B7.

This sounds like it's a backwards incompatible change. Doesn't mean we can't do it, but would require a release note and a bump in major versions.

@tswaters
Copy link
Author

I'm not entirely sure about the problems with make -- the windows make is basically this: http://gnuwin32.sourceforge.net/packages/make.htm -- it returns some nonsensical errors and ends up dying -- here's the output I see:

C:\Users\Tyler\github\mocha>npm test

> mocha@2.2.5 test C:\Users\Tyler\github\mocha
> make test-all

Access denied - LIB
File not found - -NAME
File not found - -TYPE
File not found - F
Access denied - LIB
File not found - -NAME
File not found - -TYPE
File not found - F
'node_modules' is not recognized as an internal or external command,
operable program or batch file.
make: *** [lint] Error 1
npm ERR! Test failed.  See above for more details.

So, the problem with Windows and spawn is that it doesn't read shebangs -- like, at all -- and has a tendency to fail if you pass spawn a file that isn't a bat, cmd or exe. Windows doesn't read the shabang, doesn't recognize the file as an executable and flops.

exec works a little bit differently than spawn in that you can at least pass it node and a script and it works. There's a lot of stuff I'm probably glossing over here, but if you're interested here's a node core ticket that's been open for 4.5 years that describes a lot of what is going on nodejs/node-v0.x-archive#2318

I have a feeling the failing test is a symptom of switching to use exec, which is what I was afraid of. My guess would be that this one is failing because of the regexp string passed directly into exec ... spawn must work better with the arguments.

There is another module that can be used as a drop-in replacement for spawn called https://github.com/IndigoUnited/node-cross-spawn-async -- this will work some magic on the commands before passing it to spawn to make it work properly in a windows environment.

Anyway, I'll give the travisCI a go in my repo and see if I can get a clean patch that has everything working.

@tswaters
Copy link
Author

Ok, I got it working but I borked the squash... starting over.

@tswaters
Copy link
Author

So I blew away my repo and reapplied the one commit -- travis ci was buggered, still pointing at the deleted repo -- got that setup again, but I need to push something to get it to go. I'm going to go ahead and create the PR - last I checked, I had everything working.

@santiagoaguiar
Copy link

Is having the build/tests working on windows desirable then? I did some work on it too and can submit a pull request.

Most changes require changing some paths to use path.join, using os.tmpDir instead of /tmp, and running scripts through child_process.exec(node <script>,..) instead of assuming a working shebang behavior.

I still have some issues on the integration tests on master (I think mostly caused by EOL differences), but I'll keep working on it if it's on scope.

I suppose it's not an option, but moving from the Makefile to use npm scripts might be a good idea, getting make to run on windows does take some time :).

tswaters added a commit to tswaters/mocha that referenced this issue Feb 8, 2016
@tswaters
Copy link
Author

tswaters commented Feb 8, 2016

Oh hey, I forgot about this....

I've re-based this against master (5b325c8) and verified everything is still working (it appears to be, my travis ci passed anyways). After running tests again after rebase under cygwin make, I noticed two that were failing -- each was doing a split based on new line and bullet. I had added a test helper export that has a working regex so I just updated them to use that instead (1cf2f60).

if I recall the only crunchy bit on this was updating the dot to not use full stop under windows and make it \u00B7 which is a breaking change. Now that I've had 7 months to think about it, it may be easier to instead update the titles of the tests to not include periods so the split works as expected and the tests pass. I've done that. (06911ab)

TimothyGu added a commit to TimothyGu/mocha that referenced this issue May 22, 2016
Based on work by Tyler Waters <tyler.waters@gmail.com> in mochajs#1814.

Changes from mochajs#1814 include:

- Rebasing
- Use process.argv[0] as an authoritative path for Node.js executable
- Support having spaces in path of Node.js executable
- Avoid external dependencies for child_process.spawn()
- Fix symlink tests on Windows. On Windows, creating symlinks can fail
  since it needs additional user permissions

Fixes mochajs#1813.
TimothyGu added a commit to TimothyGu/mocha that referenced this issue May 22, 2016
Based on work by Tyler Waters <tyler.waters@gmail.com> in mochajs#1814.

Changes from mochajs#1814 include:

- Rebasing
- Use process.argv[0] as an authoritative path for Node.js executable
- Support having spaces in path of Node.js executable
- Avoid external dependencies for child_process.spawn()
- Fix symlink tests on Windows. On Windows, creating symlinks can fail
  since it needs additional user permissions

Fixes mochajs#1813.
TimothyGu added a commit to TimothyGu/mocha that referenced this issue May 23, 2016
Based on work by Tyler Waters <tyler.waters@gmail.com> in mochajs#1814.

Changes from mochajs#1814 include:

- Rebasing
- Use process.argv[0] as an authoritative path for Node.js executable
- Support having spaces in path of Node.js executable
- Avoid external dependencies for child_process.spawn()
- Fix symlink tests on Windows. On Windows, creating symlinks can fail
  since it needs additional user permissions

Fixes mochajs#1813.
TimothyGu added a commit to TimothyGu/mocha that referenced this issue Jun 11, 2016
Based on work by Tyler Waters <tyler.waters@gmail.com> in mochajs#1814.

Changes from mochajs#1814 include:

- Rebasing
- Use process.argv[0] as an authoritative path for Node.js executable
- Support having spaces in path of Node.js executable
- Avoid external dependencies for child_process.spawn()
- Fix symlink tests on Windows. On Windows, creating symlinks can fail
  since it needs additional user permissions

Fixes mochajs#1813.
TimothyGu added a commit to TimothyGu/mocha that referenced this issue Jun 14, 2016
Based on work by Tyler Waters <tyler.waters@gmail.com> in mochajs#1814.

This commit contains the following changes:

- Add quotation marks to Makefile variables for programs. The variables
  use POSIX-style paths, which cannot be used on Windows to launch a
  program except when quoted. Using double quotation marks instead of
  single since the latter is not available on Windows
- Use os-tmpdir module to get an acceptable directory for temporary
  usage instead of relying on the POSIX /tmp
- Use process.execPath as an authoritative path for Node.js executable
- Detect whether symbol links are supported on the platform before
  testing. On Windows, creating symlinks can fail since it needs
  additional user permissions
- Fix hook tests. The tests parse output of the "dot" reporter to
  separate output of individual tests. The "dot" reporter uses "·"
  symbol (U+2024 ONE DOT LEADER) under POSIX environments and "." symbol
  (U+002E FULL STOP) under Windows, which means that having "." in the
  echoed message makes it ambiguous to be parsed in Windows. To fix this
  issue, two separate changes are necessary:
  - Use a dynamically created regular expression to split the tests
    based on the specific dot character used on the platform
  - Replace "." with "-" in echoed messages in fixtures for hook tests
    to avoid ambiguity with the character output by the reporter

Changes from mochajs#1814 include:

- Rebasing
- Use process.execPath as an authoritative path for Node.js executable
- Avoid external dependencies for child_process.spawn()
- Detect whether symbol links are supported on the platform before
  testing. On Windows, creating symlinks can fail since it needs
  additional user permissions

Fixes mochajs#1813.
TimothyGu added a commit to TimothyGu/mocha that referenced this issue Jun 14, 2016
Based on work by Tyler Waters <tyler.waters@gmail.com> in mochajs#1814.

This commit contains the following changes:

- Add quotation marks to Makefile variables for programs. The variables
  use POSIX-style paths, which cannot be used on Windows to launch a
  program except when quoted. Using double quotation marks instead of
  single since the latter is not available on Windows
- Use os-tmpdir module to get an acceptable directory for temporary
  usage instead of relying on the POSIX /tmp
- Use process.execPath as an authoritative path for Node.js executable
- Detect whether symbolic links are supported on the platform before
  testing. On Windows, creating symlinks can fail since it needs
  additional user permissions
- Fix hook tests. The tests parse output of the "dot" reporter to
  separate output of individual tests. The "dot" reporter uses "·"
  symbol (U+2024 ONE DOT LEADER) under POSIX environments and "." symbol
  (U+002E FULL STOP) under Windows, which means that having "." in the
  echoed message makes it ambiguous to be parsed in Windows. To fix this
  issue, two separate changes are necessary:
  - Use a dynamically created regular expression to split the tests
    based on the specific dot character used on the platform
  - Replace "." with "-" in echoed messages in fixtures for hook tests
    to avoid ambiguity with the character output by the reporter

Changes from mochajs#1814 include:

- Rebasing
- Use process.execPath as an authoritative path for Node.js executable
- Avoid external dependencies for child_process.spawn()
- Detect whether symbol links are supported on the platform before
  testing. On Windows, creating symlinks can fail since it needs
  additional user permissions

Fixes mochajs#1813.
@boneskull boneskull added type: bug a defect, confirmed by a maintainer qa labels Jul 1, 2016
boneskull added a commit that referenced this issue Jul 1, 2016
boneskull added a commit that referenced this issue Jul 2, 2016
adds ability to run tests on windows; adds appveyor to CI; closes #1813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants