Making test run on windows #256

moozzyk · 2016-10-15T03:09:45Z

Fixes: #158

moozzyk · 2016-10-15T03:10:29Z

Verified on Windows 10, OS X El Capitan, Ubuntu 14.04

coveralls · 2016-10-15T03:11:13Z

Coverage remained the same at 94.304% when pulling f5d34b4 on moozzyk:moozzyk/bug-158 into 1ceb80b on metalsmith:master.

Fixes: #158

coveralls · 2016-10-15T03:16:25Z

Coverage remained the same at 94.304% when pulling 3d4d2f9 on moozzyk:moozzyk/bug-158 into 1ceb80b on metalsmith:master.

Ajedi32 · 2016-10-17T13:39:23Z

test/index.js

@@ -247,6 +247,8 @@ describe('Metalsmith', function(){
    });

    it('should traverse a symbolic link to a directory', function(done){
+      // symbolic links are *nix specific
+      if (process.platform === 'win32') { return done(); }


FWIW, symbolic links actually do exist in NTFS, and you can create them on Windows with the mklink command if you have Adminstrative privileges. Not sure if we want to support that though. In any case, it's definitely not something that needs to be done in this PR.

Yeah... I have never seen anyone using it...

Ajedi32

Looks pretty good overall, though I do have a few questions/comments...

Ajedi32 · 2016-10-17T13:45:28Z

test/index.js

@@ -247,6 +247,8 @@ describe('Metalsmith', function(){
    });

    it('should traverse a symbolic link to a directory', function(done){
+      // symbolic links are *nix specific
+      if (process.platform === 'win32') { return done(); }


Instead of just automatically passing this test on Windows, I think it'd be more honest to skip() it: https://mochajs.org#inclusive-tests (See the section on skipping at runtime)

Ajedi32 · 2016-10-17T13:46:44Z

test/index.js

@@ -391,6 +393,8 @@ describe('Metalsmith', function(){
    });

    it('should chmod an optional mode from file metadata', function(done){
+      // chmod is not really working on windows https://github.com/nodejs/node-v0.x-archive/issues/4812#issue-11211650
+      if (process.platform === 'win32') { return done(); }


Same as above. Skip instead of pass.

Ajedi32 · 2016-10-17T13:56:13Z

lib/index.js

@@ -280,7 +280,7 @@ Metalsmith.prototype.readFile = unyield(function*(file){

    if (frontmatter && utf8(buffer)) {
      try {
-        parsed = matter(buffer.toString());
+        parsed = matter(buffer.toString().replace(/\r\n/g, '\n'));


Does gray-matter choke on Windows-style line endings? If so, have you looked into trying to fix this problem upstream?

gray-matter fails on this line: https://github.com/jonschlinkert/gray-matter/blob/master/index.js#L98. I originally changed this line to account for \r\n which fixed the problem but the usage as described here implies that \r\n is not something they want to support.

I'm confused. Where in that doc does it state that gray-matter's maintainers are unwilling to support Windows-style line endings?

Maybe I am stretching this a bit but all the usages show only \n and the code itself does not support Windows line endings so my conclusion was that they don't really care about Windows. As you proposed, I might try to add support for Windows line endings in gray-matter and see how it goes. If there is support for \r\n this line will be able to be removed.

Yeah, it's pretty clear Windows isn't currently supported by gray-matter.

My point was that since it's gray-matter's lack of Windows support that's causing Metalsmith to fail here, it's probably best to fix that problem at the root instead of trying to work around it. Doing it that way is more in line with separation of concerns, and has the potential to benefit other projects besides Metalsmith which use gray-matter.

Agreed. Will look into this.

@Ajedi32 FYI: jonschlinkert/gray-matter#32

Ajedi32 · 2016-10-17T13:58:36Z

test/fixtures/cli-plugin-array/.gitattributes

@@ -0,0 +1 @@
+*.html text=auto eol=lf


Interesting. So only the output needs Unix-style line endings? The source .md files can have either line ending style and that doesn't affect the output?

Even on windows the output seems to contain just \n. The problem is that if the line ending is set to autocrlf the baseline files will have \r\n so the tests fails. This change forces \n line endings for html files in the subtree fixing the line ending mismatch.

Interesting. I wonder where that conversion is happening... seems reasonable to me though.

Ajedi32 · 2016-10-17T14:00:53Z

test/fixtures/cli-plugin-array/.gitattributes

@@ -0,0 +1 @@
+*.html text=auto eol=lf


Missing newline at end of file.

Ajedi32 · 2016-10-17T14:03:19Z

test/fixtures/cli-plugin-local/.gitattributes

@@ -0,0 +1 @@
+*.md text=auto eol=lf


Why was this change needed? (Sorry, don't have access to a Windows computer right now that I can test this on.) Were the line endings on the file being output by Metalsmith somehow different from the input file?

See my comment above.

So Metalsmith itself is somehow converting the newline characters to LF? That's really strange. I can understand it happening when converting from Markdown to HTML, but Metalsmith shouldn't be changing anything on its own with no plugins. I guess that's a separate issue though...

Ajedi32 · 2016-10-17T14:04:23Z

test/fixtures/cli-plugin-local/.gitattributes

@@ -0,0 +1 @@
+*.md text=auto eol=lf


Again, missing newline at end of file.

Ajedi32 · 2016-10-17T14:08:46Z

Note to self: add Appveyor for Windows tests.

coveralls · 2016-10-18T04:20:27Z

Coverage remained the same at 94.304% when pulling 79f7c70 on moozzyk:moozzyk/bug-158 into 1ceb80b on metalsmith:master.

moozzyk · 2016-10-18T04:23:15Z

🆙📅

Ajedi32 · 2016-10-19T19:36:29Z

Looks good. I do want to check that issue with the seemingly random newline conversion by Metalsmith though before merging. That seems really strange...

Also might want to talk to grey-matter's maintainer about getting Windows support upstream.

moozzyk · 2016-10-19T20:05:31Z

Can the conversion be actually a side effect of this: https://github.com/metalsmith/metalsmith/pull/256/files#diff-6d186b954a58d5bb740f73d84fe39073R283?

Ajedi32 · 2016-10-19T20:35:21Z

@moozzyk Ah, you're right!

I actually considered that possibility previously but dismissed it since replace doesn't mutate the string or anything (strings in JS are immutable), but now that I look at it again I see that Metalsmith is assigning ret.contents = new Buffer(parsed.content); further down.

So yeah, that's almost certainly the problem. Good catch!

Ajedi32 · 2016-10-19T20:37:56Z

lib/index.js

@@ -280,7 +280,7 @@ Metalsmith.prototype.readFile = unyield(function*(file){

    if (frontmatter && utf8(buffer)) {
      try {
-        parsed = matter(buffer.toString());
+        parsed = matter(buffer.toString().replace(/\r\n/g, '\n'));


Yep, this should probably be changed since it's effectively altering the value of the contents property on the file object further down. Good catch!

Maybe you can kill two birds with one stone here by getting Windows support added upstream in grey-matter.

moozzyk · 2016-10-23T05:06:53Z

@Ajedi32 - I will try to send a PR to gray-matter to fix the crlf issue but even if this is fixed the should require plugins as an array test won't pass. This test use marked.js as a plugin fail because marked.js is using \n line ending all over the place (e.g. https://github.com/chjj/marked/blob/38f1727ffee0820975677027b8968bc1954e6637/lib/marked.js#L805). What is the course of action here?

Ajedi32 · 2016-10-24T14:12:44Z

Hmm. Well, it would be great if marked handled Windows line endings (actually I'm rather surprised to hear it doesn't), but since marked isn't a dependency of the Metalsmith core I don't think that's something that needs to be addressed in this PR.

I'd be fine with you either changing the test to remove the dependency on metalsmith-markdown, or adding a .gitattributes to the cli-plugin-array fixture to ensure that its source files use Unix-style line endings. (The later option would probably be quite a bit easier.)

You might also want to consider filing an issue with marked about supporting Windows line endings, just to document that for future reference.

coveralls · 2016-10-25T15:31:49Z

Coverage remained the same at 94.304% when pulling d3ceba5 on moozzyk:moozzyk/bug-158 into 1ceb80b on metalsmith:master.

coveralls · 2016-10-25T15:34:09Z

Coverage remained the same at 94.304% when pulling 030cd8c on moozzyk:moozzyk/bug-158 into e3f2e12 on metalsmith:master.

moozzyk · 2016-10-25T15:35:38Z

🆙📅

Ajedi32

Looking good! Just a couple more minor tweaks...

Ajedi32 · 2016-10-25T15:37:30Z

.gitignore

@@ -1,3 +1,4 @@
 node_modules
 test/fixtures/*/build
 coverage
+.vscode/


Missing newline at end of file.

Bad habits. It's fixed now.

Ajedi32 · 2016-10-25T15:42:49Z

test/fixtures/cli-plugin-array/.gitattributes

@@ -0,0 +1 @@
+*.html text=auto eol=lf


So metalsmith-markdown works with Windows-style newlines after all? Or is that test still failing for you on Windows?

No. The test is failing on windows because the output has \n while the baseline file has \r\n. I added .gitattributes to enforce \n in the baseline file even on windows to make the test pass.

Ah, okay. Yeah, that's totally fine. We're testing Metalsmith here, not the metalsmith-markdown plugin. Plus I'm not sure that always outputting html files with Unix-style newlines is even a bad idea...

coveralls · 2016-10-25T18:04:28Z

Coverage remained the same at 94.304% when pulling 1aafad3 on moozzyk:moozzyk/bug-158 into e3f2e12 on metalsmith:master.

coveralls · 2016-10-25T18:07:05Z

Coverage remained the same at 94.304% when pulling 7032d2e on moozzyk:moozzyk/bug-158 into e3f2e12 on metalsmith:master.

Ajedi32 · 2016-10-25T18:44:29Z

Gonna test this on my machine before merging...

Ajedi32 · 2016-10-26T04:35:53Z

Gah, Windows doesn't have make. 😝

Hold on, might take me a bit to figure out how to get that working. The GNU Win32 version is just crashing on me... long-term we may want to just use a cross-platform tool like Grunt or something.

moozzyk · 2016-10-26T05:24:07Z

I installed mingw etc. but still running things using makefile did not work due to these lines: https://github.com/metalsmith/metalsmith/blob/master/Makefile#L6-L8 (sorry don't have the error handy). NODE_FLAGS is also not portable. To run tests I just ran:

npm install
node_modules\.bin\mocha

woodyrew · 2016-10-27T19:38:37Z

Gah, Windows doesn't have make. 😝

I think this is a case for using npm scripts instead. I'll create a issue for that.

Ajedi32 · 2016-10-28T04:07:44Z

Yeah, tests work fine on my machine if I just run Mocha directly. Gonna merge this now and we can work on getting the build working without make in a future PR.

Making tests run on windows

3d4d2f9

Fixes: #158

Ajedi32 added the enhancement label Oct 17, 2016

Ajedi32 reviewed Oct 17, 2016

View reviewed changes

Ajedi32 requested changes Oct 17, 2016

View reviewed changes

Addressing review feedback

79f7c70

Ajedi32 approved these changes Oct 19, 2016

View reviewed changes

Ajedi32 requested changes Oct 19, 2016

View reviewed changes

moozzyk mentioned this pull request Oct 23, 2016

Enabling CRLF jonschlinkert/gray-matter#32

Merged

Removing workaround for gray-matter \r\n

030cd8c

Ajedi32 requested changes Oct 25, 2016

View reviewed changes

Adding missing new line

7032d2e

Ajedi32 approved these changes Oct 25, 2016

View reviewed changes

Ajedi32 merged commit 122728c into metalsmith:master Oct 28, 2016

Making test run on windows #256

Making test run on windows #256

Conversation

moozzyk commented Oct 15, 2016

moozzyk commented Oct 15, 2016

coveralls commented Oct 15, 2016 • edited

coveralls commented Oct 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ajedi32 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moozzyk Oct 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moozzyk Oct 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ajedi32 Oct 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ajedi32 commented Oct 17, 2016

coveralls commented Oct 18, 2016 • edited

moozzyk commented Oct 18, 2016

Ajedi32 commented Oct 19, 2016

moozzyk commented Oct 19, 2016

Ajedi32 commented Oct 19, 2016

Ajedi32 Oct 19, 2016 • edited

Choose a reason for hiding this comment

moozzyk commented Oct 23, 2016

Ajedi32 commented Oct 24, 2016

coveralls commented Oct 25, 2016 • edited

coveralls commented Oct 25, 2016 • edited

moozzyk commented Oct 25, 2016

Ajedi32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ajedi32 Oct 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 25, 2016 • edited

coveralls commented Oct 25, 2016 • edited

Ajedi32 commented Oct 25, 2016

Ajedi32 commented Oct 26, 2016 • edited

moozzyk commented Oct 26, 2016

woodyrew commented Oct 27, 2016 • edited

Ajedi32 commented Oct 28, 2016

coveralls commented Oct 15, 2016 •

edited

coveralls commented Oct 15, 2016 •

edited

Ajedi32 left a comment •

edited

moozzyk Oct 18, 2016 •

edited

moozzyk Oct 18, 2016 •

edited

Ajedi32 Oct 19, 2016 •

edited

coveralls commented Oct 18, 2016 •

edited

Ajedi32 Oct 19, 2016 •

edited

coveralls commented Oct 25, 2016 •

edited

coveralls commented Oct 25, 2016 •

edited

Ajedi32 Oct 25, 2016 •

edited

coveralls commented Oct 25, 2016 •

edited

coveralls commented Oct 25, 2016 •

edited

Ajedi32 commented Oct 26, 2016 •

edited

woodyrew commented Oct 27, 2016 •

edited