Auto merge of #140 - cjpearce:fix/test-race-condition, r=komaeda

Fix intermittent test failure caused by race condition

First public pull request 😬

There's an intermittent integration test failure when you use multiple test threads (at least for me on a mac). I narrowed it down to two tests each spawning a process using `Command` which then try to compile the same file at the same time. If the timing doesn't work out, they both try to compile, and then one process runs `clean` before the other can run the executable - causing a panic.

![Screenshot 2019-04-07 at 19 54 55](https://user-images.githubusercontent.com/3453268/55688324-20520980-596f-11e9-8474-5215d61a4387.png)

You can prevent it from happening by running with a single thread (`cargo test -- --test-threads=1`), because the `Command` blocks. That's not a particularly good solution though because it's not something you can configure in `Cargo.toml`.

I considered making the affected tests just run serially, but it occurred to me that this could also happen if someone accidentally runs rustlings in watch mode in two terminals without realising it. I wound't consider this that unlikely given it's a tool for learning.

I fixed it by ensuring that the executables made from separate processes don't conflict by appending a process id to the output executable name. I also extracted the commands into a single file next to `clean` so that we don't have to repeat the generated file name everywhere and risk missing something.

.travis.yml

@@ -3,7 +3,7 @@ rust:
   - stable
   - beta
   - nightly
-script: cargo test --verbose -- --test-threads=1
+script: cargo test --verbose
 matrix:
   allow_failures:
     - rust: nightly

src/run.rs

@@ -1,9 +1,8 @@
-use crate::util::clean;
+use crate::util;
 use crate::verify::test;
 use console::{style, Emoji};
 use indicatif::ProgressBar;
 use std::fs;
-use std::process::Command;
 use toml::Value;
 
 pub fn run(matches: clap::ArgMatches) -> Result<(), ()> {
@@ -33,20 +32,18 @@ pub fn compile_and_run(filename: &str) -> Result<(), ()> {
     let progress_bar = ProgressBar::new_spinner();
     progress_bar.set_message(format!("Compiling {}...", filename).as_str());
     progress_bar.enable_steady_tick(100);
-    let compilecmd = Command::new("rustc")
-        .args(&[filename, "-o", "temp", "--color", "always"])
-        .output()
-        .expect("fail");
+
+    let compilecmd = util::compile_cmd(filename);
     progress_bar.set_message(format!("Running {}...", filename).as_str());
     if compilecmd.status.success() {
-        let runcmd = Command::new("./temp").output().expect("fail");
+        let runcmd = util::run_cmd();
         progress_bar.finish_and_clear();
 
         if runcmd.status.success() {
             println!("{}", String::from_utf8_lossy(&runcmd.stdout));
             let formatstr = format!("{} Successfully ran {}", Emoji("✅", "✓"), filename);
             println!("{}", style(formatstr).green());
-            clean();
+            util::clean();
             Ok(())
         } else {
             println!("{}", String::from_utf8_lossy(&runcmd.stdout));
@@ -54,7 +51,7 @@ pub fn compile_and_run(filename: &str) -> Result<(), ()> {
 
             let formatstr = format!("{} Ran {} with errors", Emoji("⚠️ ", "!"), filename);
             println!("{}", style(formatstr).red());
-            clean();
+            util::clean();
             Err(())
         }
     } else {
@@ -66,7 +63,7 @@ pub fn compile_and_run(filename: &str) -> Result<(), ()> {
         );
         println!("{}", style(formatstr).red());
         println!("{}", String::from_utf8_lossy(&compilecmd.stderr));
-        clean();
+        util::clean();
         Err(())
     }
 }

src/util.rs

@@ -1,12 +1,41 @@
 use std::fs::remove_file;
+use std::process::{self, Command, Output};
+
+const RUSTC_COLOR_ARGS: &[&str] = &["--color", "always"];
+
+fn temp_file() -> String {
+    format!("./temp_{}", process::id())
+}
+
+pub fn compile_test_cmd(filename: &str) -> Output {
+    Command::new("rustc")
+        .args(&["--test", filename, "-o", &temp_file()])
+        .args(RUSTC_COLOR_ARGS)
+        .output()
+        .expect("failed to compile exercise")
+}
+
+pub fn compile_cmd(filename: &str) -> Output {
+    Command::new("rustc")
+        .args(&[filename, "-o", &temp_file()])
+        .args(RUSTC_COLOR_ARGS)
+        .output()
+        .expect("failed to compile exercise")
+}
+
+pub fn run_cmd() -> Output {
+    Command::new(&temp_file())
+        .output()
+        .expect("failed to run exercise")
+}
 
 pub fn clean() {
-    let _ignored = remove_file("temp");
+    let _ignored = remove_file(&temp_file());
 }
 
 #[test]
 fn test_clean() {
-    std::fs::File::create("temp").unwrap();
+    std::fs::File::create(&temp_file()).unwrap();
     clean();
-    assert!(!std::path::Path::new("temp").exists());
+    assert!(!std::path::Path::new(&temp_file()).exists());
 }

src/verify.rs

@@ -1,8 +1,7 @@
-use crate::util::clean;
+use crate::util;
 use console::{style, Emoji};
 use indicatif::ProgressBar;
 use std::fs;
-use std::process::Command;
 use toml::Value;
 
 pub fn verify(start_at: Option<&str>) -> Result<(), ()> {
@@ -34,10 +33,7 @@ fn compile_only(filename: &str) -> Result<(), ()> {
     let progress_bar = ProgressBar::new_spinner();
     progress_bar.set_message(format!("Compiling {}...", filename).as_str());
     progress_bar.enable_steady_tick(100);
-    let compilecmd = Command::new("rustc")
-        .args(&[filename, "-o", "temp", "--color", "always"])
-        .output()
-        .expect("fail");
+    let compilecmd = util::compile_cmd(filename);
     progress_bar.finish_and_clear();
     if compilecmd.status.success() {
         let formatstr = format!(
@@ -46,7 +42,7 @@ fn compile_only(filename: &str) -> Result<(), ()> {
             filename
         );
         println!("{}", style(formatstr).green());
-        clean();
+        util::clean();
         Ok(())
     } else {
         let formatstr = format!(
@@ -56,7 +52,7 @@ fn compile_only(filename: &str) -> Result<(), ()> {
         );
         println!("{}", style(formatstr).red());
         println!("{}", String::from_utf8_lossy(&compilecmd.stderr));
-        clean();
+        util::clean();
         Err(())
     }
 }
@@ -65,19 +61,16 @@ pub fn test(filename: &str) -> Result<(), ()> {
     let progress_bar = ProgressBar::new_spinner();
     progress_bar.set_message(format!("Testing {}...", filename).as_str());
     progress_bar.enable_steady_tick(100);
-    let testcmd = Command::new("rustc")
-        .args(&["--test", filename, "-o", "temp", "--color", "always"])
-        .output()
-        .expect("fail");
+    let testcmd = util::compile_test_cmd(filename);
     if testcmd.status.success() {
         progress_bar.set_message(format!("Running {}...", filename).as_str());
-        let runcmd = Command::new("./temp").output().expect("fail");
+        let runcmd = util::run_cmd();
         progress_bar.finish_and_clear();
 
         if runcmd.status.success() {
             let formatstr = format!("{} Successfully tested {}!", Emoji("✅", "✓"), filename);
             println!("{}", style(formatstr).green());
-            clean();
+            util::clean();
             Ok(())
         } else {
             let formatstr = format!(
@@ -87,7 +80,7 @@ pub fn test(filename: &str) -> Result<(), ()> {
             );
             println!("{}", style(formatstr).red());
             println!("{}", String::from_utf8_lossy(&runcmd.stdout));
-            clean();
+            util::clean();
             Err(())
         }
     } else {
@@ -99,7 +92,7 @@ pub fn test(filename: &str) -> Result<(), ()> {
         );
         println!("{}", style(formatstr).red());
         println!("{}", String::from_utf8_lossy(&testcmd.stderr));
-        clean();
+        util::clean();
         Err(())
     }
 }