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

Make karma tests work on windows #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

demeritcowboy
Copy link

BEFORE: Running "node node_modules\karma\bin\karma start" doesn't work on windows.
AFTER: It works on windows.

@@ -2,7 +2,11 @@ var execPromise = require('child-process-promise').exec;
var execSync = require('child_process').execSync;

var escape = function(cmd) {
return '\'' + cmd.replace(/'/g, "'\\''") + '\'';
if (process.platform === "win32") {
return '"' + cmd.replace(/"/g, "'") + '"';
Copy link
Member

Choose a reason for hiding this comment

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

Oh, a Windows patch. Cool. 👍

With this regex, though... suppose the content includes a double-quote. Won't this convert it to a single-quote -- and convey different substance? I feel like (on at least a few occasions) I've used cv() to call php:eval, and that that requires some of the params to be passed through quite precisely.

I was curious about how one is supposed to escape a string on Windows, and found https://stackoverflow.com/a/31413730/4195300 -- 😱 Maybe we should either:

  • Figure out which notation is specifically appropriate for launching PHP subcommands on Windows
  • Rework the file so that we don't need to call serializeArgs() or escape() -- basically replacing execSync() with execFileSyc(); the latter should bypass escaping issues.

@demeritcowboy
Copy link
Author

Yes normally it's "" or ^, which wasn't working either so it was confusing and it was trial and error that came up with replacing doubles with singles. I don't know why that works. It might be specific to the commands run just for karma. I can take a look at execfile.

@demeritcowboy
Copy link
Author

Hmm, I used cv php:script and that solves the quoting for the last argument cmd itself but then I still have the same quoting problem calling cv in the first place, i.e. in karma.conf.js
var angularTempFile = cv(['php:script', '-U', _CV.ADMIN_USER, 'karma.conf.php']);
still requires proper quoting for the cv args when it gets used in civicrm-cv.js.

So I think to go this route would require putting the entire cv command, including the call to cv itself, in the external file to be executed, so it's getting a bit convoluted so going to park this for today.

I also did try something else just to see: Git for windows includes a Git Bash shell, however the nodejs/javascript functions don't seem to care, so the original quoting also doesn't work when run via nodejs invoked from the git for windows bash shell.

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

Successfully merging this pull request may close these issues.

2 participants