Support test suite on Windows #2270

TimothyGu · 2016-05-22T00:47:22Z

Based on work by Tyler Waters tyler.waters@gmail.com in #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 #1814 include:

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

Fixes #1813.

ScottFreeCode · 2016-06-08T05:40:26Z

test/acceptance/fs.js

    })
  })

  describe('when the file does not exist', function(){
    it('should fail', function(done){
      // uncomment
-      // fs.readFile('/tmp/does-not-exist', done);
+      // fs.readFile(t('does-not-exist'), done);
      done();
    })
  })


Note to self: go through the test suites and find things like this that are failure-tests but either commented out or otherwise unusually handled and if possible change them to assert that the failure throws or passes an error to a callback or whatever the expectable failure mode is.

YES ^

Edit: I've pointed this out every now and then. Tests that we have to manually check / uncomment => please no.

Moved this into its own issue, since it's nothing to do with this PR: #2309

ScottFreeCode · 2016-06-11T16:33:05Z

test/acceptance/lookup-files.js

      .have
-      .length(2);
+      .length(1 + (+symlinkSupported));
  });



For the expected length, I'd rather see something like:

// at start of test var expectedLength = 0; //...after the .contain(t('mocha-utils.js')); expectedLength += 1; //...after the .contain(t('mocha-utils-link.js')); expectedLength += 1; //...instead of .length(1 + (+symlinkSupported)); .length(expectedLength);

This is, in my opinion, clearer than using the unary + to cast the symlinkSupported bool to the integer 1, and more flexible in the event this test were ever to be changed such that there would be more than one additional file added if symlinkSupported. Although a simpler alternative that requires manual counting (instead of adding a += 1 line to everywhere the count should increase) would be to just change .length(1 + (+symlinkSupported)); to .length(1 + (symlinkSupported ? 1 : 0)); or .length(symlinkSupported ? 2 : 1);.

In agreement.

ScottFreeCode · 2016-06-11T17:20:34Z

I think I prefer tmpFile(...) over t(...) for clarity.
I'd like to see the reason for the changes of "2.1"->"2-1" documented in the commit logs -- not necessarily thoroughly, but enough that if I see the .-to-- change in the file diffs, then look at the commit log to find out why, it will both point me in the direction of the code I need to understand and fill in the knowledge of Windows needed to see why that code had to be treated differently, without having to look up the GitHub issue.
See inline comment about .length(1 + (+symlinkSupported));.

Other than that, looks good to me.

boneskull · 2016-06-11T21:05:53Z

@TimothyGu Thanks again so much for this!

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.

TimothyGu · 2016-06-14T01:40:44Z

@ScottFreeCode, all comments have been fixed in the latest iteration of the patch.

boneskull · 2016-06-26T06:17:16Z

fires up virtualbox

boneskull · 2016-06-26T06:20:59Z

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.

This is crazy. Awesome catch

boneskull · 2016-06-26T07:23:15Z

@TimothyGu This is really close, but something is killing IE8 tests--unless that's brokenness on SauceLabs' end. The browser won't "capture". Trying to find out more.

Related, I'm getting this setup on AppVeyor.

TimothyGu · 2016-06-27T00:31:23Z

@boneskull, is there a publicly available link to look at Sauce Labs output? Either way, that sounds like a Sauce Labs problem...

boneskull · 2016-06-27T00:49:38Z

@TimothyGu I think this is public

Anyway, I reported an issue to SauceLabs, hoping they can point me in the right direction. I don't know how to debug this myself--do you?

TimothyGu · 2016-06-27T07:00:15Z

I don't know how to debug this myself--do you?

Nope; in fact, I am totally at a loss here on how to interpret the Sauce Labs reports. Unfortunately I don't think I own a computer with IE8 either.

ScottFreeCode · 2016-06-28T04:25:29Z

