test: change equal to strictEqual and var to const #9941
Conversation
imyller
added
the
code-and-learn
|
|
||
| assert.equal('xyz\n', fs.readFileSync(fixture)); | ||
| assert.strictEqual('xyz\n', fs.readFileSync(fixture, 'utf8')); |
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.
|
Aside: If you (or someone else) wants a good second PR after this lands: Merge this and |
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?
|
@Trott Updated with requested changes. |
|
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]>
make -j8 testAffected core subsystem(s)
Description of change
equaltostrictEqual(required addition ofencoding="utf"parameter forreadFileSynccall)vartoconst