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

Fix nunjucks for windows: relative paths and tests #391

Merged
merged 10 commits into from
Mar 11, 2015
Merged

Fix nunjucks for windows: relative paths and tests #391

merged 10 commits into from
Mar 11, 2015

Conversation

SamyPesse
Copy link
Contributor

The goals of this PR are:

  • Fix relative paths on Windows
  • Adapt tests for windows (some tests are using \n and on windows it's \r\n)
  • Maybe if you want to, adapt tests script to work with Appveyor

@SamyPesse
Copy link
Contributor Author

The main problem with unit tests is that we are comparing strings that include \n like at https://github.com/mozilla/nunjucks/blob/master/tests/compiler.js#L326

This is failing on Windows because it's replaced by \r\n.

@jlongster Should I adapt the tests to use os.EOL or should we normalize the output or render to normalize EOLs?

Or we can just normalize tests by wrapping the render with .replace('\r\n', '\n')

@jlongster
Copy link
Contributor

@SamyPesse hm that's tricky. Any chance you know what other projects do? Is it safe to normalize it?

@SamyPesse
Copy link
Contributor Author

This is maybe safer to just normalize tests for now, for example at https://github.com/mozilla/nunjucks/blob/master/tests/compiler.js#L326:

expect(normEOL(res)).to.be('somecontenthere\n');

with:

function normEOL(s) {
    return s.replace("\r\n", "\n");
}

ok ?

@jlongster
Copy link
Contributor

That works for me!

@jlongster
Copy link
Contributor

I'd say we only need to do it in tests where we need to compare the \n.

@SamyPesse
Copy link
Contributor Author

Tests are now working on Windows, I'm testing setup tests on Appveyor (for my fork).

the use of make for tests is not really usefull and I'm not usre it'll work by default on windows (make is not installed with node).
The use of make at https://github.com/mozilla/nunjucks/blob/master/package.json#L23 can be replaced by only using npm scripts. Can I include this in my PR ?

@SamyPesse
Copy link
Contributor Author

BTW here are the builds that I'm testing using AppVeyor: https://ci.appveyor.com/project/SamyPesse/nunjucks

@SamyPesse
Copy link
Contributor Author

There is a very important issue in Mocha for windows that has been fixed in a merged PR but not yet release to NPM: mochajs/mocha#333 (comment)

@jlongster
Copy link
Contributor

What the issue we experience because of that bug? They say they won't backport to 0.10 but I still want to support that.

@SamyPesse
Copy link
Contributor Author

Basically stdout and stderr are not flushed and so not visible in the build logs. The only other workaround is to use the --no-exit option of mocha, but in this case builds fail because of timeout (but logs are available ...).

I'm running some tests, but I fix this is not important if logs are not available for some windows builds, it's enough to know that it's failing and getting the logs for node 0.11, because if it's failing only for 0.10/0.8, it will probably fail for the same reason on travis (where the logs are always available).

Sorry if my comment is not clear enough, I'm not an english native :)

@SamyPesse
Copy link
Contributor Author

🎉 🎈 🚀 Tests are passing on AppVeyor: https://ci.appveyor.com/project/SamyPesse/nunjucks/build/job/94ovau3gea1co8nr !

At least on 0.11, I'm still waiting for 0.8 and 0.10: https://ci.appveyor.com/project/SamyPesse/nunjucks

@SamyPesse
Copy link
Contributor Author

@jlongster this is ready to be merged :)

I disabled windows builds for node 0.8 because it was always failing without being related to Nunjucks: https://ci.appveyor.com/project/SamyPesse/nunjucks/build/1.0.3/job/nd0nj203yqeods0c
Having tests for 0.10 and 0.11 on Windows is enough and better than nothing.

You can signup to appveyor and create a project for nunjucks if you want to enable windows testing, otherwise at least it's ready for it :)

@nijikokun
Copy link

👍

@@ -52,6 +52,11 @@
}
}

function normEOL(str) {
if (!str) return str;
return str.replace(/\r\n|\r|\n/g, "\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

\n needn't be replaced, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean using /\r\n|\r/g instead? I think it will work fine, yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll replace it

@popomore
Copy link
Contributor

LGTM

jlongster added a commit that referenced this pull request Mar 11, 2015
Fix nunjucks for windows: relative paths and tests
@jlongster jlongster merged commit 3664163 into mozilla:master Mar 11, 2015
@SamyPesse
Copy link
Contributor Author

Thanks 👍 🍻

@jlongster
Copy link
Contributor

@SamyPesse thanks for your work on it! As far as Appveyor is concerned, is your project here (https://ci.appveyor.com/project/SamyPesse/nunjucks) testing mozilla's project or your local fork? I added Mozilla's repo but its showing up under my name: https://ci.appveyor.com/project/jlongster/nunjucks

We can keep it under your name if its testing this repo. I'll add a link to the README.

@SamyPesse
Copy link
Contributor Author

It's testing my fork, I can't enable it to test this repo (since I don't have access to the repo webhooks).

@popomore
Copy link
Contributor

https://ci.appveyor.com/project/jlongster/nunjucks should work now.

@jlongster you can register a mozilla account if want the link project/mozilla/nunjucks, or rename your account.

http://help.appveyor.com/discussions/questions/744-using-appveyor-for-a-github-org

@jlongster
Copy link
Contributor

Ok, thanks. I'll keep it under jlongster because I don't want to manage the mozilla account at Appveyor. As long as it's testing this repo.

@SamyPesse
Copy link
Contributor Author

Any news on when the latest updates will be available on NPM? :)

@jlongster
Copy link
Contributor

Because of some of the bugs we found I wanted to make sure there wasn't anything else outstanding with this. And I got busy recently. I will put some time into it this week though and make a release if it looks stable.

@jlongster
Copy link
Contributor

@SamyPesse just pushed out v1.3.0. Thanks for all your work! Is there any chance you could add documentation for the features you added in the /docs folder? Particularly for relative templates, but also for the noCache option in FileSystemLoader.

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.

4 participants