From 85a2943435d645c0b45e27e4f0312b5434e1fb65 Mon Sep 17 00:00:00 2001 From: Zheyu Zhang Date: Sat, 30 Oct 2021 15:59:53 +0800 Subject: [PATCH] fix(cli): lint/format all discoverd files on each change (#12518) --- cli/tests/integration/watcher_tests.rs | 100 ++++++++++++++++++++++++- cli/tools/fmt.rs | 60 +++++++++------ cli/tools/lint.rs | 37 ++++----- 3 files changed, 154 insertions(+), 43 deletions(-) diff --git a/cli/tests/integration/watcher_tests.rs b/cli/tests/integration/watcher_tests.rs index feda1bac70..ce78485276 100644 --- a/cli/tests/integration/watcher_tests.rs +++ b/cli/tests/integration/watcher_tests.rs @@ -55,6 +55,10 @@ fn wait_for(s: &str, lines: &mut impl Iterator) { } } +fn read_line(s: &str, lines: &mut impl Iterator) -> String { + lines.find(|m| m.contains(s)).unwrap() +} + fn check_alive_then_kill(mut child: std::process::Child) { assert!(child.try_wait().unwrap().is_none()); child.kill().unwrap(); @@ -77,6 +81,8 @@ fn lint_watch_test() { let t = TempDir::new().expect("tempdir fail"); let badly_linted_original = util::testdata_path().join("lint/watch/badly_linted.js"); + let badly_linted_output = + util::testdata_path().join("lint/watch/badly_linted.js.out"); let badly_linted_fixed1 = util::testdata_path().join("lint/watch/badly_linted_fixed1.js"); let badly_linted_fixed1_output = @@ -86,8 +92,6 @@ fn lint_watch_test() { let badly_linted_fixed2_output = util::testdata_path().join("lint/watch/badly_linted_fixed2.js.out"); let badly_linted = t.path().join("badly_linted.js"); - let badly_linted_output = - util::testdata_path().join("lint/watch/badly_linted.js.out"); std::fs::copy(&badly_linted_original, &badly_linted) .expect("Failed to copy file"); @@ -139,6 +143,54 @@ fn lint_watch_test() { drop(t); } +#[test] +fn lint_all_files_on_each_change_test() { + let t = TempDir::new().expect("tempdir fail"); + let badly_linted_fixed0 = + util::testdata_path().join("lint/watch/badly_linted.js"); + let badly_linted_fixed1 = + util::testdata_path().join("lint/watch/badly_linted_fixed1.js"); + let badly_linted_fixed2 = + util::testdata_path().join("lint/watch/badly_linted_fixed2.js"); + + let badly_linted_1 = t.path().join("badly_linted_1.js"); + let badly_linted_2 = t.path().join("badly_linted_2.js"); + std::fs::copy(&badly_linted_fixed0, &badly_linted_1) + .expect("Failed to copy file"); + std::fs::copy(&badly_linted_fixed1, &badly_linted_2) + .expect("Failed to copy file"); + + let mut child = util::deno_cmd() + .current_dir(util::testdata_path()) + .arg("lint") + .arg(&t.path()) + .arg("--watch") + .arg("--unstable") + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .expect("Failed to spawn script"); + let mut stderr = child.stderr.as_mut().unwrap(); + let mut stderr_lines = std::io::BufReader::new(&mut stderr) + .lines() + .map(|r| r.unwrap()); + + std::thread::sleep(std::time::Duration::from_secs(1)); + + assert_contains!(read_line("Checked", &mut stderr_lines), "Checked 2 files"); + + std::fs::copy(&badly_linted_fixed2, &badly_linted_2) + .expect("Failed to copy file"); + std::thread::sleep(std::time::Duration::from_secs(1)); + + assert_contains!(read_line("Checked", &mut stderr_lines), "Checked 2 files"); + + assert!(child.try_wait().unwrap().is_none()); + + child.kill().unwrap(); + drop(t); +} + #[test] fn fmt_watch_test() { let t = TempDir::new().unwrap(); @@ -180,6 +232,50 @@ fn fmt_watch_test() { check_alive_then_kill(child); } +#[test] +fn fmt_check_all_files_on_each_change_test() { + let t = TempDir::new().unwrap(); + let badly_formatted_original = + util::testdata_path().join("badly_formatted.mjs"); + let badly_formatted_1 = t.path().join("badly_formatted_1.js"); + let badly_formatted_2 = t.path().join("badly_formatted_2.js"); + std::fs::copy(&badly_formatted_original, &badly_formatted_1).unwrap(); + std::fs::copy(&badly_formatted_original, &badly_formatted_2).unwrap(); + + let mut child = util::deno_cmd() + .current_dir(util::testdata_path()) + .arg("fmt") + .arg(&t.path()) + .arg("--watch") + .arg("--check") + .arg("--unstable") + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .unwrap(); + let (_stdout_lines, mut stderr_lines) = child_lines(&mut child); + + // TODO(lucacasonato): remove this timeout. It seems to be needed on Linux. + std::thread::sleep(std::time::Duration::from_secs(1)); + + assert_contains!( + read_line("error", &mut stderr_lines), + "Found 2 not formatted files in 2 files" + ); + + // Change content of the file again to be badly formatted + std::fs::copy(&badly_formatted_original, &badly_formatted_1).unwrap(); + + std::thread::sleep(std::time::Duration::from_secs(1)); + + assert_contains!( + read_line("error", &mut stderr_lines), + "Found 2 not formatted files in 2 files" + ); + + check_alive_then_kill(child); +} + #[test] fn bundle_js_watch() { use std::path::PathBuf; diff --git a/cli/tools/fmt.rs b/cli/tools/fmt.rs index 9ac9557cd3..2cc276c5b4 100644 --- a/cli/tools/fmt.rs +++ b/cli/tools/fmt.rs @@ -81,25 +81,36 @@ pub async fn format( let resolver = |changed: Option>| { let files_changed = changed.is_some(); - let result = - collect_files(&include_files, &exclude_files, is_supported_ext_fmt).map( - |files| { - let collected_files = if let Some(paths) = changed { - files - .into_iter() - .filter(|path| paths.contains(path)) - .collect::>() + + let collect_files = + collect_files(&include_files, &exclude_files, is_supported_ext_fmt); + + let (result, should_refmt) = match collect_files { + Ok(value) => { + if let Some(paths) = changed { + let refmt_files = value + .clone() + .into_iter() + .filter(|path| paths.contains(path)) + .collect::>(); + + let should_refmt = !refmt_files.is_empty(); + + if check { + (Ok((value, fmt_options.clone())), Some(should_refmt)) } else { - files - }; - (collected_files, fmt_options.clone()) - }, - ); + (Ok((refmt_files, fmt_options.clone())), Some(should_refmt)) + } + } else { + (Ok((value, fmt_options.clone())), None) + } + } + Err(e) => (Err(e), None), + }; + let paths_to_watch = include_files.clone(); async move { - if (files_changed || !watch) - && matches!(result, Ok((ref files, _)) if files.is_empty()) - { + if files_changed && matches!(should_refmt, Some(false)) { ResolutionResult::Ignore } else { ResolutionResult::Restart { @@ -121,13 +132,16 @@ pub async fn format( if watch { file_watcher::watch_func(resolver, operation, "Fmt").await?; } else { - let (files, fmt_options) = - if let ResolutionResult::Restart { result, .. } = resolver(None).await { - result? - } else { - return Err(generic_error("No target files found.")); - }; - operation((files, fmt_options)).await?; + let files = + collect_files(&include_files, &exclude_files, is_supported_ext_fmt) + .and_then(|files| { + if files.is_empty() { + Err(generic_error("No target files found.")) + } else { + Ok(files) + } + })?; + operation((files, fmt_options.clone())).await?; } Ok(()) diff --git a/cli/tools/lint.rs b/cli/tools/lint.rs index 6948d2a1f8..1f85e8b02a 100644 --- a/cli/tools/lint.rs +++ b/cli/tools/lint.rs @@ -105,26 +105,27 @@ pub async fn lint( let resolver = |changed: Option>| { let files_changed = changed.is_some(); - let result = collect_files( - &*include_files.clone(), - &*exclude_files.clone(), - is_supported_ext, - ) - .map(|files| { - if let Some(paths) = changed { - files - .into_iter() - .filter(|path| paths.contains(path)) - .collect::>() - } else { - files + let collect_files = + collect_files(&include_files, &exclude_files, is_supported_ext); + + let paths_to_watch = include_files.clone(); + + let (result, should_relint) = match collect_files { + Ok(value) => { + if let Some(paths) = changed { + ( + Ok(value.clone()), + Some(value.iter().any(|path| paths.contains(path))), + ) + } else { + (Ok(value), None) + } } - }); - let paths_to_watch = args.clone(); + Err(e) => (Err(e), None), + }; + async move { - if (files_changed || !watch) - && matches!(result, Ok(ref files) if files.is_empty()) - { + if files_changed && matches!(should_relint, Some(false)) { ResolutionResult::Ignore } else { ResolutionResult::Restart {