From b4ff3803d24a0debd1b960757652a1152a267c76 Mon Sep 17 00:00:00 2001 From: Alex Butler Date: Fri, 7 Dec 2018 17:28:48 +0000 Subject: [PATCH 1/6] Split std tooltip tests out to allow others to run always Remove --cfg=enabled_tooltip_tests jazz --- .travis.yml | 3 ++- src/actions/hover.rs | 39 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 1af6399823a..699891faf3a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -20,9 +20,10 @@ install: | script: - cargo build -v - cargo test -v +- cargo test test_tooltip_std -- --ignored + env: global: - - RUSTFLAGS=--cfg=enable_tooltip_tests - RUST_BACKTRACE=1 - RLS_TEST_WAIT_FOR_AGES=1 - CARGO_INCREMENTAL=0 diff --git a/src/actions/hover.rs b/src/actions/hover.rs index 24d9504e34f..aa800beb682 100644 --- a/src/actions/hover.rs +++ b/src/actions/hover.rs @@ -2030,8 +2030,6 @@ pub mod test { } #[test] - // doesn't work in the rust-lang/rust repo, enable on CI - #[cfg_attr(not(enable_tooltip_tests), ignore)] fn test_tooltip() -> Result<(), Box> { use self::test::{LineOutput, Test, TooltipTestHarness}; use std::env; @@ -2090,6 +2088,43 @@ pub mod test { Test::new("test_tooltip_mod_use_external.rs", 12, 12), Test::new("test_tooltip_mod_use_external.rs", 14, 12), Test::new("test_tooltip_mod_use_external.rs", 15, 12), + ]; + + let cwd = env::current_dir()?; + let out = LineOutput::default(); + let proj_dir = cwd.join("test_data").join("hover"); + let save_dir = cwd + .join("target") + .join("tests") + .join("hover") + .join("save_data"); + let load_dir = proj_dir.join("save_data"); + + let harness = TooltipTestHarness::new(proj_dir, &out); + + out.reset(); + + let failures = harness.run_tests(&tests, load_dir, save_dir)?; + + if failures.is_empty() { + Ok(()) + } else { + eprintln!("{}\n\n", out.reset().join("\n")); + eprintln!("{:#?}\n\n", failures); + Err(format!("{} of {} tooltip tests failed", failures.len(), tests.len()).into()) + } + } + + /// Note: This test is ignored as it doesn't work in the rust-lang/rust repo. + /// It is enabled on CI. + /// Run with `cargo test test_tooltip_std -- --ignored` + #[test] + #[ignore] + fn test_tooltip_std() -> Result<(), Box> { + use self::test::{LineOutput, Test, TooltipTestHarness}; + use std::env; + + let tests = vec![ Test::new("test_tooltip_std.rs", 18, 15), Test::new("test_tooltip_std.rs", 18, 27), Test::new("test_tooltip_std.rs", 19, 7), From d0aa8584661640b621bd7183b4c835ad17909e72 Mon Sep 17 00:00:00 2001 From: Alex Butler Date: Fri, 7 Dec 2018 18:08:10 +0000 Subject: [PATCH 2/6] TooltipTestHarness wait for concurrent jobs on drop --- src/actions/hover.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/actions/hover.rs b/src/actions/hover.rs index aa800beb682..2e5d63bc51b 100644 --- a/src/actions/hover.rs +++ b/src/actions/hover.rs @@ -1289,6 +1289,10 @@ pub mod test { impl Drop for TooltipTestHarness { fn drop(&mut self) { + if let Ok(mut jobs) = self.ctx.jobs.lock() { + jobs.wait_for_all(); + } + if fs::metadata(&self.working_dir).is_ok() { fs::remove_dir_all(&self.working_dir).expect("failed to tidy up"); } From eed6227f1ea402ed6113b541f020afd7c212e26d Mon Sep 17 00:00:00 2001 From: Alex Butler Date: Sat, 8 Dec 2018 01:17:08 +0000 Subject: [PATCH 3/6] Handle hover rustfmt panic * Enable logging in test_tooltip tests --- src/actions/hover.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/actions/hover.rs b/src/actions/hover.rs index 2e5d63bc51b..da46ae7507b 100644 --- a/src/actions/hover.rs +++ b/src/actions/hover.rs @@ -782,15 +782,19 @@ fn format_object(rustfmt: Rustfmt, fmt_config: &FmtConfig, the_type: String) -> format!("{}{{}}", trimmed) }; - let formatted = match rustfmt.format(object.clone(), config) { - Ok(lines) => match lines.rfind('{') { + let formatted = match std::panic::catch_unwind(|| rustfmt.format(object.clone(), config)) { + Ok(Ok(lines)) => match lines.rfind('{') { Some(pos) => lines[0..pos].into(), None => lines, }, - Err(e) => { + Ok(Err(e)) => { error!("format_object: error: {:?}, input: {:?}", e, object); trimmed.to_string() } + Err(_) => { + error!("format_object: rustfmt panicked on input: {:?}", object); + trimmed.to_string() + } }; // If it's a tuple, remove the trailing ';' and hide non-pub components @@ -2035,6 +2039,7 @@ pub mod test { #[test] fn test_tooltip() -> Result<(), Box> { + let _ = env_logger::try_init(); use self::test::{LineOutput, Test, TooltipTestHarness}; use std::env; @@ -2125,6 +2130,7 @@ pub mod test { #[test] #[ignore] fn test_tooltip_std() -> Result<(), Box> { + let _ = env_logger::try_init(); use self::test::{LineOutput, Test, TooltipTestHarness}; use std::env; From 6b6847112c72773152ac37f5f80277a140518eb3 Mon Sep 17 00:00:00 2001 From: Alex Butler Date: Mon, 10 Dec 2018 12:17:57 +0000 Subject: [PATCH 4/6] Move latter test_tooltip_mod_use_external to test_tooltip_std --- src/actions/hover.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/actions/hover.rs b/src/actions/hover.rs index da46ae7507b..c2a364dde95 100644 --- a/src/actions/hover.rs +++ b/src/actions/hover.rs @@ -2095,8 +2095,6 @@ pub mod test { Test::new("test_tooltip_mod_use_external.rs", 11, 7), Test::new("test_tooltip_mod_use_external.rs", 12, 7), Test::new("test_tooltip_mod_use_external.rs", 12, 12), - Test::new("test_tooltip_mod_use_external.rs", 14, 12), - Test::new("test_tooltip_mod_use_external.rs", 15, 12), ]; let cwd = env::current_dir()?; @@ -2135,6 +2133,10 @@ pub mod test { use std::env; let tests = vec![ + // these test std stuff + Test::new("test_tooltip_mod_use_external.rs", 14, 12), + Test::new("test_tooltip_mod_use_external.rs", 15, 12), + Test::new("test_tooltip_std.rs", 18, 15), Test::new("test_tooltip_std.rs", 18, 27), Test::new("test_tooltip_std.rs", 19, 7), From dede5d58c16016a565e5f8d8a2851c2ce9cfaac6 Mon Sep 17 00:00:00 2001 From: Alex Butler Date: Mon, 10 Dec 2018 12:39:40 +0000 Subject: [PATCH 5/6] Output expected/actual string diffs for tooltip test failures --- Cargo.lock | 7 +++++++ Cargo.toml | 3 +++ src/actions/hover.rs | 24 +++++++++++++++++++++--- 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8403ac15b95..19618a80159 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -390,6 +390,11 @@ name = "diff" version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "difference" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" + [[package]] name = "either" version = "1.5.0" @@ -1136,6 +1141,7 @@ dependencies = [ "cargo_metadata 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)", "clippy_lints 0.0.212 (git+https://github.com/rust-lang/rust-clippy?rev=a3c77f6ad1c1c185e561e9cd7fdec7db569169d1)", "crossbeam-channel 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)", + "difference 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "env_logger 0.5.13 (registry+https://github.com/rust-lang/crates.io-index)", "failure 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)", "itertools 0.7.9 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1818,6 +1824,7 @@ dependencies = [ "checksum derive-new 0.5.6 (registry+https://github.com/rust-lang/crates.io-index)" = "6ca414e896ae072546f4d789f452daaecf60ddee4c9df5dc6d5936d769e3d87c" "checksum derive_more 0.13.0 (registry+https://github.com/rust-lang/crates.io-index)" = "3f57d78cf3bd45270dad4e70c21ec77a960b36c7a841ff9db76aaa775a8fb871" "checksum diff 0.1.11 (registry+https://github.com/rust-lang/crates.io-index)" = "3c2b69f912779fbb121ceb775d74d51e915af17aaebc38d28a592843a2dd0a3a" +"checksum difference 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "524cbf6897b527295dff137cec09ecf3a05f4fddffd7dfcd1585403449e74198" "checksum either 1.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "3be565ca5c557d7f59e7cfcf1844f9e3033650c929c6566f511e8005f205c1d0" "checksum ena 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)" = "f56c93cc076508c549d9bb747f79aa9b4eb098be7b8cad8830c3137ef52d1e00" "checksum env_logger 0.5.13 (registry+https://github.com/rust-lang/crates.io-index)" = "15b0a4d2e39f8420210be8b27eeda28029729e2fd4291019455016c348240c38" diff --git a/Cargo.toml b/Cargo.toml index f603dca53b5..81344c9db99 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,6 +49,9 @@ toml = "0.4" # for more information. rustc-workspace-hack = "1.0.0" +[dev-dependencies] +difference = "2" + [build-dependencies] rustc_tools_util = { git = "https://github.com/rust-lang/rust-clippy", rev = "a3c77f6ad1c1c185e561e9cd7fdec7db569169d1" } diff --git a/src/actions/hover.rs b/src/actions/hover.rs index c2a364dde95..04b5f9b5661 100644 --- a/src/actions/hover.rs +++ b/src/actions/hover.rs @@ -991,6 +991,7 @@ pub mod test { use std::path::PathBuf; use std::process; use std::sync::{Arc, Mutex}; + use std::fmt; #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)] pub struct Test { @@ -1106,7 +1107,7 @@ pub mod test { } } - #[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] + #[derive(PartialEq, Eq)] pub struct TestFailure { /// The test case, indicating file, line, and column pub test: Test, @@ -1123,6 +1124,23 @@ pub mod test { pub actual_data: Result, String>, ()>, } + impl fmt::Debug for TestFailure { + fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt.debug_struct("TestFailure") + .field("test", &self.test) + .field("expect_file", &self.expect_file) + .field("actual_file", &self.actual_file) + .field("expect_data", &self.expect_data) + .field("actual_data", &self.actual_data) + .finish()?; + + let expected = format!("{:#?}", self.expect_data); + let actual = format!("{:#?}", self.actual_data); + write!(fmt, "-diff: {}", difference::Changeset::new(&expected, &actual, "")) + } + } + + #[derive(Clone, Default)] pub struct LineOutput { req_id: Arc>, @@ -2117,7 +2135,7 @@ pub mod test { Ok(()) } else { eprintln!("{}\n\n", out.reset().join("\n")); - eprintln!("{:#?}\n\n", failures); + eprintln!("Failures (\x1b[91mexpected\x1b[92mactual\x1b[0m): {:#?}\n\n", failures); Err(format!("{} of {} tooltip tests failed", failures.len(), tests.len()).into()) } } @@ -2172,7 +2190,7 @@ pub mod test { Ok(()) } else { eprintln!("{}\n\n", out.reset().join("\n")); - eprintln!("{:#?}\n\n", failures); + eprintln!("Failures (\x1b[91mexpected\x1b[92mactual\x1b[0m): {:#?}\n\n", failures); Err(format!("{} of {} tooltip tests failed", failures.len(), tests.len()).into()) } } From 22ade3d4f7873ecad719d323457072de77fca3fa Mon Sep 17 00:00:00 2001 From: Alex Butler Date: Thu, 13 Dec 2018 15:27:25 +0000 Subject: [PATCH 6/6] Remove catch_unwind let rustfmt.format handle this properly --- src/actions/hover.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/actions/hover.rs b/src/actions/hover.rs index 04b5f9b5661..31c172a8f3f 100644 --- a/src/actions/hover.rs +++ b/src/actions/hover.rs @@ -782,19 +782,15 @@ fn format_object(rustfmt: Rustfmt, fmt_config: &FmtConfig, the_type: String) -> format!("{}{{}}", trimmed) }; - let formatted = match std::panic::catch_unwind(|| rustfmt.format(object.clone(), config)) { - Ok(Ok(lines)) => match lines.rfind('{') { + let formatted = match rustfmt.format(object.clone(), config) { + Ok(lines) => match lines.rfind('{') { Some(pos) => lines[0..pos].into(), None => lines, }, - Ok(Err(e)) => { + Err(e) => { error!("format_object: error: {:?}, input: {:?}", e, object); trimmed.to_string() } - Err(_) => { - error!("format_object: rustfmt panicked on input: {:?}", object); - trimmed.to_string() - } }; // If it's a tuple, remove the trailing ';' and hide non-pub components