Skip to content

editorconfig: don't trim trailing whitespace in tests #144642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lolbinarycat
Copy link
Contributor

some test snapshot files require trailing whitespace, and previously manually editing those snapshot files (as is required for run-make tests and some platform-specific tests) in an editor with editorconfig support would cause that whitespace to be removed, causing CI failures like this one

@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2025

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2025

Some changes occurred in src/tools/cargo

cc @ehuss

@rustbot

This comment has been minimized.

@lolbinarycat lolbinarycat force-pushed the editorconfig-no-run-make branch from 9f764bd to 504a2c1 Compare July 29, 2025 17:01
@mati865
Copy link
Member

mati865 commented Jul 31, 2025

How about disabling it only for stdout and stderr files in tests?

@lolbinarycat
Copy link
Contributor Author

@mati865 there's a few run-make tests that use other file extensions, so collecting a comprehensive list might be a bit tricky.

i think more reliable maybe would be disabling it for all tests/ files except those written in rust and js?

@mati865
Copy link
Member

mati865 commented Jul 31, 2025

@lolbinarycat that sounds sensible to me but I'd leave the decision to the reviewer.

@ehuss
Copy link
Contributor

ehuss commented Aug 6, 2025

r? compiler

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 6, 2025
@rustbot rustbot assigned lcnr and unassigned ehuss Aug 6, 2025
@lcnr
Copy link
Contributor

lcnr commented Aug 7, 2025

I would personally also prefer to limit this restriction only to affected tests. I think it's fine for this to be an ad-hoc list of tests where it matters. I would personally love there to be a tidy check which makes sure we keep some //@ editor-no-trim-whitespace attribute in sync with an opt out in the editor config (and some way to bless the editor config file)

But 🤷 I think only listing the effected test in the editor config would also be fine, don't have any strong opinions here

@lolbinarycat
Copy link
Contributor Author

I would personally also prefer to limit this restriction only to affected tests.

There are over 850 affected tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants