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

Support test suite on Windows #2270

Closed
wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
Contributor

@TimothyGu TimothyGu commented May 22, 2016

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.

})
})

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();
})
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dasilvacontin dasilvacontin Jun 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YES ^

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@boneskull boneskull added the type: bug a defect, confirmed by a maintainer label Jun 10, 2016
.have
.length(2);
.length(1 + (+symlinkSupported));
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In agreement.

@ScottFreeCode
Copy link
Contributor

  • 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
Copy link
Contributor

@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
Copy link
Contributor Author

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

@boneskull
Copy link
Contributor

fires up virtualbox

@boneskull
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor Author

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

@boneskull
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@TimothyGu
Copy link
Contributor Author

  • 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
Copy link
Contributor

👍 for .gitattributes

@boneskull
Copy link
Contributor

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

@ScottFreeCode
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

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 this pull request may close these issues.

Tests are broken in Windows
4 participants