test: change equal to strictEqual and var to const#9941
Conversation
imyller added the code-and-learn 
princejwesley left a comment
princejwesley left a comment There was a problem hiding this comment.
Commit message suggestion:
First line as test: refactor test-sync-fileread
and the description in subsequent line
test/parallel/test-sync-fileread.js Outdated
There was a problem hiding this comment.
Why the addition of 'utf8'? We should still test the case where the encoding is not provided. If anything, maybe add another assertion.
There was a problem hiding this comment.
According to the documentation:
If the encoding option is specified then this function returns a string. Otherwise it returns a buffer.
So, it seemed to me that the old test with just assert.equal was not actually doing much...although I could be wrong? In any case, I did not see much value in adding another test beyond what was added.
Perhaps I could add an assertion that compares Buffer content of a readFileSync call with no encoding? I am still new to the style and preferences of contributing to the Node.js project, but personally if I was to do such a thing, I would do so in a different PR in an effort to separate refactoring tests and adding tests.
There was a problem hiding this comment.
TL;DR How about just change this line to this?:
assert.strictEqual(fs.readFileSync(fixture).toString(), 'xyz\n`);The reasoning:
This line has the arguments reversed from the documentation. Let's switch them.
To address @cjihrig's comment, leave the
fs.readFileSync()call as it was so that we're not altering the code path (inreadFileSync()) that is being tested. Instead, applytoString()to the result so we can compare it to a string.
Trott commented Dec 16, 2016
Aside: If you (or someone else) wants a good second PR after this lands: Merge this and |
Trott left a comment
Trott left a comment There was a problem hiding this comment.
Ping! @jhwohlgemuth Any chance you can address the make the change requested in https://github.com/nodejs/node/pull/9941/files#r92760787?
jhwohlgemuth commented Dec 22, 2016
@Trott Updated with requested changes. |
Trott commented Dec 22, 2016
italoacasas commented Dec 22, 2016
Landed 5c85ae6 |
change equal to strictEqual and var to const PR-URL: #9941 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
change equal to strictEqual and var to const PR-URL: #9941 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
change equal to strictEqual and var to const PR-URL: #9941 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
change equal to strictEqual and var to const PR-URL: #9941 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
change equal to strictEqual and var to const PR-URL: #9941 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
change equal to strictEqual and var to const PR-URL: #9941 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
change equal to strictEqual and var to const PR-URL: #9941 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]>
11 participants
make -j8 testAffected core subsystem(s)
Description of change
equaltostrictEqual(required addition ofencoding="utf"parameter forreadFileSynccall)vartoconst