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

Fixes CLI tests for windows #560

Merged
merged 3 commits into from
Apr 24, 2016
Merged

Fixes CLI tests for windows #560

merged 3 commits into from
Apr 24, 2016

Conversation

kookookchoozeus
Copy link
Contributor

Fixes #559

All that was needed was a helper function for transforming *nix commands into windows commands:

  • Makes sure bin/mustache is run with node
  • Replaces cat with type
  • Replaces forward slashes with backward slashes

Leaves commands as they are for *nix, so won't break the tests there

Also replaced the Could not find file strings to be compatible for windows

@dasilvacontin
Copy link
Collaborator

dasilvacontin commented Apr 16, 2016

Thanks @kookookchoozeus!

I think it would be cleaner to hijack exec calls so that you don't need to explicitly use changeOS around, alla:

var child_process = require('child_process')

function exec () {
  arguments[0] = changeOS(arguments[0])
  return child_process.exec.apply(child_process, arguments)
}

if(process.platform === 'win32') {
return command.
replace(/bin\/mustache/g, 'node bin\\mustache').
replace(/cat /g, 'type ').
Copy link
Collaborator

@dasilvacontin dasilvacontin Apr 16, 2016

Choose a reason for hiding this comment

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

I think it would be preferable to wrap the regexes with \b symbols, like /\bcat\b/. Right now any word/filename ending with cat will instead end with type. It's an unlikely scenario, but it's existence makes the solution not as clean, I feel.

…nd made replacing 'cat' safer with word boundaries
@kookookchoozeus
Copy link
Contributor Author

Thanks @dasilvacontin, very good suggestions that I should have realized myself, haha.

assert.notEqual(stderr.indexOf('Could not find file: test/_files/non-existing-template.mustache'), -1);
console.log(stderr);
assert.notEqual(stderr.indexOf('Could not find file:'), -1);
assert.notEqual(stderr.indexOf(changeForOS('test/_files/non-existing-template.mustache')), -1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain why the assertion was split into two different ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The filepath is absolute on Windows (e.g. C:\Users\you\path\to\mustache\test_files...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. That's not very obvious, just by reading the code..

I was going to suggest using a regex, eg /Could not find file.*non-existing-template\.mustache/., but don't worry about it, I'm going to refactor these assertions after we merge this PR (using unexpected or some nice assertion library).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial thought was to use a regex as well, but then you'd also have to take the backslash problem into account with [\/\\]{1,2}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not if you only add the last part of the file's path to the regex, you have no worries about the slashes that way. :)

@dasilvacontin
Copy link
Collaborator

And I just noticed I used fs instead of child_process in my example. :')

@kookookchoozeus
Copy link
Contributor Author

Update: made the changes suggested above, hoping to get this merged soon so I can update #558

@dasilvacontin
Copy link
Collaborator

LGTM, thanks @kookookchoozeus! :)

@dasilvacontin dasilvacontin merged commit 329bfd0 into janl:master Apr 24, 2016
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