Merge #334

334: Simplify snapshot tests r=justahero a=Urhengulas

This PR simplifies the probe-run snapshot tests by using the `rstest` macro. ~~Additionally we update the dependencies used in the tests.~~

# How to test the tests

These tests are ignored by default, since they require the hardware (`nrf52840dk`) to be present. You can run them locally by connecting your computer to the nrf52 and executing `cargo test -- --ignored`.

# Reviewer notes

Please don't get intimidated by the number of files and lines changed. Most of the files are just renamed to work with `rstest` and the new name of the test file. The many deleted lines are because the stack overflow test got simplified in the past, but the output wasn't adapted yet. You should mainly focus on `tests/snapshot.rs`.

# Remarks

Note that the `panic_verbose` test will fail in most cases. It's because this test output contains timing related numbers, which will slightly change from run to run. This also happened before this PR, therefore isn't a regression, but should be fixed in the future.

~~Note that updating `serial_test` pulls in many `futures-*` libraries, which seems unnecessary since we are not using any async functions. I've asked if that can be avoided: palfrey/serial_test#73

---

Edit(1): Drop dependency update from this PR, in order to unblock it.
Edit(2): Add reviewer notes.
Edit(3): Add section on how to tests the tests and structure the PR description.

Co-authored-by: Urhengulas <johann.hemmann@code.berlin>
Co-authored-by: Johann Hemmann <johann.hemmann@code.berlin>

CHANGELOG.md

@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p
 
 ## [Unreleased]
 
