test: update test-buffer-alloc to use template literals #15867
Conversation
bwhitty
changed the title
bwhitty
force-pushed
the
test-buffer-alloc-tpl-lit
branch
from
4705507
to
3f1b1f5
Compare
Trott
added
the
code-and-learn
Updated a single usage of string concatenation to use template literals. There are other usages of string concatenation in this file, but they are used to make test code more readable.
bwhitty
force-pushed
the
test-buffer-alloc-tpl-lit
branch
from
3f1b1f5
to
34e7486
Compare
There was a problem hiding this comment.
Hi @bwhitty! Welcome and thanks for this!
This change adds leading white space to each line of the string (except the first). What do you think of something more like this?:
const expectedWhite = `${expected.slice(0, 60)}\n` +
`${expected.slice(60, 120)}\n` +
...Alternatively, you can put all the expected.slice() calls into an array and use .join('\n') on the array. (You'll still need to append on '\n' at the end of the array in that case.)
|
Hey @Trott ! Thanks for the review. This code appears to specifically be testing that whitespace is ignored (see the comment right above my diff) and the test continues to pass... I guess I'd argue this change is actually keeping the test assertion the same, just with more whitespace, and moderately more readable code. Changing anything in this file to use tpl strings is sketchy at best as it's What do you think? |
|
I think this is the way to go: const expectedWhite = `${expected.slice(0, 60)}\n` +
`${expected.slice(60, 120)}\n` +
...It preserves the exact string already used in the test but gets rid of concatenation within lines. |
|
Fixed up, @Trott ! It still makes me feel uncomfortable that my first change did not cause the tests to fail. |
|
It all looks unrelated, but there's a bit too much red in that last CI run: https://ci.nodejs.org/job/node-test-pull-request/10759/ |
|
Ping @Trott |
BridgeAR
dismissed
Trott’s stale review
Comment got addressed. Just removing the review to land this.
|
Landed in e021de3 Thanks for the PR, and congratulations on becoming a Node.js Contributor 🎉 ! |
PR-URL: #15867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
base64 encoding in |
PR-URL: #15867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs/node#15867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #15867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #15867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #15867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs/node#15867 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
tests