Skip to content

Conversation

@avivkeller
Copy link
Member

On many windows machines, lines end with \r\n, rather than just \n. This PR updates the tests to trim off the newlines, so that the test passes on both Windows and Posix machines.

@avivkeller avivkeller requested a review from a team as a code owner March 22, 2025 18:50
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ovflowd
Copy link
Member

ovflowd commented Mar 22, 2025

@araujogui I wonder if this could also be the causes of misleading line numbers on CI? The fact that line termination can be different? I have no idea why the lines could be reported correctly on local runs but different on Node's CI 🤔

@avivkeller avivkeller merged commit c0f0f3c into main Mar 22, 2025
7 checks passed
@avivkeller avivkeller deleted the fix/crlf branch March 22, 2025 22:08
@araujogui
Copy link
Member

araujogui commented Mar 22, 2025

@araujogui I wonder if this could also be the causes of misleading line numbers on CI? The fact that line termination can be different? I have no idea why the lines could be reported correctly on local runs but different on Node's CI 🤔

I don't think this will solve that bug, I tested the script via Docker with a Debian image and it worked as expected.

@ovflowd
Copy link
Member

ovflowd commented Mar 22, 2025

@araujogui I wonder if this could also be the causes of misleading line numbers on CI? The fact that line termination can be different? I have no idea why the lines could be reported correctly on local runs but different on Node's CI 🤔

I don't think this will some that bug, I tested the script via Docker with a Debian image and it worked as expected.

Sorry not sure I got your message. This PR definitely does not fix any bug on that regard, was just commenting what if it was related to it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants