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

fix(watcher): handle paths on Windows #854

Merged
merged 2 commits into from Jul 25, 2014

Conversation

Projects
None yet
4 participants
@axemclion
Contributor

axemclion commented Dec 10, 2013

Fixed paths for tests/code so that they also run on Windows. Most of the fixes are to normalize paths.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Dec 10, 2013

Contributor

All the tests pass except this - Do you know where config.basePath is set as default. For some reason, config.basePath is undefined and this is test still fails on windows. I could investigate more if you don't know this form the top of your head.


1) middleware.runner should refresh explicit files if specified:
     TypeError: Arguments to path.resolve must be strings
      at Object.exports.resolve (path.js:116:15)
      at c:\_workspace\axe\karma\lib\middleware\runner.js:57:36
      at Array.forEach (native)
      at c:\_workspace\axe\karma\lib\middleware\runner.js:56:27
      at null.<anonymous> (c:\_workspace\axe\karma\node_modules\connect\lib\middleware\json.js:82:9)
      at EventEmitter.emit (events.js:92:17)
      at Context.<anonymous> (c:\_workspace\axe\karma\test\unit\middleware\runner.spec.coffee:122:15)
      at Test.Runnable.run (c:\_workspace\axe\karma\node_modules\mocha\lib\runnable.js:196:15)
      at Runner.runTest (c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:344:10)
      at c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:390:12
      at next (c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:270:14)
      at c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:279:7
      at next (c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:227:23)
      at c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:242:7
      at Hook.Runnable.run (c:\_workspace\axe\karma\node_modules\mocha\lib\runnable.js:215:5)
      at next (c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:236:10)
      at Object._onImmediate (c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:247:5)
      at processImmediate [as _immediateCallback] (timers.js:330:15)

Contributor

axemclion commented Dec 10, 2013

All the tests pass except this - Do you know where config.basePath is set as default. For some reason, config.basePath is undefined and this is test still fails on windows. I could investigate more if you don't know this form the top of your head.


1) middleware.runner should refresh explicit files if specified:
     TypeError: Arguments to path.resolve must be strings
      at Object.exports.resolve (path.js:116:15)
      at c:\_workspace\axe\karma\lib\middleware\runner.js:57:36
      at Array.forEach (native)
      at c:\_workspace\axe\karma\lib\middleware\runner.js:56:27
      at null.<anonymous> (c:\_workspace\axe\karma\node_modules\connect\lib\middleware\json.js:82:9)
      at EventEmitter.emit (events.js:92:17)
      at Context.<anonymous> (c:\_workspace\axe\karma\test\unit\middleware\runner.spec.coffee:122:15)
      at Test.Runnable.run (c:\_workspace\axe\karma\node_modules\mocha\lib\runnable.js:196:15)
      at Runner.runTest (c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:344:10)
      at c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:390:12
      at next (c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:270:14)
      at c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:279:7
      at next (c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:227:23)
      at c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:242:7
      at Hook.Runnable.run (c:\_workspace\axe\karma\node_modules\mocha\lib\runnable.js:215:5)
      at next (c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:236:10)
      at Object._onImmediate (c:\_workspace\axe\karma\node_modules\mocha\lib\runner.js:247:5)
      at processImmediate [as _immediateCallback] (timers.js:330:15)

@vojtajina

View changes

