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

Invalid string in instrumented code caused by unescaped Windows pathmarks #491

Closed
TomGault opened this issue May 11, 2015 · 17 comments
Closed

Comments

@TomGault
Copy link
Contributor

I don't have the time to do a complete PR for this guy but I'll tell you what I do know:

  • I develop on a Windows box. I just cloned a project which was developed likely entirely on Macs.
  • I ran the gruntfile which fired up Mocha to run tests using blanket for code coverage reporting.
  • Blanket barfed with an error something like this:
[15:55:11] /c/git-repos/project (master)
$ grunt
Running "jshint:files" (jshint) task
>> 24 files lint free.

Running "mochaTest:test" (mochaTest) task
>> Mocha exploded!
>> Error: BLANKET-Error parsing instrumented code: Error: BLANKET-Error parsing instrumented code: SyntaxError: c:\git-repos\project\somefile.js:2
>> if (typeof _$jscoverage['c:\git-repos\project\somefile.js'] === 'u
>>                         ^^^^^^^^^^^^^^^^^^^^^^^
>> Unexpected token ILLEGAL
>>     at c:\git-repos\project\node_modules\blanket\src\index.js:174:35
>>     at Object.blanket._blanket.instrument (c:\git-repos\project\node_modules\blanket\src\blanket.js:125:17)
>>     at Object.require.extensions..js (c:\git-repos\project\node_modules\blanket\src\index.js:155:25)
>>     at Module.load (module.js:355:32)
>>     at Function.Module._load (module.js:310:12)
>>     at Module.require (module.js:365:17)
>>     at require (module.js:384:17)
>>     at Object.<anonymous> (c:\git-repos\project\test\core\authSpec.js:11:12)
>>     at Module._compile (module.js:460:26)
>>     at Module._extensions..js (module.js:478:10)
Warning: Task "mochaTest:test" failed. Use --force to continue.

Aborted due to warnings.
[15:57:01] /c/git-repos/project (master)
$

It looks like the Windows pathmarks are not being escaped. That is, c:\git-repos should be c:\\git-repos, or, since node is pretty smart, it totally works if you use c:/git-repos.

project/node_modules/blanket/src/index.js looks like this (starting at line 128 for me)

        //instrument js files
        require.extensions['.js'] = function(localModule, filename) {
            var pattern = blanket.options("filter"),
                reporter_options = blanket.options("reporter_options"),
                originalFilename = filename,
                inputFilename = filename;
            filename = blanket.normalizeBackslashes(filename);

            //we check the never matches first

If I change it to this (i.e. normalize the input filename), then the error goes away:

        //instrument js files
        require.extensions['.js'] = function(localModule, filename) {
            var pattern = blanket.options("filter"),
                reporter_options = blanket.options("reporter_options"),
                originalFilename = filename,
                inputFilename = blanket.normalizeBackslashes(filename);
            filename = inputFilename;

            //we check the never matches first
@Jeff-Lewis
Copy link

+1

@bengreenier
Copy link

@TomGault FWIW, that fix worked great for me :shipit:

f2bo added a commit to pwa-builder/PWABuilder-CLI that referenced this issue May 27, 2015
@CaptainNic
Copy link

@TomGault : Fix works for me as well 👍

@theneva
Copy link

theneva commented Jun 6, 2015

+1

theneva added a commit to theneva/blanket that referenced this issue Jun 6, 2015
Running `mocha` exploded in Windows environments after 1.1.6 because the paths aren't properly normalized (see alex-seville#491). Normalizing `inputFilename` instead of `filename` fixes this problem.
theneva added a commit to theneva/blanket that referenced this issue Jun 6, 2015
…the paths aren't properly normalized (see alex-seville#491). Normalizing `inputFilename` instead of `filename` fixes this problem.
theneva added a commit to theneva/blanket that referenced this issue Jun 6, 2015
…the paths aren't properly normalized (see alex-seville#491). Normalizing `inputFilename` instead of `filename` fixes this problem, as demonstrated by @TomGault.
theneva added a commit to theneva/blanket that referenced this issue Jun 6, 2015
…aren't properly normalized (see alex-seville#491). Normalizing `inputFilename` instead of `filename` fixes this problem, as demonstrated by @TomGault.
theneva added a commit to theneva/blanket that referenced this issue Jun 6, 2015
…aren't properly normalized (see alex-seville#491). Normalizing `inputFilename` instead of `filename` fixes this problem, as demonstrated by @TomGault.
theneva added a commit to theneva/blanket that referenced this issue Jun 6, 2015
Mocha explodes in Windows environments after 1.1.6 because the paths aren't properly normalized (see alex-seville#491). Normalizing `inputFilename` instead of `filename` fixes this problem, as demonstrated by @TomGault.
theneva added a commit to theneva/blanket that referenced this issue Jun 6, 2015
Mocha explodes in Windows environments after 1.1.6 because the paths aren't properly normalized (see alex-seville#491). Normalizing `inputFilename` instead of `filename` fixes this problem, as demonstrated by @TomGault.
@theneva
Copy link

theneva commented Jun 6, 2015

... wtf. Submitted PR #502 with @TomGault's fix. Sorry about the spam – I thought GitHub knew better.

@TomGault
Copy link
Contributor Author

TomGault commented Jun 7, 2015

Haha, no problem. Thanks so much for doing the leg-work!

@mikkotikkanen
Copy link

+1 to this as well. Just ran into the same issue.

Would be nice to get the PR merged and new patch released since I would like to use blanket but since most of our development is happening in Windows envs, the thing doesn't currently work :(

@mikkotikkanen
Copy link

Also, I noticed that the relativepath reporter configuration doesn't work neither (for the same reason).

However, in node_modules/blanket/src/index.js:149 changing this:

inputFilename = filename.replace(process.cwd(),"");

...into this...

inputFilename = filename.replace(blanket.normalizeBackslashes(process.cwd()),"");

...fixed the issue.

Could you update this into the PR?

@theneva
Copy link

theneva commented Jul 6, 2015

@mikkotikkanen That line doesn't exist on the development branch, which I branched out from due to the contribution guidelines... hopefully @alex-seville can explain how we can handle this to make everyone's lives easier?

@mikkotikkanen
Copy link

Oh. Great.

@alex-seville
Copy link
Owner

Due to a lack of resources for project maintenance, the master branch is effectively the main branch now. If you open a PR against master I may have time to review and merge. Sorry.

@mikkotikkanen
Copy link

Kk. Yeah, that's why the dev branch is usually master since it keeps the maintenance work at minimum, providing the pull requests are proper. :)

@TomGault
Copy link
Contributor Author

TomGault commented Aug 3, 2015

Okay, new PR created! @mikkotikkanen, probably best that you make a new PR for your suggestion.

@TomGault
Copy link
Contributor Author

TomGault commented Aug 3, 2015

#514 is merged now. Thanks everyone!

@TomGault TomGault closed this as completed Aug 3, 2015
@danieltanfh95
Copy link

This bug is still found in the version ^1.1.7 of blanket. I've changed the code like what the OP did and it went away.

@morris
Copy link

morris commented Oct 15, 2015

Yep, it's back in 1.1.7

@TomGault
Copy link
Contributor Author

@alex-seville Could you deploy a new version when you have time? GitHub tells me 1.1.7 was released back in May. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants