test: lint test-fs-non-number-arguments-throw

[silverwind]

This test was excluded from linting changes because of a #1737.

I'm going to check if the issue is still present on the CI through this PR.

[silverwind]

CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/129/

[bnoordhuis]

LGTM in theory but the test is failing on the Windows buildbots.

[silverwind]

Still reading one position too early on Windows, hmm. Must be line-ending related.

[silverwind]

I think it's related to the git checkout on the CI, attempting a fix through text=auto in .gitattributes.

New CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/172/

[silverwind]

So the fix didn't help because I don't think CRLF normalization applies when the checkout is performed.

@nodejs/build can someone try modifiying this command on the CI:

git -c core.askpass=true fetch --tags --progress https://github.com/nodejs/node.git +refs/heads/*:refs/remotes/origin/* +refs/pull/2494/head:refs/remotes/origin/_jenkins_local_branch

so it includes core.autocrlf=true, and then rerun test-pull-request on this PR?

[joaocgreis]

@silverwind core.autocrlf=true is set global on all windows machines.

[silverwind]

@joaocgreis In that case, may I ask to disable that option for a quick test run with this PR? Just want to confirm that this issue is checkout-related.

[joaocgreis]

@silverwind I checked this locally, it is indeed because of the line ending. I converted the line endings to UNIX on that test alone and it passes. Can you change the test to use the first line?

[silverwind]

@joaocgreis unfortunately not, because we want all tests to run in strict mode.

I assume the CRLF normalization affects only patches applied by the CI. My theory is that once this commit is in the tree, the CI would pass. I think we should just disabled CRLF normalization on Windows CI hosts, because we don't do it in the repo either (no text=auto in .gitattributes).

[joaocgreis]

I didn't mean to remove the first like, I meant to use it to compare. Something like changing sanity to be sanity = 'e strict\';' and s/31/3/ and s/43/13/ . Would that work?

[silverwind]

Yeah, but then all other platforms would fail that test. I think the only sane way is to fix the CI.

[joaocgreis]

The first line has no leading new line, so it would be the same for all platforms. That's how it was before. I think the test would be correct that way.

CI is not broken, it's actually doing it's job pretty well 😄 autocrlf is common to use on windows, having something that is not compatible with it would be a problem.

[silverwind]

Still not totally convinced 😉

@nodejs/platform-windows any opinions if we should disable autocrlf for commits pulled into the CI or if it's better to add platform-specific read indexes to this test?

[joaocgreis]

No need for platform-specific indexes! I would also be against that. I meant to change the test to this:

'use strict';
var assert = require('assert'),
    fs = require('fs'),
    saneEmitter,
    sanity = 'e strict\';';

saneEmitter = fs.createReadStream(__filename, { start: 3, end: 12 });

assert.throws(function() {
  fs.createReadStream(__filename, { start: '3', end: 12 });
}, "start as string didn't throw an error for createReadStream");

assert.throws(function() {
  fs.createReadStream(__filename, { start: 3, end: '12' });
}, "end as string didn't throw an error");

assert.throws(function() {
  fs.createWriteStream(__filename, { start: '3' });
}, "start as string didn't throw an error for createWriteStream");

saneEmitter.on('data', function(data) {
  // a sanity check when using numbers instead of strings
  assert.strictEqual(sanity, data.toString('utf8'), 'read ' +
                     data.toString('utf8') + ' instead of ' + sanity);
});

(end on 12 instead of 13 as I said above, sorry)

[domenic]

any opinions if we should disable autocrlf for commits pulled into the CI or if it's better to add platform-specific read indexes to this test?

I strongly support disabling autocrlf. You're essentially checking out a different repo; no wonder your tests don't pass.

[silverwind]

@joaocgreis I see what you mean, but that's just a workaround by not reading past the newline 😉

[orangemocha]

autocrlf is the default for git on Windows and I think it would be better to run CI in the same mode that the vast majority of Windows users use git.

Would expressing the indexes in terms of c1 + c2 * os.EOL.length make the test portable?

[orangemocha]

...or make git treat the file (or a separate test file) as binary.

[domenic]

What about making it treat all files as binary, so as to avoid autocrlf?

[rmg]

@domenic that would mean CI would remain green even if someone added something ridiculous like if (c == '\r') { abort(); } deep inside some v8 or uv code, unless we added a binary marked file specifically for sanity testing CRLF cases.

[domenic]

I just think expecting Node to run when you change all the line endings is absolutely crazy.

[rmg]

I think a lot of people have far crazier expectations of node than that :-)

[silverwind]

I think it's a bit of bad practice that this test reads itself actually, and I think I'll refactor that test to create a temp file and read that. I'm still leaning towards disabling CRLF for the source checkouts done by the CI.

Tests that work on files with CRLF line-endings should ideally create them themselves and not rely on files present in the tree, because we i assume we don't want mixed formats and all files in the tree should be LF only.

[orangemocha]

I am -1 for changing the autocrlf away from default behavior, and also for treating all files as binary. Line endings on Windows are CRLF and we cannot change Windows - even in the context of Node.js - to make them the same as on the Unices. In my opinion, the correct solution is to write our code - and this test - in a portable way, not making any assumptions on what the line-ending sequence is and relying on os.EOL instead.

[silverwind]

Refactored the test to use a temp file, which makes it independent on line ending.

CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/203/

[rmg]

Refactored the test to use a temp file, which makes it independent on line ending.

👍 thanks @silverwind

[silverwind]

Squashed and made a small tweak so it's more in line with the old test.

PTAL @bnoordhuis @thefourtheye

[silverwind]

One more CI for good measure: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/207/

[silverwind]

Forgot a semicolon, fixed now.

[joaocgreis]

LGTM

@silverwind thanks for adapting the test!

[silverwind]

Thanks, landed in 83c2c3b!