Show outdated Hide outdated lib/watcher.js
@@ -30,7 +31,7 @@ var watchPatterns = function(patterns, watcher) {
// watch only common parents, no sub paths
pathsToWatch.forEach(function(path) {
if (!pathsToWatch.some(function(p) {
return p !== path && path.substr(0, p.length + 1) === p + DIR_SEP;
return p !== path && normalize(path.substr(0, p.length + 1)) === normalize(p + DIR_SEP);

This comment has been minimized.

@vojtajina

vojtajina Dec 22, 2013

Contributor

why do you need this normalize?

@vojtajina

vojtajina Dec 22, 2013

Contributor

why do you need this normalize?

This comment has been minimized.

@axemclion

axemclion Dec 22, 2013

Contributor

For the test case - https://github.com/karma-runner/karma/blob/master/test/unit/watcher.spec.coffee#L66

The watchedPath_ are [ '/some/sub', '/some' ], since the paths are ./some/sub and /some/sub\ when they are not normalized. i.e. In windows, the trailing slashes and the initial dot still exist

@axemclion

axemclion Dec 22, 2013

Contributor

For the test case - https://github.com/karma-runner/karma/blob/master/test/unit/watcher.spec.coffee#L66

The watchedPath_ are [ '/some/sub', '/some' ], since the paths are ./some/sub and /some/sub\ when they are not normalized. i.e. In windows, the trailing slashes and the initial dot still exist

This comment has been minimized.

@vojtajina

vojtajina Jan 21, 2014

Contributor

So can't you just normalize it in the test? I don't like the idea of changing production code in order to make a test pass on windows...

@vojtajina

vojtajina Jan 21, 2014

Contributor

So can't you just normalize it in the test? I don't like the idea of changing production code in order to make a test pass on windows...

@vojtajina

View changes

Show outdated Hide outdated lib/init.js
@@ -145,7 +145,7 @@ var questions = [{
var getBasePath = function(configFilePath, cwd) {
var configParts = path.dirname(configFilePath).split(path.sep);
var cwdParts = cwd.split(path.sep);
var cwdParts = path.normalize(cwd).split(path.sep);

This comment has been minimized.

@vojtajina

vojtajina Dec 22, 2013

Contributor

why do you need this? it's coming from process.cwd() so it should be already normalized....

@vojtajina

vojtajina Dec 22, 2013

Contributor

why do you need this? it's coming from process.cwd() so it should be already normalized....

This comment has been minimized.

@axemclion

axemclion Dec 22, 2013

Contributor

cwd is supposed to come from process.cwd, but in case of the tests, it is initialized - see this https://github.com/karma-runner/karma/blob/master/test/unit/init.spec.coffee#L114

I could fix it in the test only, but then if we add a new test case, they have to be tested on windows also. Given that it is permitted for cwd to not come from process.cwd (atleast for tests) , i sanitized it in the function.

@axemclion

axemclion Dec 22, 2013

Contributor

cwd is supposed to come from process.cwd, but in case of the tests, it is initialized - see this https://github.com/karma-runner/karma/blob/master/test/unit/init.spec.coffee#L114

I could fix it in the test only, but then if we add a new test case, they have to be tested on windows also. Given that it is permitted for cwd to not come from process.cwd (atleast for tests) , i sanitized it in the function.

This comment has been minimized.

@vojtajina

vojtajina Jan 21, 2014

Contributor

Yep, fix the test.

@vojtajina

vojtajina Jan 21, 2014

Contributor

Yep, fix the test.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Jan 13, 2014

Contributor

Ping .. @vojtajina Are there any other comments on this ? Should I rebase and is it good to be merged ?

Contributor

axemclion commented Jan 13, 2014

Ping .. @vojtajina Are there any other comments on this ? Should I rebase and is it good to be merged ?

@vojtajina

This comment has been minimized.

Show comment
Hide comment
@vojtajina

vojtajina Jan 21, 2014

Contributor

Sorry for delays @axemclion. Can you also rebase it on the latest master? Thanks a lot.

Contributor

vojtajina commented Jan 21, 2014

Sorry for delays @axemclion. Can you also rebase it on the latest master? Thanks a lot.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Jan 21, 2014

Contributor

@vojtajina done. There is just 1 test case that fails now. Investigating it, will send in another pull request for that.

Contributor

axemclion commented Jan 21, 2014

@vojtajina done. There is just 1 test case that fails now. Investigating it, will send in another pull request for that.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Jan 22, 2014

Contributor

The last issue is due to path.resolve.

On linux, path.resolve(undefined, '/changed.js') works, but on Windows, it fails. The Windows equivalent of this would be path.resolve(undefined, 'c:\changed.js'). Alternatively, path.resolve('', '/changed.js') returns c:\changed.js. To fix this, I am guessing that we should change the test case?

Contributor

axemclion commented Jan 22, 2014

The last issue is due to path.resolve.

On linux, path.resolve(undefined, '/changed.js') works, but on Windows, it fails. The Windows equivalent of this would be path.resolve(undefined, 'c:\changed.js'). Alternatively, path.resolve('', '/changed.js') returns c:\changed.js. To fix this, I am guessing that we should change the test case?

@vojtajina

This comment has been minimized.

Show comment
Hide comment
@vojtajina

vojtajina Jan 30, 2014

Contributor

You don't have to send a new PR, just rebase and force push to this one.

Yep correct the test. In production basePath should always be defined and always be a string. (The validation has to be improved, but that's in lib/config.js)

Contributor

vojtajina commented Jan 30, 2014

You don't have to send a new PR, just rebase and force push to this one.

Yep correct the test. In production basePath should always be defined and always be a string. (The validation has to be improved, but that's in lib/config.js)

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Feb 4, 2014

Contributor

@vojtajina Fixed the last failing tests. Was able to get all unit tests passing on a Windows Machine However, I am getting an unrelated error in the line length of process.spec.js.

Contributor

axemclion commented Feb 4, 2014

@vojtajina Fixed the last failing tests. Was able to get all unit tests passing on a Windows Machine However, I am getting an unrelated error in the line length of process.spec.js.

@vojtajina

This comment has been minimized.

Show comment
Hide comment
@vojtajina

vojtajina Feb 5, 2014

Contributor

The error is from linting. You can run grunt lint, you have to wrap lines at 100 chars.

Contributor

vojtajina commented Feb 5, 2014

The error is from linting. You can run grunt lint, you have to wrap lines at 100 chars.

@vojtajina

View changes

Show outdated Hide outdated lib/init.js
@@ -144,8 +144,8 @@ var questions = [{
var getBasePath = function(configFilePath, cwd) {
var configParts = path.dirname(configFilePath).split(path.sep);
var cwdParts = cwd.split(path.sep);
var configParts = path.dirname(path.normalize(configFilePath)).split(path.sep);

This comment has been minimized.

@vojtajina

vojtajina Feb 5, 2014

Contributor

I don't think you need these path.normalize, fix the tests. Let's not change production code unless there is some real bug.

@vojtajina

vojtajina Feb 5, 2014

Contributor

I don't think you need these path.normalize, fix the tests. Let's not change production code unless there is some real bug.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Feb 5, 2014

Contributor

@vojtajina Fixed the issue with init.js. Moved the normalize to the test case instead.

Contributor

axemclion commented Feb 5, 2014

@vojtajina Fixed the issue with init.js. Moved the normalize to the test case instead.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Feb 5, 2014

Contributor

The task "coffeelint:unittests" has more errors. Not sure if they should be fixed as a part of this commit. Travis is however failing due to the lining errors.

Contributor

axemclion commented Feb 5, 2014

The task "coffeelint:unittests" has more errors. Not sure if they should be fixed as a part of this commit. Travis is however failing due to the lining errors.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Feb 5, 2014

Contributor

Added the extra commit to fix the coffeelint:unittest issues. Surprised that the regular build do not show this error. Would not want to ideally have the commit as a part of this pull request.

Contributor

axemclion commented Feb 5, 2014

Added the extra commit to fix the coffeelint:unittest issues. Surprised that the regular build do not show this error. Would not want to ideally have the commit as a part of this pull request.

@vojtajina

This comment has been minimized.

Show comment
Hide comment
@vojtajina

vojtajina Feb 14, 2014

Contributor

@axemclion running grunt lint on the current master passes, so just make sure you rebase it to the latest master and you should not need the additional commit with fixing style.

Also there are still comments that you didn't fix - for instance the watcher. I don't wanna change production code just to make tests to pass on Windows. Either fix the tests or prove that the production code is wrong.

Contributor

vojtajina commented Feb 14, 2014

@axemclion running grunt lint on the current master passes, so just make sure you rebase it to the latest master and you should not need the additional commit with fixing style.

Also there are still comments that you didn't fix - for instance the watcher. I don't wanna change production code just to make tests to pass on Windows. Either fix the tests or prove that the production code is wrong.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Feb 15, 2014

Contributor

The problem with watcher is in the way the base directory is returned- https://github.com/karma-runner/karma/blob/master/lib/watcher.js#L11

You are trying to replace unix type directory separator instead of getting the directory separator from DIR_SEP.

Contributor

axemclion commented Feb 15, 2014

The problem with watcher is in the way the base directory is returned- https://github.com/karma-runner/karma/blob/master/lib/watcher.js#L11

You are trying to replace unix type directory separator instead of getting the directory separator from DIR_SEP.

.replace(/\/[^\/]*\)\?.*$/, '') || '/'; // remove parts with (...)?
return pattern.replace(/[\/\\][^\/\\]*\*.*$/, '') // remove parts with *
.replace(/[\/\\][^\/\\]*[\!\+]\(.*$/, '') // remove parts with !(...) and +(...)
.replace(/[\/\\][^\/\\]*\)\?.*$/, '') || DIR_SEP; // remove parts with (...)?
};

This comment has been minimized.

@axemclion

axemclion Feb 15, 2014

Contributor

Need to check for windows and for posix file patterns. This regex could cause problems when a path contains \ and /, but my guess is that \ and / are treated as path separators. Is that fine ? If not, we may have to write logic similar to what node.js path module does.

Do you think it may make sense to use the same logic that path.js uses in node - https://github.com/joyent/node/blob/master/lib/path.js#L61

Note that they seem to use completely different code paths for Windows and for Posix.

@axemclion

axemclion Feb 15, 2014

Contributor

Need to check for windows and for posix file patterns. This regex could cause problems when a path contains \ and /, but my guess is that \ and / are treated as path separators. Is that fine ? If not, we may have to write logic similar to what node.js path module does.

Do you think it may make sense to use the same logic that path.js uses in node - https://github.com/joyent/node/blob/master/lib/path.js#L61

Note that they seem to use completely different code paths for Windows and for Posix.

This comment has been minimized.

@vojtajina

vojtajina Mar 10, 2014

Contributor

You are right, this code doesn't work on Windows. Can you make this change as a separate commit and add a test for it? This is a great fix for Windows watching! Thank you!

but I don't wanna make it a part of "fixing tests to run on windows commit", because this is actually a production code fix

the DIR_SEP at the end is useless, you could leave '/' there because that's a fallback if you have a pattern in root (eg. /*.js) in which case you need to watch the root. This however won't work on Windows, but that's not a problem, because this fallback should never happen on Win (it's gonna be C:\**.js, in which case C:\ will be watched)

I think your solution should work even if the path contains both \ and /, what could be a problem?

@vojtajina

vojtajina Mar 10, 2014

Contributor

You are right, this code doesn't work on Windows. Can you make this change as a separate commit and add a test for it? This is a great fix for Windows watching! Thank you!

but I don't wanna make it a part of "fixing tests to run on windows commit", because this is actually a production code fix

the DIR_SEP at the end is useless, you could leave '/' there because that's a fallback if you have a pattern in root (eg. /*.js) in which case you need to watch the root. This however won't work on Windows, but that's not a problem, because this fallback should never happen on Win (it's gonna be C:\**.js, in which case C:\ will be watched)

I think your solution should work even if the path contains both \ and /, what could be a problem?

This comment has been minimized.

@vojtajina

vojtajina Apr 14, 2014

Contributor

@axemclion Sorry for taking it so long to get this in.

Could you please separate this change to prod code into a separate file? I think it's good to be merged then... Thank you so much.

@vojtajina

vojtajina Apr 14, 2014

Contributor

@axemclion Sorry for taking it so long to get this in.

Could you please separate this change to prod code into a separate file? I think it's good to be merged then... Thank you so much.

This comment has been minimized.

@axemclion

axemclion Jul 6, 2014

Contributor

you mean separate commit, and not separate file, right ?

@axemclion

axemclion Jul 6, 2014

Contributor

you mean separate commit, and not separate file, right ?

This comment has been minimized.

@vojtajina

vojtajina Jul 25, 2014

Contributor

yes, I meant separate commit

@vojtajina

vojtajina Jul 25, 2014

Contributor

yes, I meant separate commit

@vojtajina

This comment has been minimized.

Show comment
Hide comment
@vojtajina

vojtajina Apr 14, 2014

Contributor

Also, the commit message needs to be tests: fix the unit tests on Windows and the commit changing the prod code should be fix(watcher): handle paths on Windows and please add a description of the problem...

Can you also rebase it on the latest stable branch? The Travis is failing, but I think if you rebase it, it might be fine...

Contributor

vojtajina commented Apr 14, 2014

Also, the commit message needs to be tests: fix the unit tests on Windows and the commit changing the prod code should be fix(watcher): handle paths on Windows and please add a description of the problem...

Can you also rebase it on the latest stable branch? The Travis is failing, but I think if you rebase it, it might be fine...

@bajtos

This comment has been minimized.

Show comment
Hide comment
@bajtos

bajtos Jun 26, 2014

Ping.

Seems the progress has stopped :( @axemclion could you please find time to finish your patch? Is there any way how I can help you?

Alternatively, is there any workaround to get karma tests working on windows?

bajtos commented Jun 26, 2014

Ping.

Seems the progress has stopped :( @axemclion could you please find time to finish your patch? Is there any way how I can help you?

Alternatively, is there any workaround to get karma tests working on windows?

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Jun 26, 2014

Contributor

Sorry my bad, I will look at it this weekend. Was busy with another project.

Contributor

axemclion commented Jun 26, 2014

Sorry my bad, I will look at it this weekend. Was busy with another project.

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Jul 6, 2014

Contributor

@bajtos, @vojtajina, rebased, and added the 2 commits that you wanted.

Contributor

axemclion commented Jul 6, 2014

@bajtos, @vojtajina, rebased, and added the 2 commits that you wanted.

@maksimr

This comment has been minimized.

Show comment
Hide comment
@maksimr

maksimr Jul 6, 2014

Contributor

@axemclion Thanks!
Can you fix some lint errors?

Contributor

maksimr commented Jul 6, 2014

@axemclion Thanks!
Can you fix some lint errors?

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Jul 6, 2014

Contributor

@maksimr Done.

Contributor

axemclion commented Jul 6, 2014

@maksimr Done.

@maksimr

View changes

Show outdated Hide outdated test/unit/watcher.spec.coffee
@@ -64,8 +65,9 @@ describe 'watcher', ->
it 'should not watch subpaths that are already watched', ->
m.watchPatterns ['/some/sub/*.js', '/some/a*.*'], chokidarWatcher
expect(chokidarWatcher.watchedPaths_).to.deep.equal ['/some']
m.watchPatterns [path.normalize('/some/sub/*.js'), path.normalize('/some/a*.*')],

This comment has been minimized.

@maksimr

maksimr Jul 6, 2014

Contributor

Suggest

['/some/*.js', '/some/a*.*'].map(path.normalize)

instead

[path.normalize('/some/sub/*.js'), path.normalize('/some/a*.*')]
@maksimr

maksimr Jul 6, 2014

Contributor

Suggest

['/some/*.js', '/some/a*.*'].map(path.normalize)

instead

[path.normalize('/some/sub/*.js'), path.normalize('/some/a*.*')]
@maksimr

This comment has been minimized.

Show comment
Hide comment
@maksimr

maksimr Jul 6, 2014

Contributor

I think we need rename PR to fix(watcher): handle paths on Windows because it is main issue
and add description where describe problem with path.

In the rest of good 👍

LGTM

Contributor

maksimr commented Jul 6, 2014

I think we need rename PR to fix(watcher): handle paths on Windows because it is main issue
and add description where describe problem with path.

In the rest of good 👍

LGTM

@axemclion axemclion changed the title from fix(tests): Fixed paths for Tests in Windows to fix(watcher): handle paths on Windows Jul 6, 2014

@axemclion

This comment has been minimized.

Show comment
Hide comment
@axemclion

axemclion Jul 17, 2014

Contributor

Anything else I need to do before this gets merged ?

Contributor

axemclion commented Jul 17, 2014

Anything else I need to do before this gets merged ?

@vojtajina

This comment has been minimized.

Show comment
Hide comment
@vojtajina

vojtajina Jul 25, 2014

Contributor

Sorry for delays. Looks good to me. Merging.

Contributor

vojtajina commented Jul 25, 2014

Sorry for delays. Looks good to me. Merging.

@vojtajina vojtajina merged commit c21d9d5 into karma-runner:master Jul 25, 2014

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