Skip to content

test: stop requiring core.autocrlf=true on windows - #63102

Open
nadalaba wants to merge 1 commit into
nodejs:mainfrom
nadalaba:fix-windows-tests
Open

test: stop requiring core.autocrlf=true on windows#63102
nadalaba wants to merge 1 commit into
nodejs:mainfrom
nadalaba:fix-windows-tests

Conversation

@nadalaba

@nadalaba nadalaba commented May 3, 2026

Copy link
Copy Markdown
Contributor

Some tests on Windows (test\parallel\test-snapshot-typescript) require the repo to be checked-out with the git config core.autocrlf set to true.
However, a lot of users prefer to use modern editors that can handle LF and keep core.autocrlf set to false to minimize headaches.
This PR fixes the tests for them without breaking it for others.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels May 3, 2026
@aduh95

aduh95 commented May 4, 2026

Copy link
Copy Markdown
Contributor

Note that we explicitly request it to be set to true:

node/test/README.md

Lines 11 to 12 in f694c3a

For the tests to run on Windows, be sure to clone Node.js source code with the
`autocrlf` git config flag set to true.

Can you split this into two PR so this can be reviewed and backported separately?

@nadalaba
nadalaba force-pushed the fix-windows-tests branch from 867cffd to a0a5162 Compare May 4, 2026 17:06
@nadalaba nadalaba changed the title test: fix tests on windows test: stop requiring core.autocrlf=true on windows May 4, 2026
@nadalaba

nadalaba commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

Can you split this into two PR so this can be reviewed and backported separately?

I opened #63118 for the other test.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actions github-actions Bot added the stale label Aug 3, 2026
@nadalaba

nadalaba commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

This is still relevant. Waiting for review.

@aduh95

aduh95 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

I think in order to land this, we would need some kind of linter or at least make sure some CI would fail if some test makes incorrect assumption about the autocrlf value

@nadalaba
nadalaba force-pushed the fix-windows-tests branch from 0a4578f to 3066cdd Compare August 3, 2026 14:56
Comment thread .github/workflows/coverage-windows.yml Outdated
@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.30%. Comparing base (9766a67) to head (5d03015).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63102      +/-   ##
==========================================
- Coverage   90.30%   90.30%   -0.01%     
==========================================
  Files         759      759              
  Lines      247629   247629              
  Branches    46677    46692      +15     
==========================================
- Hits       223628   223619       -9     
+ Misses      15470    15461       -9     
- Partials     8531     8549      +18     

see 38 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aduh95

aduh95 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

With autocrlf:

Failed tests:
D:\a\node\node\out\Release\node.exe --experimental-vm-modules --max-old-space-size=16 --trace-gc D:\a\node\node\test\es-module\test-vm-source-text-module-leak.js
D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-child-process-exec-any-shells-windows.js
D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-inspector-wait-for-connection.js
D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-util-getcallsites-sourcemap.js

Without autocrlf:

Failed tests:
D:\a\node\node\out\Release\node.exe --experimental-vm-modules --max-old-space-size=16 --trace-gc D:\a\node\node\test\es-module\test-vm-source-text-module-leak.js
D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-child-process-exec-any-shells-windows.js
D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-inspector-wait-for-connection.js
D:\a\node\node\out\Release\node.exe D:\a\node\node\test\parallel\test-util-getcallsites-sourcemap.js

But the issue with the coverage job is that it doesn't report test failures, so we would likely still miss regressions

@nadalaba

nadalaba commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@aduh95 Oh, you're right. Sorry about that.
I can move the matrix job to a new test-windows workflow, but the same 4 tests above will fail. Maybe we should put off this step until we fix these tests.
P.S., #63118 will fix one of them

[EDIT]:
I did the move to a new workflow and skipped the only test that always fails (fixable by #63118).
This should be ready now 🤞

@github-actions github-actions Bot removed the stale label Aug 4, 2026
@nadalaba
nadalaba force-pushed the fix-windows-tests branch from 9d7e7a7 to d4903ca Compare August 5, 2026 17:30
Signed-off-by: Nad Alaba <37968805+nadalaba@users.noreply.github.com>
@nadalaba
nadalaba force-pushed the fix-windows-tests branch from d4903ca to 5d03015 Compare August 5, 2026 20:33
@nadalaba
nadalaba requested a review from aduh95 August 6, 2026 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants