From a50337a118c3fe9e0f32aa5cf4209b856d1f7bfc Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Sat, 2 May 2015 16:50:33 +0200 Subject: [PATCH 1/6] first step to a non-panicking idem_check function --- tests/idem.rs | 75 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 20 deletions(-) diff --git a/tests/idem.rs b/tests/idem.rs index a82d326087eb..72f3277f5946 100644 --- a/tests/idem.rs +++ b/tests/idem.rs @@ -8,12 +8,13 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +#![feature(std_misc)] + extern crate rustfmt; use std::collections::HashMap; use std::fs; use std::io::Read; -use std::sync::atomic; use rustfmt::*; // For now, the only supported regression tests are idempotent tests - the input and @@ -22,52 +23,86 @@ use rustfmt::*; #[test] fn idempotent_tests() { println!("Idempotent tests:"); - FAILURES.store(0, atomic::Ordering::Relaxed); // Get all files in the tests/idem directory let files = fs::read_dir("tests/idem").unwrap(); + let files2 = fs::read_dir("tests").unwrap(); + let files3 = fs::read_dir("src/bin").unwrap(); // For each file, run rustfmt and collect the output + let mut count = 0; - for entry in files { + let mut fails = 0; + for entry in files.chain(files2).chain(files3) { let path = entry.unwrap().path(); let file_name = path.to_str().unwrap(); + if !file_name.ends_with(".rs") { + continue; + } println!("Testing '{}'...", file_name); - run(vec!["rustfmt".to_owned(), file_name.to_owned()], WriteMode::Return(HANDLE_RESULT)); + match idempotent_check(vec!["rustfmt".to_owned(), file_name.to_owned()]) { + Ok(()) => {}, + Err(m) => { + print_mismatches(m); + fails += 1; + }, + } count += 1; } - // And also dogfood ourselves! + // And also dogfood rustfmt! println!("Testing 'src/lib.rs'..."); - run(vec!["rustfmt".to_string(), "src/lib.rs".to_string()], - WriteMode::Return(HANDLE_RESULT)); + match idempotent_check(vec!["rustfmt".to_owned(), "src/lib.rs".to_owned()]) { + Ok(()) => {}, + Err(m) => { + print_mismatches(m); + fails += 1; + }, + } count += 1; // Display results - let fails = FAILURES.load(atomic::Ordering::Relaxed); println!("Ran {} idempotent tests; {} failures.", count, fails); assert!(fails == 0, "{} idempotent tests failed", fails); } -// 'global' used by sys_tests and handle_result. -static FAILURES: atomic::AtomicUsize = atomic::ATOMIC_USIZE_INIT; +// Compare output to input. +fn print_mismatches(result: HashMap) { + for (file_name, fmt_text) in result { + println!("Mismatch in {}.", file_name); + println!("{}", fmt_text); + } +} + // Ick, just needed to get a &'static to handle_result. static HANDLE_RESULT: &'static Fn(HashMap) = &handle_result; +pub fn idempotent_check(args: Vec) -> Result<(), HashMap> { + use std::thread; + use std::fs; + use std::io::Read; + thread::spawn(move || { + run(args, WriteMode::Return(HANDLE_RESULT)); + }).join().map_err(|mut any| + any.downcast_mut::>() + .unwrap() // i know it is a hashmap + .drain() // i only get a reference :( + .collect() // so i need to turn it into an iter and then back + ) +} + // Compare output to input. fn handle_result(result: HashMap) { - let mut fails = 0; + let mut failures = HashMap::new(); - for file_name in result.keys() { - let mut f = fs::File::open(file_name).unwrap(); + for (file_name, fmt_text) in result { + let mut f = fs::File::open(&file_name).unwrap(); let mut text = String::new(); + // TODO: speedup by running through bytes iterator f.read_to_string(&mut text).unwrap(); - if result[file_name] != text { - fails += 1; - println!("Mismatch in {}.", file_name); - println!("{}", result[file_name]); + if fmt_text != text { + failures.insert(file_name, fmt_text); } } - - if fails > 0 { - FAILURES.fetch_add(1, atomic::Ordering::Relaxed); + if !failures.is_empty() { + panic!(failures); } } \ No newline at end of file From 968344fba183e30d1d19154b5a6126233743155a Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Sat, 2 May 2015 16:56:46 +0200 Subject: [PATCH 2/6] pass single filename instead of full argument list to idem_check --- tests/idem.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/idem.rs b/tests/idem.rs index 72f3277f5946..401e5fffcc6a 100644 --- a/tests/idem.rs +++ b/tests/idem.rs @@ -39,7 +39,7 @@ fn idempotent_tests() { continue; } println!("Testing '{}'...", file_name); - match idempotent_check(vec!["rustfmt".to_owned(), file_name.to_owned()]) { + match idempotent_check(file_name.to_owned()) { Ok(()) => {}, Err(m) => { print_mismatches(m); @@ -50,7 +50,7 @@ fn idempotent_tests() { } // And also dogfood rustfmt! println!("Testing 'src/lib.rs'..."); - match idempotent_check(vec!["rustfmt".to_owned(), "src/lib.rs".to_owned()]) { + match idempotent_check("src/lib.rs".to_owned()) { Ok(()) => {}, Err(m) => { print_mismatches(m); @@ -75,10 +75,11 @@ fn print_mismatches(result: HashMap) { // Ick, just needed to get a &'static to handle_result. static HANDLE_RESULT: &'static Fn(HashMap) = &handle_result; -pub fn idempotent_check(args: Vec) -> Result<(), HashMap> { +pub fn idempotent_check(filename: String) -> Result<(), HashMap> { use std::thread; use std::fs; use std::io::Read; + let args = vec!["rustfmt".to_owned(), filename]; thread::spawn(move || { run(args, WriteMode::Return(HANDLE_RESULT)); }).join().map_err(|mut any| From b546dfcf8305d4260ffad796f9544c89e75952bf Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Sat, 2 May 2015 17:09:33 +0200 Subject: [PATCH 3/6] forgot newline at eof rustfmt found it --- tests/idem.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/idem.rs b/tests/idem.rs index 401e5fffcc6a..d897cc34151a 100644 --- a/tests/idem.rs +++ b/tests/idem.rs @@ -106,4 +106,4 @@ fn handle_result(result: HashMap) { if !failures.is_empty() { panic!(failures); } -} \ No newline at end of file +} From 07637aed418e9839f665eda24fe823712713790f Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 4 May 2015 10:09:41 +0200 Subject: [PATCH 4/6] rustfmt dislikes tabs and some newlines --- tests/idem.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/idem.rs b/tests/idem.rs index d897cc34151a..9f6b36020614 100644 --- a/tests/idem.rs +++ b/tests/idem.rs @@ -9,7 +9,6 @@ // except according to those terms. #![feature(std_misc)] - extern crate rustfmt; use std::collections::HashMap; @@ -79,7 +78,7 @@ pub fn idempotent_check(filename: String) -> Result<(), HashMap> use std::thread; use std::fs; use std::io::Read; - let args = vec!["rustfmt".to_owned(), filename]; + let args = vec!["rustfmt".to_owned(), filename]; thread::spawn(move || { run(args, WriteMode::Return(HANDLE_RESULT)); }).join().map_err(|mut any| From a14dcdf0c25ed7e1714f80cee3a24be796db4deb Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 4 May 2015 11:00:14 +0200 Subject: [PATCH 5/6] addressed comments --- tests/idem.rs | 46 +++++++++++++++++----------------------------- 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/tests/idem.rs b/tests/idem.rs index 9f6b36020614..64700be69f9a 100644 --- a/tests/idem.rs +++ b/tests/idem.rs @@ -8,12 +8,12 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -#![feature(std_misc)] extern crate rustfmt; use std::collections::HashMap; use std::fs; use std::io::Read; +use std::thread; use rustfmt::*; // For now, the only supported regression tests are idempotent tests - the input and @@ -25,20 +25,19 @@ fn idempotent_tests() { // Get all files in the tests/idem directory let files = fs::read_dir("tests/idem").unwrap(); - let files2 = fs::read_dir("tests").unwrap(); - let files3 = fs::read_dir("src/bin").unwrap(); - // For each file, run rustfmt and collect the output + let files = files.chain(fs::read_dir("tests").unwrap()); + let files = files.chain(fs::read_dir("src/bin").unwrap()); + // turn a DirEntry into a String that represents the relative path to the file + let files = files.map(|e| e.unwrap().path().to_str().unwrap().to_owned()); + // hack because there's no `IntoIterator` impl for `[T; N]` + let files = files.chain(Some("src/lib.rs".to_owned()).into_iter()); + // For each file, run rustfmt and collect the output let mut count = 0; let mut fails = 0; - for entry in files.chain(files2).chain(files3) { - let path = entry.unwrap().path(); - let file_name = path.to_str().unwrap(); - if !file_name.ends_with(".rs") { - continue; - } + for file_name in files.filter(|f| f.ends_with(".rs")) { println!("Testing '{}'...", file_name); - match idempotent_check(file_name.to_owned()) { + match idempotent_check(file_name) { Ok(()) => {}, Err(m) => { print_mismatches(m); @@ -47,16 +46,6 @@ fn idempotent_tests() { } count += 1; } - // And also dogfood rustfmt! - println!("Testing 'src/lib.rs'..."); - match idempotent_check("src/lib.rs".to_owned()) { - Ok(()) => {}, - Err(m) => { - print_mismatches(m); - fails += 1; - }, - } - count += 1; // Display results println!("Ran {} idempotent tests; {} failures.", count, fails); @@ -75,17 +64,16 @@ fn print_mismatches(result: HashMap) { static HANDLE_RESULT: &'static Fn(HashMap) = &handle_result; pub fn idempotent_check(filename: String) -> Result<(), HashMap> { - use std::thread; - use std::fs; - use std::io::Read; let args = vec!["rustfmt".to_owned(), filename]; + // this thread is not used for concurrency, but rather to workaround the issue that the passed + // function handle needs to have static lifetime. Instead of using a global RefCell, we use + // panic to return a result in case of failure. This has the advantage of smoothing the road to + // multithreaded rustfmt thread::spawn(move || { run(args, WriteMode::Return(HANDLE_RESULT)); - }).join().map_err(|mut any| - any.downcast_mut::>() - .unwrap() // i know it is a hashmap - .drain() // i only get a reference :( - .collect() // so i need to turn it into an iter and then back + }).join().map_err(|any| + // i know it is a hashmap + *any.downcast().unwrap() ) } From ee338d934e0ab1c13cfc14a90219036da3a67ed1 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 5 May 2015 16:56:29 +0200 Subject: [PATCH 6/6] don't create a thread just for panic-isolation --- tests/idem.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/idem.rs b/tests/idem.rs index 64700be69f9a..d19bcc6fa55c 100644 --- a/tests/idem.rs +++ b/tests/idem.rs @@ -8,6 +8,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +#![feature(catch_panic)] + extern crate rustfmt; use std::collections::HashMap; @@ -69,9 +71,9 @@ pub fn idempotent_check(filename: String) -> Result<(), HashMap> // function handle needs to have static lifetime. Instead of using a global RefCell, we use // panic to return a result in case of failure. This has the advantage of smoothing the road to // multithreaded rustfmt - thread::spawn(move || { + thread::catch_panic(move || { run(args, WriteMode::Return(HANDLE_RESULT)); - }).join().map_err(|any| + }).map_err(|any| // i know it is a hashmap *any.downcast().unwrap() )