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.

Sign up for GitHub

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3b254a
Fixes CLI tests for windows
kookookchoozeus Apr 16, 2016
a9aa38c
Hijacked exec so we don't have to wrap each command in changeForOS, a…
kookookchoozeus Apr 16, 2016
a6cb014
Fixing changed tests to be more consistent with the code base
kookookchoozeus Apr 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions test/cli-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,33 +9,46 @@ var moduleVersion = require('../package').version;

var exec = require('child_process').exec;

function changeForOS(command) {

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 •
edited
Loading

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.

replace(/\//g, '\\');
Copy link
Collaborator

Choose a reason for hiding this comment

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

When chaining, dots are usually put in the beginning of the line:

if (...) {
  return command
  .replace(...)
  .replace(...)
  .replace(...)
}

Also, I'd say the blank lines inside the function are unnecessary. Specially the first one. Blank lines are mainly used to separate chunks / logic sections, and there's not much to separate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I prefer having dots at the end of a line in these cases because 1) I'm paranoid about JS's semi-colon insertion (even though I've never had a semi-colon inserted where I didn't want one), and 2) just seeing return command sort of implies that that is all you want to return, having the dot at the end implies that's not the end of the statement.

That aside, I will, of course, make my changes consistent with the rest of the code base :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Declaring a new var into which you make the modifications could solve both points:

if (...) {
  var winCommand = command
    .replace(...)
    .replace(...)
    .replace(...)
  return winCommand
}

I've been writing JS without semicolons for over a year and I didn't have any problem, I swear! :)

}

return command;
}

describe('Mustache CLI', function () {

var expectedOutput;

it('writes syntax hints into stderr when runned with wrong number of arguments', function(done) {
exec('bin/mustache', function(err, stdout, stderr) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

You missed un-adding this line, right?

exec(changeForOS('bin/mustache'), function(err, stdout, stderr) {
assert.notEqual(stderr.indexOf('Syntax'), -1);
done();
});
});

it('writes hints about JSON parsing errors when given invalid JSON', function(done) {
exec('echo {name:"lebron"} | bin/mustache - test/_files/cli.mustache', function(err, stdout, stderr) {
exec(changeForOS('echo {name:"lebron"} | bin/mustache - test/_files/cli.mustache'), function(err, stdout, stderr) {
assert.notEqual(stderr.indexOf('Shooot, could not parse view as JSON'), -1);
done();
});
});

it('writes module version into stdout when runned with --version', function(done){
exec('bin/mustache --version', function(err, stdout, stderr) {
exec(changeForOS('bin/mustache --version'), function(err, stdout, stderr) {
assert.notEqual(stdout.indexOf(moduleVersion), -1);
done();
});
});

it('writes module version into stdout when runned with -v', function(done){
exec('bin/mustache -v', function(err, stdout, stderr) {
exec(changeForOS('bin/mustache -v'), function(err, stdout, stderr) {
assert.notEqual(stdout.indexOf(moduleVersion), -1);
done();
});
Expand All @@ -52,7 +65,7 @@ describe('Mustache CLI', function () {
});

it('writes rendered template into stdout when successfull', function(done) {
exec('bin/mustache test/_files/cli.json test/_files/cli.mustache', function(err, stdout, stderr) {
exec(changeForOS('bin/mustache test/_files/cli.json test/_files/cli.mustache'), function(err, stdout, stderr) {
assert.equal(err, null);
assert.equal(stderr, '');
assert.equal(stdout, expectedOutput);
Expand All @@ -62,7 +75,7 @@ describe('Mustache CLI', function () {

it('writes rendered template into the file specified by the third argument', function(done) {
var outputFile = 'test/_files/cli_output.txt';
exec('bin/mustache test/_files/cli.json test/_files/cli.mustache ' + outputFile, function(err, stdout, stderr) {
exec(changeForOS('bin/mustache test/_files/cli.json test/_files/cli.mustache ' + outputFile), function(err, stdout, stderr) {
assert.equal(err, null);
assert.equal(stderr, '');
assert.equal(stdout, '');
Expand All @@ -73,7 +86,7 @@ describe('Mustache CLI', function () {
});

it('reads view data from stdin when first argument equals "-"', function(done){
exec('cat test/_files/cli.json | bin/mustache - test/_files/cli.mustache', function(err, stdout, stderr) {
exec(changeForOS('cat test/_files/cli.json | bin/mustache - test/_files/cli.mustache'), function(err, stdout, stderr) {
assert.equal(err, null);
assert.equal(stderr, '');
assert.equal(stdout, expectedOutput);
Expand All @@ -82,15 +95,18 @@ describe('Mustache CLI', function () {
});

it('writes it couldnt find template into stderr when second argument doesnt resolve to a file', function(done) {
exec('bin/mustache test/_files/cli.json test/_files/non-existing-template.mustache', function(err, stdout, stderr) {
assert.notEqual(stderr.indexOf('Could not find file: test/_files/non-existing-template.mustache'), -1);
exec(changeForOS('bin/mustache test/_files/cli.json test/_files/non-existing-template.mustache'), function(err, stdout, stderr) {
console.log(stderr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

missed removing this console.log.

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. :)

done();
});
});

it('writes it couldnt find view into stderr when first argument doesnt resolve to a file', function(done) {
exec('bin/mustache test/_files/non-existing-view.json test/_files/cli.mustache', function(err, stdout, stderr) {
assert.notEqual(stderr.indexOf('Could not find file: test/_files/non-existing-view.json'), -1);
exec(changeForOS('bin/mustache test/_files/non-existing-view.json test/_files/cli.mustache'), function(err, stdout, stderr) {
assert.notEqual(stderr.indexOf('Could not find file:'), -1);
assert.notEqual(stderr.indexOf(changeForOS('test/_files/non-existing-view.json')), -1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

done();
});
});
Expand All @@ -108,7 +124,7 @@ describe('Mustache CLI', function () {
});

it('writes rendered template with partials into stdout', function(done) {
exec('bin/mustache test/_files/cli_with_partials.json test/_files/cli_with_partials.mustache -p test/_files/cli.mustache -p test/_files/comments.mustache', function(err, stdout, stderr) {
exec(changeForOS('bin/mustache test/_files/cli_with_partials.json test/_files/cli_with_partials.mustache -p test/_files/cli.mustache -p test/_files/comments.mustache'), function(err, stdout, stderr) {
assert.equal(err, null);
assert.equal(stderr, '');
assert.equal(stdout, expectedOutput);
Expand All @@ -117,7 +133,7 @@ describe('Mustache CLI', function () {
});

it('writes rendered template with partials when partials args before required args', function(done) {
exec('bin/mustache -p test/_files/cli.mustache -p test/_files/comments.mustache test/_files/cli_with_partials.json test/_files/cli_with_partials.mustache', function(err, stdout, stderr) {
exec(changeForOS('bin/mustache -p test/_files/cli.mustache -p test/_files/comments.mustache test/_files/cli_with_partials.json test/_files/cli_with_partials.mustache'), function(err, stdout, stderr) {
assert.equal(err, null);
assert.equal(stderr, '');
assert.equal(stdout, expectedOutput);
Expand Down