search

massive-oss / mcover

A cross platform code coverage framework for Haxe with testing and profiling applications. Supports AVM1, AVM2, JavaScript, C++, PHP and Neko.
Other
59 stars 9 forks source link

Fix backslashes missing (or blowing up) on Windows #41

Open 3vilguy opened 7 years ago

3vilguy commented 7 years ago

I was getting an error while trying to run unit tests for JS target.

Uncaught SyntaxError: Invalid Unicode escape sequence
    at massive_munit_client_ExternalPrintClientJS.queue (js_test.js:6799)
    at massive_munit_client_ExternalPrintClientJS.addTestClassCoverageItem (js_test.js:6768)
    at massive_munit_client_RichPrintClient.setCurrentTestClassCoverage (js_test.js:6948)
    at mcover_coverage_munit_client_MCoverPrintClient.updateTestClassCoverage (js_test.js:9429)
    at mcover_coverage_munit_client_MCoverPrintClient.setCurrentTestClass (js_test.js:9397)
    at massive_munit_TestRunner.execute (js_test.js:5730)
    at massive_munit_TestRunner.run (js_test.js:5695)
    at delayStartup (js_test.js:369)
    at TestMain [as __class__] (js_test.js:373)
    at Function.TestMain.main (js_test.js:378)

After bit of digging I found that it's blowing up in PrintClientBase.hx#L510.

Seems like my utils package was causing eval() issues because of \u. After even more digging I finally found place where I can fix it, I came here to do a fork so i can PR and (surprise, surprise) I found THIS COMMIT. I think it was accidentally removed in this commit while fixing split on windows, but looks like this replace line is still needed before using regex. And just "\\\\" are not enough, because eval() is called again in printclient.js#L51.

On top of that this PR should also fix https://github.com/massiveinteractive/mcover/issues/36

Regards :)

elsassph commented 7 years ago

Would it work if we just replaced backslashes by forward slashes on Windows?

3vilguy commented 7 years ago

@elsassph Do you mean just replace it for alternateLocation variable in same place my fix is done (createCodeBlockReference function) ? Or for the classFile, which currently is done by FileSystem.fullPath() and then it's calling createCodeBlockReference function?

3vilguy commented 7 years ago

Just tried: if(IS_WINDOWS) alternateLocation = StringTools.replace(alternateLocation, "\\", "/"); and it seems to be working as well.

elsassph commented 7 years ago

I'm not really familiar with this library honestly (trying to find someone who worked on it), but maybe the path should be sanitized before, in createCodeBlockReference when the alternateLocation is constructed.

misprintt commented 7 years ago

Makes sense. It can be merged once someone has confirmed that it passes all the unit tests (on a windows machine)

3vilguy commented 7 years ago

"Works on My Machine" ™

worksonmymachine worksonmymachine2

But you should double check it with someone else.