+- [#334] Simplify snapshot tests
 - [#333] Clean up `enum Outcome`
 - [#331] Refactor stack painting
 - [#330] Fix `fn round_up`
@@ -19,6 +20,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p
 - [#314] Clarify documentation in README
 - [#293] Update snapshot tests to new TRACE output
 
+[#334]: https://github.com/knurling-rs/probe-run/pull/334
 [#333]: https://github.com/knurling-rs/probe-run/pull/333
 [#331]: https://github.com/knurling-rs/probe-run/pull/331
 [#330]: https://github.com/knurling-rs/probe-run/pull/330

tests/test.rs → tests/snapshot.rs

@@ -1,10 +1,12 @@
-use os_pipe::pipe;
-use serial_test::serial;
 use std::{
     io::Read,
     process::{Command, ExitStatus},
 };
 
+use os_pipe::pipe;
+use rstest::rstest;
+use serial_test::serial;
+
 struct RunResult {
     exit_status: ExitStatus,
     output: String,
@@ -60,13 +62,13 @@ fn run_and_terminate(args: &str, timeout_s: u64) -> RunResult {
 fn run_command(args: &str) -> (os_pipe::PipeReader, std::process::Child) {
     // add prefix to run this repository's version of `probe-run` and
     // remove user-dependent registry and rustc information from backtrace paths
-    let complete_command = format!("run -- {} --shorten-paths", args);
+    let cmd = format!("run -- --chip nRF52840_xxAA tests/test_elfs/{args} --shorten-paths");
 
     let (reader, writer) = pipe().unwrap();
     let writer_clone = writer.try_clone().unwrap();
 
     let handle = Command::new("cargo")
-        .args(complete_command.split(" "))
+        .args(cmd.split(" "))
         // capture stderr and stdout while preserving line order
         .stdout(writer)
         .stderr(writer_clone)
@@ -93,103 +95,30 @@ fn truncate_output(probe_run_output: String) -> String {
         .collect()
 }
 
-#[test]
-#[serial]
-// this test should not be run by default, as it requires the target hardware to be present
-#[ignore]
-fn successful_run_has_no_backtrace() {
-    let run_result = run("--chip nRF52840_xxAA tests/test_elfs/hello-rzcobs");
-
-    assert_eq!(true, run_result.exit_status.success());
-    insta::assert_snapshot!(run_result.output);
-}
-
-#[test]
-#[serial]
-// this test should not be run by default, as it requires the target hardware to be present
-#[ignore]
-fn raw_encoding() {
-    let run_result = run("--chip nRF52840_xxAA tests/test_elfs/hello-raw");
-
-    assert_eq!(true, run_result.exit_status.success());
-    insta::assert_snapshot!(run_result.output);
-}
-
-#[test]
-#[serial]
-// this test should not be run by default, as it requires the target hardware to be present
-#[ignore]
-fn successful_run_can_enforce_backtrace() {
-    let run_result = run("--chip nRF52840_xxAA tests/test_elfs/hello-rzcobs --backtrace=always");
-
-    assert_eq!(true, run_result.exit_status.success());
-    insta::assert_snapshot!(run_result.output);
-}
-
-#[test]
-#[serial]
-// this test should not be run by default, as it requires the target hardware to be present
-#[ignore]
-fn stack_overflow_is_reported_as_such() {
-    let run_result = run("--chip nRF52840_xxAA tests/test_elfs/overflow-rzcobs");
-
-    assert_eq!(false, run_result.exit_status.success());
-    insta::assert_snapshot!(run_result.output);
-}
-
-#[test]
-#[serial]
-// this test should not be run by default, as it requires the target hardware to be present
-#[ignore]
-fn panic_is_reported_as_such() {
-    let run_result = run("--chip nRF52840_xxAA tests/test_elfs/panic-rzcobs");
-
-    assert_eq!(false, run_result.exit_status.success());
-    insta::assert_snapshot!(run_result.output);
-}
-
-#[test]
+#[rstest]
+#[case::successful_run_has_no_backtrace("hello-rzcobs", true)]
+#[case::raw_encoding("hello-raw", true)]
+#[case::successful_run_can_enforce_backtrace("hello-rzcobs --backtrace=always", true)]
+#[case::stack_overflow_is_reported_as_such("overflow-rzcobs", false)]
+#[case::panic_is_reported_as_such("panic-rzcobs", false)]
+// FIXME: Filter out timing related number
+#[case::panic_verbose("panic-rzcobs --verbose", false)]
+#[case::unsuccessful_run_can_suppress_backtrace("panic-rzcobs --backtrace=never", false)]
+#[case::stack_overflow_can_suppress_backtrace("overflow-rzcobs --backtrace=never", false)]
 #[serial]
-// this test should not be run by default, as it requires the target hardware to be present
-#[ignore]
-fn panic_verbose() {
-    // record current verbose backtrace to catch deviations
-    let run_result = run("--chip nRF52840_xxAA tests/test_elfs/panic-rzcobs --verbose");
-
-    assert_eq!(false, run_result.exit_status.success());
+#[ignore = "requires the target hardware to be present"]
+fn snapshot_test(#[case] args: &str, #[case] success: bool) {
+    let run_result = run(args);
+    assert_eq!(success, run_result.exit_status.success());
     insta::assert_snapshot!(run_result.output);
 }
 
 #[test]
 #[serial]
-// this test should not be run by default, as it requires the target hardware to be present
-#[ignore]
-fn unsuccessful_run_can_suppress_backtrace() {
-    let run_result = run("--chip nRF52840_xxAA tests/test_elfs/panic-rzcobs --backtrace=never");
-
-    assert_eq!(false, run_result.exit_status.success());
-    insta::assert_snapshot!(run_result.output);
-}
-
-#[test]
-#[serial]
-// this test should not be run by default, as it requires the target hardware to be present
-#[ignore]
-fn stack_overflow_can_suppress_backtrace() {
-    let run_result = run("--chip nRF52840_xxAA tests/test_elfs/overflow-rzcobs --backtrace=never");
-
-    assert_eq!(false, run_result.exit_status.success());
-}
-
-#[test]
-#[serial]
-// this test should not be run by default, as it requires the target hardware to be present
-#[ignore]
+#[ignore = "requires the target hardware to be present"]
 #[cfg(target_family = "unix")]
 fn ctrl_c_by_user_is_reported_as_such() {
-    let run_result =
-        run_and_terminate("--chip nRF52840_xxAA tests/test_elfs/silent-loop-rzcobs", 5);
-
+    let run_result = run_and_terminate("silent-loop-rzcobs", 5);
     assert_eq!(false, run_result.exit_status.success());
     insta::assert_snapshot!(run_result.output);
 }

tests/snapshots/test__successful_run_has_no_backtrace.snap → tests/snapshots/snapshot__case_1_successful_run_has_no_backtrace.snap

@@ -1,5 +1,5 @@
 ---
-source: tests/test.rs
+source: tests/snapshot.rs
 expression: run_result.output
 
 ---

tests/snapshots/test__raw_encoding.snap → tests/snapshots/snapshot__case_2_raw_encoding.snap

@@ -1,5 +1,5 @@
 ---
-source: tests/test.rs
+source: tests/snapshot.rs
 expression: run_result.output
 
 ---

tests/snapshots/test__successful_run_can_enforce_backtrace.snap → tests/snapshots/snapshot__case_3_successful_run_can_enforce_backtrace.snap

@@ -1,5 +1,5 @@
 ---
-source: tests/test.rs
+source: tests/snapshot.rs
 expression: run_result.output
 
 ---

tests/snapshots/test__stack_overflow_is_reported_as_such.snap → tests/snapshots/snapshot__case_4_stack_overflow_is_reported_as_such.snap

@@ -1,5 +1,5 @@
 ---
-source: tests/test.rs
+source: tests/snapshot.rs
 expression: run_result.output
 
 ---

tests/snapshots/test__panic_is_reported_as_such.snap → tests/snapshots/snapshot__case_5_panic_is_reported_as_such.snap

@@ -1,5 +1,5 @@
 ---
-source: tests/test.rs
+source: tests/snapshot.rs
 expression: run_result.output
 
 ---

tests/snapshots/test__panic_verbose.snap → tests/snapshots/snapshot__case_6_panic_verbose.snap

@@ -1,31 +1,31 @@
 ---
-source: tests/test.rs
+source: tests/snapshot.rs
 expression: run_result.output
 
 ---
 (HOST) DEBUG vector table: VectorTable { initial_stack_pointer: 2003fbc8, hard_fault: 15b1 }
 (HOST) DEBUG RAM region: 0x20000000-0x2003FFFF
-(HOST) DEBUG section `.data` is in RAM at 0x2003FBC8 ..= 0x2003FBF7
-(HOST) DEBUG section `.bss` is in RAM at 0x2003FBF8 ..= 0x2003FBFF
-(HOST) DEBUG section `.uninit` is in RAM at 0x2003FC00 ..= 0x2003FFFF
-(HOST) DEBUG valid SP range: 0x20000000 ..= 0x2003FBC7
+(HOST) DEBUG section `.data` is in RAM at 0x2003FBC8..=0x2003FBF7
+(HOST) DEBUG section `.bss` is in RAM at 0x2003FBF8..=0x2003FBFF
+(HOST) DEBUG section `.uninit` is in RAM at 0x2003FC00..=0x2003FFFF
+(HOST) DEBUG valid SP range: 0x20000000..=0x2003FBC4
 (HOST) DEBUG found 1 probes
 (HOST) DEBUG opened probe
 (HOST) DEBUG started session
 (HOST) INFO  flashing program (2 pages / 8.00 KiB)
-(HOST) DEBUG Erased sector of size 4096 bytes in 149 ms
-(HOST) DEBUG Erased sector of size 4096 bytes in 113 ms
-(HOST) DEBUG Programmed page of size 4096 bytes in 96 ms
-(HOST) DEBUG Programmed page of size 4096 bytes in 80 ms
+(HOST) DEBUG Erased sector of size 4096 bytes in 144 ms
+(HOST) DEBUG Erased sector of size 4096 bytes in 109 ms
+(HOST) DEBUG Programmed page of size 4096 bytes in 79 ms
+(HOST) DEBUG Programmed page of size 4096 bytes in 71 ms
 (HOST) INFO  success!
-(HOST) DEBUG 261062 bytes of stack available (0x20000000 ..= 0x2003FBC7), using 1024 byte canary
-(HOST) TRACE setting up canary took 0.011s (91.54 KiB/s)
+(HOST) DEBUG 261060 bytes of stack available (0x20000000 ..= 0x2003FBC4), using 1024 byte canary
+(HOST) TRACE setting up canary took 0.020s (49.74 KiB/s)
 (HOST) DEBUG starting device
 (HOST) DEBUG Successfully attached RTT
 ────────────────────────────────────────────────────────────────────────────────
 ERROR panicked at 'explicit panic'
 ────────────────────────────────────────────────────────────────────────────────
-(HOST) TRACE reading canary took 0.010s (95.49 KiB/s)
+(HOST) TRACE reading canary took 0.010s (104.18 KiB/s)
 (HOST) DEBUG stack canary intact
 (HOST) TRACE 0x000015b0: found FDE for 0x000015b0 .. 0x000015c4 at offset 6432
 (HOST) TRACE uwt row for pc 0x000015b0: UnwindTableRow { start_address: 5552, end_address: 5572, saved_args_size: 0, cfa: RegisterAndOffset { register: Register(13), offset: 0 }, registers: RegisterRuleMap { rules: [] } }

tests/snapshots/test__unsuccessful_run_can_suppress_backtrace.snap → tests/snapshots/snapshot__case_7_unsuccessful_run_can_suppress_backtrace.snap

@@ -1,5 +1,5 @@
 ---
-source: tests/test.rs
+source: tests/snapshot.rs
 expression: run_result.output
 
 ---

tests/snapshots/snapshot__case_8_stack_overflow_can_suppress_backtrace.snap

@@ -0,0 +1,18 @@
+---
+source: tests/snapshot.rs
+expression: run_result.output
+
+---
+(HOST) INFO  flashing program (2 pages / 8.00 KiB)
+(HOST) INFO  success!
+────────────────────────────────────────────────────────────────────────────────
+INFO  ack(m=10, n=10, SP=20037b88)
+INFO  ack(m=10, n=9, SP=2002fb60)
+INFO  ack(m=10, n=8, SP=20027b38)
+INFO  ack(m=10, n=7, SP=2001fb10)
+INFO  ack(m=10, n=6, SP=20017ae8)
+INFO  ack(m=10, n=5, SP=2000fac0)
+INFO  ack(m=10, n=4, SP=20007a98)
+────────────────────────────────────────────────────────────────────────────────
+(HOST) ERROR the program has overflowed its stack
+

tests/snapshots/test__ctrl_c_by_user_is_reported_as_such.snap → tests/snapshots/snapshot__ctrl_c_by_user_is_reported_as_such.snap

@@ -1,5 +1,5 @@
 ---
-source: tests/test.rs
+source: tests/snapshot.rs
 expression: run_result.output
 
 ---