assert: do not use EOL in ERR_ASSERTION messages #19221
Conversation
On Windows if an error is thrown from a script that uses `\n` to break lines - which is very common in the JavaScript ecosystem, and is the case in our own code base - then the error messages would contain mixed line feeds: the part coming from the source code breaks with `\n` while the message itself break with `\r\n`. Since we do not use `\r\n` in util.inspect(), we should use `\n` in error messages as well.
nodejs-github-bot
added
the
assert
|
I am wondering why this does not fail on the CI but fails on my Windows 10 machine. I have EDIT: huh, I guess it's because the windows CI does not run the linter so it just converts LF in the code to CRLF and gets away with it.. |
|
Refs: #18967 |
|
Is this semver-major? |
|
@vsemozhetbyt this is not semver-major because it relies on a semver-major feature. I added the do not land labels. |
|
@joyeecheung this currently fails on the CI. When I implemented this I had the exact same issue when first using |
|
@nodejs/build Can we set |
joyeecheung
added
the
errors
|
Instead of setting a git config option, wouldn't it be better to add a line to |
|
@MSLaguana That sounds like a better idea, I'll try to figure out the necessary update to |
|
We should try to respect naive EOL where possible. I believe a better solution would be to mark this test explicitly for EOL normalization, adding something like this to |
|
Superseded by #20754. Closing. |
On Windows if an error is thrown from a script that uses
\nto break lines - which is very common in the JavaScript ecosystem,
and is the case in our own code base -
then the error messages would contain mixed line feeds:
the part coming from the source code breaks with
\nwhile themessage itself break with
\r\n.Since we do not use
\r\nin util.inspect(), we should use\nin error messages as well.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passescc @BridgeAR