I was able to get Karma's tests to trigger IE8 compatibility mode on IE11 by adding a meta tag to Karma's HTML files. Let's see, I think it was... <meta http-equiv="x-ua-compatible" content="IE=8"> in either node_modules/karma/static/client.html or node_modules/karma/static/context.html or maybe both. Probably has to be the first thing in <html><head>, or maybe the first thing except for a charset meta? There's also manual running of tests (create by hand a page that pulls in the test files you want) with the developer toolbar's compatibility-mode setting, if you want, say, to set allowUncaught and open up the debugger. Either of those could help dig into IE8 issues unrelated to Sauce. The downside is there are a few truly hairy corner cases where things will behave differently in real IE8 than in mere compatibility mode -- I forget what exactly it was, but one of the things I had to fix for the browser testing PR I was only getting the failure on Sauce and not on my local Karma runs (I want to say it was something involving the behavior of replacing toString).

ScottFreeCode · 2016-07-01T04:45:29Z

I haven't tried to run it with SauceLabs yet, but preliminary check that it runs on Windows yielded just a couple other bumps:

In Makefile, $(MAKE) may need to be "$(MAKE)" (or maybe I'm not setting make right on Cygwin/Git Shell?)
We're going to have to deal with line endings in the integration diffs tests. Seems like forcing the contents of test/integration/fixtures/diffs to Unix line endings might resolve the issue; alternatively, however, test/integration/diffs.js could try to normalize the line endings before comparison...?

boneskull · 2016-07-01T05:10:22Z

going to close this PR to send a different one. @TimothyGu 's changes are in it

TimothyGu · 2016-07-01T06:24:37Z

In Makefile, $(MAKE) may need to be "$(MAKE)" (or maybe I'm not setting make right on Cygwin/Git Shell?)

:sigh: Technically, no, since $(MAKE) might contain command line options that should not be quoted. If they are, the shell will interpret the options as if they are part of the executable file. I'm not certain enough to say it's a problem with your setup, but then it looks the most possible.

We're going to have to deal with line endings in the integration diffs tests. Seems like forcing the contents of test/integration/fixtures/diffs to Unix line endings might resolve the issue; alternatively, however, test/integration/diffs.js could try to normalize the line endings before comparison...?

Hmm, I didn't meet this error. It's entirely possible that the Git configuration has something to do with this. I set Git's core.autocrlf to input which resulted in UNIX line endings for those files.

We can force Git to store those files with UNIX line endings on all systems using a .gitattributes file, or of course we can normalize the test toolchain. Normalization sounds more robust, but the .editorconfig file mandates the use of UNIX line endings on all files.

boneskull · 2016-07-01T06:25:55Z

👍 for .gitattributes

boneskull · 2016-07-01T06:26:29Z

@TimothyGu at any rate, I think the proof's in the pudding

ScottFreeCode · 2016-07-01T21:47:44Z

Ah, I see. I guess I probably set MAKE wrong then; I am not that familiar with configuring it, on any system, thanks to packaging on Linux and CMake on Windows.

Looks as though the makefile being run on AppVeyor has some difficulties we don't run into using it in Cygwin-type shells, such as "find" being the unrelated Windows tool. (Also, it's getting a printout of ESLint's help options right after that, so I'm not sure what's going wrong there.) I wonder if we could configure it to run in something like Cygwin until we can replace the makefile with something more cross-platform.

boneskull · 2016-07-01T21:52:44Z

Appveyor has Cygwin and we can use its tools but I don't know if actually
running it in Cygwin is an option. I feel like we should't and it should
just work with a vanilla cmd
On Fri, Jul 1, 2016 at 14:47 Scott Santucci notifications@github.com
wrote:

Ah, I see. I guess I probably set MAKE wrong then; I am not that familiar
with configuring it, on any system, thanks to packaging on Linux and CMake
on Windows.

Looks as though the makefile being run on AppVeyor has some difficulties
we don't run into using it in Cygwin-type shells, such as "find" being
the unrelated Windows tool
https://ci.appveyor.com/project/boneskull/mocha/build/17/job/njpmatvtdrug258n#L613.
(Also, it's getting a printout of ESLint's help options right after that,
so I'm not sure what's going wrong there.) I wonder if we could configure
it to run in something like Cygwin until we can replace the makefile with
something more cross-platform.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#2270 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AA4bMa4u16PfIwFt1A_a5FGF_f2YrFjrks5qRYsEgaJpZM4Ij45S
.

Christopher Hiller
https://boneskull.com | https://github.com/boneskull

TimothyGu · 2016-07-02T08:58:47Z

I encountered the find.exe problem too. The only clean way to fix it is to make the MinGW or Cygwin bin path first in the env var, which the AppVeyor PR already does.

TimothyGu mentioned this pull request May 22, 2016

Fixes tests in windows (fixed #1813) #1814

Closed

TimothyGu force-pushed the test-windows branch 2 times, most recently from a08d768 to bc3ec87 Compare May 23, 2016 15:23

ScottFreeCode reviewed Jun 8, 2016
View reviewed changes

boneskull added the type: bug a defect, confirmed by a maintainer label Jun 10, 2016

TimothyGu force-pushed the test-windows branch from bc3ec87 to 4497053 Compare June 11, 2016 13:05

ScottFreeCode reviewed Jun 11, 2016
View reviewed changes

boneskull added the pr-needs-work label Jun 11, 2016

TimothyGu force-pushed the test-windows branch from 4497053 to f3564bd Compare June 14, 2016 01:37

TimothyGu force-pushed the test-windows branch from f3564bd to 6c2addc Compare June 14, 2016 01:39

boneskull closed this Jul 1, 2016

TimothyGu deleted the test-windows branch July 1, 2016 06:11

boneskull mentioned this pull request Jul 1, 2016

adds ability to run tests on windows; adds appveyor to CI; closes #1813 #2345

Merged

boneskull removed the pr-needs-work label Dec 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support test suite on Windows #2270

Support test suite on Windows #2270

TimothyGu commented May 22, 2016 •

edited

ScottFreeCode Jun 8, 2016

dasilvacontin Jun 9, 2016 •

edited

ScottFreeCode Jun 13, 2016

ScottFreeCode Jun 11, 2016

boneskull Jun 11, 2016

ScottFreeCode commented Jun 11, 2016

boneskull commented Jun 11, 2016

TimothyGu commented Jun 14, 2016

boneskull commented Jun 26, 2016

boneskull commented Jun 26, 2016

boneskull commented Jun 26, 2016

TimothyGu commented Jun 27, 2016

boneskull commented Jun 27, 2016

TimothyGu commented Jun 27, 2016

ScottFreeCode commented Jun 28, 2016

ScottFreeCode commented Jul 1, 2016

boneskull commented Jul 1, 2016

TimothyGu commented Jul 1, 2016

boneskull commented Jul 1, 2016

boneskull commented Jul 1, 2016

ScottFreeCode commented Jul 1, 2016

boneskull commented Jul 1, 2016

TimothyGu commented Jul 2, 2016

Support test suite on Windows #2270

Support test suite on Windows #2270

Conversation

TimothyGu commented May 22, 2016 • edited

ScottFreeCode Jun 8, 2016

Choose a reason for hiding this comment

dasilvacontin Jun 9, 2016 • edited

Choose a reason for hiding this comment

ScottFreeCode Jun 13, 2016

Choose a reason for hiding this comment

ScottFreeCode Jun 11, 2016

Choose a reason for hiding this comment

boneskull Jun 11, 2016

Choose a reason for hiding this comment

ScottFreeCode commented Jun 11, 2016

boneskull commented Jun 11, 2016

TimothyGu commented Jun 14, 2016

boneskull commented Jun 26, 2016

boneskull commented Jun 26, 2016

boneskull commented Jun 26, 2016

TimothyGu commented Jun 27, 2016

boneskull commented Jun 27, 2016

TimothyGu commented Jun 27, 2016

ScottFreeCode commented Jun 28, 2016

ScottFreeCode commented Jul 1, 2016

boneskull commented Jul 1, 2016

TimothyGu commented Jul 1, 2016

boneskull commented Jul 1, 2016

boneskull commented Jul 1, 2016

ScottFreeCode commented Jul 1, 2016

boneskull commented Jul 1, 2016

TimothyGu commented Jul 2, 2016

TimothyGu commented May 22, 2016 •

edited

dasilvacontin Jun 9, 2016 •

edited