Merge #592

592: xtask: add backward compability test r=Urhengulas a=japaric

**UPDATE**: I have left the defmt-version as it is since we have not made a defmt 0.3.0 release. I have also tweaked the defmt-test to not exit with non-zero exit code and remove some test-specific logic from xtask. See individual commits messages for details.

---

implements #506 (comment)
closes #506

I tested this against c7b20d5 (I called that `defmt-version-3` locally) and found that a bitflags change broke the wire format. Not a big deal but I guess we should bump the version to 4? cc `@jonas-schievink` 

``` console
bitflags (dev)
Error: malformed bitflags value string 'Flags::0::FLAG_0'
```

the other thing that I found out is that some of the snapshot test exit with non-zero code:

``` console
defmt-test (dev)
INFO (1/7) running `change_init_struct`...
INFO (2/7) running `test_for_changed_init_struct`...
INFO (3/7) running `assert_true`...
INFO (4/7) running `assert_imported_max`...
INFO (5/7) running `result`...
INFO (6/7) running `should_error`...
INFO (7/7) running `fail`...
ERROR panicked at '`#[should_error]` test failed with outcome: Ok(this should have returned `Err`)'

Timer with period zero, disabling
```

`cargo xtask test-snapshot` is not checking the exit code; it only looks at stdout so that non-zero exit code is not a problem for those tests. in the backcompat test we want to look for decoding errors, not at stdout, so I was using the exit code of `cargo run` but this `defmt-test` is problematic with that check.

Co-authored-by: Jorge Aparicio <jorge.aparicio@ferrous-systems.com>

.github/workflows/ci.yml

@@ -114,6 +114,23 @@ jobs:
       - name: Run QEMU snapshot tests
         run: cargo xtask test-snapshot
 
+  backcompat:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+        with:
+          fetch-depth: 0
+      - uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: stable
+          override: true
+          target: ${{ env.QEMU_TARGET }}
+      - name: Install dependencies
+        run: sudo apt-get update && sudo apt-get install qemu qemu-system-arm
+      - name: Run backward compatibility test
+        run: cargo xtask test-backcompat
+
   # Refs: https://github.com/rust-lang/crater/blob/9ab6f9697c901c4a44025cf0a39b73ad5b37d198/.github/workflows/bors.yml#L125-L149
   # bors.tech integration
   ci-success:
@@ -125,6 +142,7 @@ jobs:
       - lint
       - mdbook
       - qemu-snapshot
+      - backcompat
     runs-on: ubuntu-20.04
     steps:
       - name: CI succeeded

firmware/qemu/src/bin/defmt-test.rs

@@ -1,11 +1,15 @@
 #![no_std]
 #![no_main]
 
+use core::sync::atomic::{AtomicBool, Ordering};
+
 use defmt_semihosting as _; // global logger
 
+static MAY_PANIC: AtomicBool = AtomicBool::new(false);
+
 #[defmt_test::tests]
 mod tests {
-    use core::u8::MAX;
+    use core::{sync::atomic::Ordering, u8::MAX};
     use defmt::{assert, assert_eq};
 
     struct InitStruct {
@@ -19,12 +23,9 @@ mod tests {
         elem2: f32,
     }
 
-
     #[init]
     fn init() -> InitStruct {
-        InitStruct {
-            test: 8,
-        }
+        InitStruct { test: 8 }
     }
 
     #[test]
@@ -71,6 +72,9 @@ mod tests {
     #[test]
     #[should_error]
     fn fail() -> Result<&'static str, ()> {
+        // this test is expected to fail (= panic)
+        super::MAY_PANIC.store(true, Ordering::Relaxed);
+
         Ok("this should have returned `Err`")
     }
 }
@@ -82,6 +86,11 @@ fn panic(_: &core::panic::PanicInfo) -> ! {
     use cortex_m_semihosting::debug;
 
     loop {
-        debug::exit(debug::EXIT_FAILURE)
+        let exit_code = if MAY_PANIC.load(Ordering::Relaxed) {
+            debug::EXIT_SUCCESS
+        } else {
+            debug::EXIT_FAILURE
+        };
+        debug::exit(exit_code)
     }
 }

xtask/Cargo.toml

@@ -14,3 +14,4 @@ colored = "2.0"
 once_cell = "1.7"
 similar = "1.3"
 structopt = "0.3"
+tempfile = "3.2.0"

xtask/src/backcompat.rs

@@ -0,0 +1,171 @@
+use std::{
+    borrow::Cow,
+    path::{Path, PathBuf},
+    process::Command,
+};
+
+use anyhow::anyhow;
+use colored::Colorize as _;
+use tempfile::TempDir;
+
+use crate::{utils, ALL_ERRORS, ALL_SNAPSHOT_TESTS, SNAPSHOT_TESTS_DIRECTORY};
+
+// PR #564
+const REVISION_UNDER_TEST: &str = "45beb423a5c2b4e6c645ea98b293513a6feadf6d";
+
+// the target name is in `firmware/qemu/.cargo/config.toml` but it'd be hard to extract it from that file
+const RUNNER_ENV_VAR: &str = "CARGO_TARGET_THUMBV7M_NONE_EABI_RUNNER";
+
+pub fn test() {
+    println!("🧪 backcompat");
+
+    println!("building old qemu-run.. (git revision: {})", REVISION_UNDER_TEST);
+    let qemu_run = match QemuRun::build() {
+        Ok(qemu_run) => qemu_run,
+        Err(e) => {
+            // only print build errors so the user can fix those manually if needed
+            eprintln!("error building old qemu-run: {}", e);
+            ALL_ERRORS
+                .lock()
+                .unwrap()
+                .push("backcompat (building qemu-run)".to_string());
+            return;
+        }
+    };
+
+    for release_mode in [false, true] {
+        for snapshot_test in ALL_SNAPSHOT_TESTS {
+            super::do_test(
+                || qemu_run.run_snapshot(snapshot_test, release_mode),
+                "backcompat",
+            );
+        }
+    }
+}
+
+struct QemuRun {
+    executable_path: PathBuf,
+    _tempdir: TempDir,
+}
+
+impl QemuRun {
+    fn build() -> anyhow::Result<Self> {
+        let tempdir = tempfile::tempdir()?;
+
+        let tempdir_path = tempdir.path();
+        clone_repo(tempdir_path)?;
+        let executable_path = build_qemu_run(tempdir_path)?;
+
+        Ok(Self {
+            executable_path,
+            _tempdir: tempdir,
+        })
+    }
+
+    fn run_snapshot(&self, name: &str, release_mode: bool) -> anyhow::Result<()> {
+        let formatted_test_name = utils::formatted_test_name(name, release_mode);
+        println!("{}", formatted_test_name.bold());
+
+        let args = if release_mode {
+            ["-q", "rrb", name]
+        } else {
+            ["-q", "rb", name]
+        };
+
+        run_silently(
+            Command::new("cargo")
+                .args(args)
+                .current_dir(SNAPSHOT_TESTS_DIRECTORY)
+                .env(RUNNER_ENV_VAR, self.path()),
+            || anyhow!("{}", formatted_test_name),
+        )?;
+
+        Ok(())
+    }
+
+    fn path(&self) -> &Path {
+        &self.executable_path
+    }
+}
+
+fn clone_repo(tempdir: &Path) -> anyhow::Result<()> {
+    let repo_path = Path::new(env!("CARGO_MANIFEST_DIR")).parent().unwrap();
+    run_silently(
+        Command::new("git")
+            .arg("clone")
+            .arg(repo_path)
+            .arg(".")
+            .current_dir(tempdir),
+        || anyhow!("`git clone` failed"),
+    )?;
+
+    run_silently(
+        Command::new("git")
+            .args(&["reset", "--hard", REVISION_UNDER_TEST])
+            .current_dir(tempdir),
+        || anyhow!("`git reset` failed"),
+    )?;
+
+    Ok(())
+}
+
+fn build_qemu_run(tempdir: &Path) -> anyhow::Result<PathBuf> {
+    run_silently(
+        Command::new("cargo")
+            .args(&["build", "-p", "qemu-run"])
+            .current_dir(tempdir),
+        || anyhow!("`cargo build` failed"),
+    )?;
+
+    let mut executable_path = tempdir.to_owned();
+    executable_path.push("target");
+    executable_path.push("debug");
+    executable_path.push("qemu-run");
+
+    assert!(executable_path.exists(), "`qemu-run` executable not found");
+
+    Ok(executable_path)
+}
+
+fn run_silently(command: &mut Command, err: impl FnOnce() -> anyhow::Error) -> anyhow::Result<()> {
+    let output = command.output()?;
+
+    if !output.status.success() {
+        let formatted_command = format!("{:?}", command);
+
+        if !output.stdout.is_empty() {
+            println!(
+                "stdout:\n{}",
+                std::str::from_utf8(&output.stdout).map_err(|e| anyhow!(
+                    "`{}` output is not UTF-8: {}",
+                    formatted_command,
+                    e
+                ))?
+            );
+        }
+
+        if !output.stderr.is_empty() {
+            println!(
+                "stderr:\n{}",
+                std::str::from_utf8(&output.stderr).map_err(|e| anyhow!(
+                    "`{}` output is not UTF-8: {}",
+                    formatted_command,
+                    e
+                ))?
+            );
+        }
+
+        println!(
+            "exit-code: {}",
+            output
+                .status
+                .code()
+                .map(|code| code.to_string().into())
+                .unwrap_or(Cow::Borrowed("non-zero"))
+        );
+
+        return Err(err());
+    }
+
+    Ok(())
+}

xtask/src/main.rs

@@ -6,6 +6,7 @@ use once_cell::sync::Lazy;
 use similar::{ChangeTag, TextDiff};
 use structopt::StructOpt;
 
+mod backcompat;
 mod targets;
 mod utils;
 
@@ -15,6 +16,7 @@ use crate::utils::{
 
 static ALL_ERRORS: Lazy<Mutex<Vec<String>>> = Lazy::new(|| Mutex::new(vec![]));
 
+const SNAPSHOT_TESTS_DIRECTORY: &str = "firmware/qemu";
 const ALL_SNAPSHOT_TESTS: [&str; 12] = [
     "log",
     "bitflags",
@@ -70,6 +72,7 @@ struct Options {
 #[allow(clippy::enum_variant_names)]
 enum TestCommand {
     TestAll,
+    TestBackcompat,
     TestBook,
     TestCross,
     TestHost,
@@ -91,6 +94,7 @@ fn main() -> anyhow::Result<()> {
 
     match opt.cmd {
         TestCommand::TestBook => test_book(),
+        TestCommand::TestBackcompat => backcompat::test(),
         TestCommand::TestHost => test_host(opt.deny_warnings),
         TestCommand::TestLint => test_lint(),
 
@@ -106,6 +110,7 @@ fn main() -> anyhow::Result<()> {
                     test_host(opt.deny_warnings);
                     test_cross();
                     test_snapshot(false, None);
+                    backcompat::test();
                     test_book();
                     test_lint();
                 }
@@ -333,7 +338,7 @@ fn test_single_snapshot(
     release_mode: bool,
     overwrite: bool,
 ) -> anyhow::Result<()> {
-    let display_name = format!("{} ({})", name, if release_mode { "release" } else { "dev" });
+    let display_name = utils::formatted_test_name(name, release_mode);
     println!("{}", display_name.bold());
 
     let mut args = if release_mode {
@@ -346,9 +351,12 @@ fn test_single_snapshot(
         args.extend_from_slice(&["--features", features]);
     }
 
-    const CWD: &str = "firmware/qemu";
-    let actual = run_capturing_stdout(Command::new("cargo").args(&args).current_dir(CWD))
-        .with_context(|| display_name.clone())?;
+    let actual = run_capturing_stdout(
+        Command::new("cargo")
+            .args(&args)
+            .current_dir(SNAPSHOT_TESTS_DIRECTORY),
+    )
+    .with_context(|| display_name.clone())?;
 
     if overwrite {
         overwrite_expected_output(name, release_mode, actual.as_bytes())?;

xtask/src/utils.rs

@@ -41,15 +41,6 @@ pub fn run_capturing_stdout(cmd: &mut Command) -> anyhow::Result<String> {
     match output.status.success() {
         true => Ok(str::from_utf8(&output.stdout)?.to_string()),
         false => {
-            // #[should_error]-tests return non-zero code
-            if output.status.code() == Some(1) {
-                let stdout = str::from_utf8(&output.stdout)?;
-                if stdout.contains(
-                    "`#[should_error]` test failed with outcome: Ok(this should have returned `Err`)",
-                ) {
-                    return Ok(stdout.to_string());
-                }
-            }
             eprintln!("{}", str::from_utf8(&output.stderr)?.dimmed());
             Err(anyhow!(""))
         }
@@ -92,3 +83,7 @@ pub fn rustc_is_nightly() -> bool {
     let out = run_capturing_stdout(Command::new("rustc").args(&["-V"])).unwrap();
     out.contains("nightly")
 }
+
+pub fn formatted_test_name(name: &str, release_mode: bool) -> String {
+    format!("{} ({})", name, if release_mode { "release" } else { "dev" })
+}