Skip to content

ci tests rustfmt, but rustfmt doesn't test ci - #7044

Open
AsthaMishra wants to merge 3 commits into
rust-lang:mainfrom
AsthaMishra:fix-issue-7039
Open

ci tests rustfmt, but rustfmt doesn't test ci#7044
AsthaMishra wants to merge 3 commits into
rust-lang:mainfrom
AsthaMishra:fix-issue-7039

Conversation

@AsthaMishra

@AsthaMishra AsthaMishra commented Aug 22, 2026

Copy link
Copy Markdown
Contributor
Mismatch at ci/src/common.rs:12:
     })
 }
 
-pub                                fn run_command_with_env<I, S>(
+pub fn run_command_with_env<I, S>(
     bin: &str,
     args: I,
     current_dir: &str,

@ytmimi your suggestion works. Confirmed: before adding "ci" to external_crates, self_tests passed even with pub fn run_command_with_env<I, S>( in ci/src/common.rs. After the change it fails with a mismatch on that line.

fixes : #7039

@rustbot rustbot added the S-waiting-on-review Status: awaiting review from the assignee but also interested parties. label Aug 22, 2026
@ytmimi

ytmimi commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

before adding "ci" to external_crates, self_tests passed even with pub fn run_command_with_env<I, S>( in ci/src/common.rs. After the change it fails with a mismatch on that line.

What does this mean? CI is green. Are you saying that there's a formatting error that we're not catching?

@ytmimi

ytmimi commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: awaiting review from the assignee but also interested parties. labels Aug 22, 2026
@rustbot

rustbot commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@AsthaMishra

Copy link
Copy Markdown
Contributor Author

What does this mean?

it works after your suggestion

Are you saying that there's a formatting error that we're not catching?

no

@matthewhughes934

matthewhughes934 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

CI is green. Are you saying that there's a formatting error that we're not catching?

Not directly related to this change (i.e. the issue exists on main), but I see a diff:

/rustfmt$ cargo build
/rustfmt$ cd ./check_diff
/rustfmt/check_diff$ RUSTFMT=../target/debug/rustfmt cargo fmt --check
Diff in /home/mjh/src/rustfmt/check_diff/tests/check_diff.rs:1:
 use check_diff::{
-    CheckDiffError, DiffChecker, CodeFormatter, FormatCodeError, Repository,
-    RustFmtFileFinder, check_diff,
+    CheckDiffError, CodeFormatter, DiffChecker, FormatCodeError, Repository, RustFmtFileFinder,
+    check_diff,
 };
 use std::fs::File;
 use tempfile::Builder;

@AsthaMishra

Copy link
Copy Markdown
Contributor Author

@matthewhughes934 because we are only checking files of src folder in external_crate

rustfmt/src/test/mod.rs

Lines 489 to 506 in 1191d91

let external_crates = vec!["check_diff", "config_proc_macro"];
for external_crate in external_crates {
let mut path = PathBuf::from(external_crate);
path.push("src");
let directory = fs::read_dir(&path).unwrap();
let search_files = directory.filter_map(|file| {
file.ok().and_then(|f| {
let name = f.file_name();
if matches!(name.as_os_str().to_str(), Some("main.rs" | "lib.rs")) {
Some(f.path())
} else {
None
}
})
});
for file in search_files {
files.push(file);
}

This fixes the external_crate/tests/ directory:

let mut tests_files = get_test_files(&PathBuf::from(external_crate).join("tests"), false);
files.append(&mut tests_files);

@AsthaMishra

Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. labels Aug 24, 2026
@ytmimi

ytmimi commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@AsthaMishra has the test directory issue been fixed? We should probably just take care of that in this PR too.

@rustbot rustbot added the A-CI Area: CI label Aug 24, 2026
@AsthaMishra

Copy link
Copy Markdown
Contributor Author

@ytmimi yes, pushed

Comment thread src/test/mod.rs Outdated
files.push(file);
}

let mut tests_files = get_test_files(&PathBuf::from(external_crate).join("tests"), false);

@ytmimi ytmimi Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be fine for us to recursively look for .rs files within the "tests" directory of check_diff, config_proc_macro, and ci since those test files don't have a special structure to them like tests/source and tests/target do for the rustfmt test suite.

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

pushed

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

Labels

A-CI Area: CI S-waiting-on-review Status: awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci tests rustfmt, but rustfmt doesn't test ci

4 participants