Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix unit tests on Windows #1580

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants

SLaks commented Apr 16, 2013

ENOENT errors are reported differently on Windows; see comment.

This assumes the presence of make to run the tests in the first place.

SLaks commented Apr 16, 2013

For those who don't have make, I wrote a simple batch file to run the tests:

@ECHO OFF
SETLOCAL
SET MOCHA_OPTS=--check-leaks
SET REPORTER=dot


SET NODE_ENV=test

call node_modules\.bin\mocha --reporter %REPORTER% %MOCHA_OPTS%
call node_modules\.bin\mocha --reporter %REPORTER% %MOCHA_OPTS% test/acceptance

ENDLOCAL
Member

jonathanong commented Sep 10, 2013

can you rebase please? also, can anybody that uses windows confirm?

@SLaks SLaks Fix unit tests on Windows
This assumes the presence of make
a4cdf1b

SLaks commented Sep 10, 2013

I rebased.

I'm now getting a strange Windows-only failure in which the beforeEach() callback in test/Router.js doesn't run before one or two of the tests, causing router to be undefined.

Running mocha test\Router.js only makes those tests work fine; I'm trying to figure out what's going on.

Member

jonathanong commented Sep 17, 2013

just fyi, i want to actually test what's going on in windows before merging anything, but i haven't had the time yet. if anyone else with windows could verify or help, that would be great.

Contributor

caridy commented Sep 17, 2013

The fact that express uses make to execute the test is the first issue we should try to solve. This is what happen in a clean windows vm:

C:\Users\caridy\repos> git clone https://github.com/visionmedia/express.git
C:\Users\caridy\repos> cd express
C:\Users\caridy\repos> npm i
C:\Users\caridy\repos\express [master]> npm test

> express@3.4.0 test C:\Users\caridy\repos\express
> make test

'make' is not recognized as an internal or external command,
operable program or batch file.
npm ERR! Test failed.  See above for more details.
npm ERR! not ok code 0

There is nothing in that Makefile that we could not do in javascript (or even within package.json script commands directly), lets change that so it works on windows as well:

"scripts": {
    "prepublish": "npm prune",
    "test": "node ./path/to/the/new/Makefile.js"
  }

Hopefully, travis-ci folks will add windows support very soon (travis-ci/travis-ci#216), so we don't have to go thru the hazard of firing up a vm to test express when patching it.

Owner

tj commented Sep 17, 2013

or someone can just write a make clone for windows... that would be nicer than re-implementing similar things in javascript all the time, and every other language out there

SLaks commented Sep 17, 2013

Such as http://gnuwin32.sourceforge.net/, which I already have.

Contributor

caridy commented Sep 17, 2013

@visionmedia sure, but we will not be leveraging the support for multi-platform that comes with nodejs :), make is a mature piece of software, yes, but it never really striked for multi-platform support as far as I can tell.

@SLaks I will agree that it solves the problem if we could install it thru npm, otherwise it will complicate things. The premise here is to do git clone, npm install and npm test. If we have to do anything else, then we will always have situations like this one in new platforms that are already supported by nodejs. Remember, the whole point is to make constributor's life easier, lowering the barrier for them to contribute.

Install make, dammit :p. Being on windows isn't a valable reason not to use good tools ;). I have DevKit and it's just too great to be ignored.

Owner

tj commented Sep 17, 2013

because people keep avoiding it and re-inventing worse tools haha, x-platform codesmell

Contributor

caridy commented Sep 17, 2013

@nami-doc it is not about me (lazy dev using a mac), it depends on the CI (travis-ci in this case), and if they are going to provision DevKit (or something similar) to support calling make in their CI. The other solution is to push @visionmedia, @jonathanong and the others with commit access to run make in a windows machine before releasing a new version :p.

Anyways, lets way for travis to add support for windows in "few weeks" and we will see :)

mtalal16 commented Oct 2, 2013

I am not able to run command express under node directory
I am having error express is not recognize as internal or external command
I have copied express folder to node folder still having same problem
I am using windows 8

SLaks commented Oct 2, 2013

You need to run npm install -g express from an administrative command prompt.
This has nothing to do with this issue.

mtalal16 commented Oct 2, 2013

ok its done what i did is in my express is installed in my user directory I copied it and paste in nodejs folder after it its working
now i am having one more problem
express --sessions --css stylus --ejs myapp
I am using above command to create express project but its giving me following error
Error: EPERM mkdir /myapp
can you help me in it

SLaks commented Oct 2, 2013

You don't have permission to write to that folder. (or it exists or is in use)

Member

jonathanong commented Oct 30, 2013

how about just doing:

res.text.should.equal("ENOENT, stat '" + path.join('test', 'fixtures', 'nope.html') + "'");

that's what path.join() is for, and it'll be easier to understand what's going on than with that regexp

Owner

dougwilson commented Oct 30, 2013

I have Windows I can test this on.

Owner

dougwilson commented Oct 30, 2013

Looking at the test, I don't think it matters what the exact error message is. Setting the line to

res.text.should.startWith("ENOENT, stat");

should be just fine and will work on all platforms.

SLaks commented Oct 30, 2013

@dougwilson Is the point of this test to make sure that the path is included in the error message?

SLaks commented Oct 30, 2013

@jonathanong: That won't work; Windows includes the absolute path in the error.

Actually, that's an information disclosure vulnerability. @visionmedia, should I change the test to what @jonathanong suggested, and change the code to strip the absolute path on Windows?

Owner

dougwilson commented Oct 30, 2013

The point of the test was to check that it was a ENOENT error only, and even then that was before res.sendfile was broken out into it's own modules send.

SLaks commented Oct 30, 2013

In fact, it might make more sense to not modify the test at all (& change the server to always return /), to hide the platform from attackers.

Owner

dougwilson commented Oct 30, 2013

Normal requests don't return the file path with ENOENT, stat... just that test app.

Member

jonathanong commented Oct 30, 2013

yeah, i say just make it res.text.should.startWith("ENOENT, stat");. the test is called "should invoke the callback on 404" so the path is irrelevant to the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment