From 015dfb3b164921432f92820714babf445aea05e0 Mon Sep 17 00:00:00 2001
From: Marco Neumann <marco@crepererum.net>
Date: Tue, 21 Sep 2021 12:00:34 +0200
Subject: [PATCH] test: do not (ab)use the panic hook for replay tests

The old construct uses a single assert-statement for both:

- "bubble-up" scenario, were a panic should fire
- a check, were a panic should not fire

That makes it easy to add new tests. However we need two rather
questionable things to make that work:

- catch panic: to convert an assertion to a check
- a custom panic hook: to make tests not overly verbose (aka caught
  panics should not show up on stdout)

Esp. the custom panic hook doesn't work too well w/ multi-threaded tests
since it might swallow error messages from unrelated tests and makes
debugging of CI failures hard.

So instead of using assertions for checks, we now implement a proper
assertion and a check for each test. That's a bit more code per check
but easier probably more stable.
---
 server/src/db/replay.rs | 91 +++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 58 deletions(-)

diff --git a/server/src/db/replay.rs b/server/src/db/replay.rs
index 7fd2aa73bf..500cd9a058 100644
--- a/server/src/db/replay.rs
+++ b/server/src/db/replay.rs
@@ -816,44 +816,48 @@ mod tests {
                             .map(|(s1, s2)| (s1.as_ref(), s2.as_ref()))
                             .collect();
 
-                        Self::eval_assert(
-                            || {
-                                assert_eq!(&partitions_actual, partitions);
-                            },
-                            use_assert,
-                        )
+                        if use_assert {
+                            assert_eq!(&partitions_actual, partitions);
+                            true
+                        } else {
+                            &partitions_actual == partitions
+                        }
                     }
                     Check::Query(query, expected) => {
                         let planner = SqlQueryPlanner::default();
                         let ctx = db.new_query_context(None);
 
-                        match planner.query(query, &ctx) {
-                            Ok(physical_plan) => {
-                                match ctx.collect(physical_plan).await {
-                                    Ok(batches) => {
-                                        // we are throwing away the record batches after the assert, so we don't care about interior
-                                        // mutability
-                                        let batches = std::panic::AssertUnwindSafe(batches);
-
-                                        Self::eval_assert(
-                                            || {
-                                                assert_batches_eq!(expected, &batches);
-                                            },
-                                            use_assert,
-                                        )
-                                    }
-                                    err if use_assert => {
-                                        err.unwrap();
-                                        unreachable!()
-                                    }
-                                    _ => false,
-                                }
-                            }
+                        let physical_plan = match planner.query(query, &ctx) {
+                            Ok(physical_plan) => physical_plan,
                             err if use_assert => {
                                 err.unwrap();
                                 unreachable!()
                             }
-                            _ => false,
+                            _ => {
+                                return false;
+                            }
+                        };
+
+                        let batches = match ctx.collect(physical_plan).await {
+                            Ok(batches) => batches,
+                            err if use_assert => {
+                                err.unwrap();
+                                unreachable!()
+                            }
+                            _ => {
+                                return false;
+                            }
+                        };
+
+                        if use_assert {
+                            assert_batches_eq!(expected, &batches);
+                            true
+                        } else {
+                            let formatted =
+                                arrow_util::display::pretty_format_batches(&batches).unwrap();
+                            let actual_lines = formatted.trim().split('\n').collect::<Vec<_>>();
+
+                            expected == &actual_lines
                         }
                     }
                 };
@@ -865,35 +869,6 @@ mod tests {
 
             true
         }
-
-        /// Evaluates given function that may contain an `assert`.
-        ///
-        /// This helper allows you use the same `assert` statement no matter if you want the error case to panic or if
-        /// you just want to have an boolean expression that states "did it succeed?".
-        ///
-        /// Behavior dependson `raise`:
-        ///
-        /// - `raise = true`: The given function `f` will be executed. If it panics, `eval_assert` will just bubble up
-        ///   the error (aka panic as well). If the function `f` does not panic, `true` is returned (aka "it
-        ///   succeeded").
-        /// - `raise = false`: The given function `f` will be executed but unwinds will be caught. If `f` did panic
-        ///   `false` will be returned (aka "it failed"), otherwise `true` will be returned (aka "it succeeded").
-        fn eval_assert<F>(f: F, raise: bool) -> bool
-        where
-            F: Fn() + std::panic::UnwindSafe,
-        {
-            if raise {
-                f();
-                true
-            } else {
-                // catch unwind w/o printing backtrace
-                let hook_backup = std::panic::take_hook();
-                std::panic::set_hook(Box::new(|_| {}));
-                let res = std::panic::catch_unwind(f);
-                std::panic::set_hook(hook_backup);
-                res.is_ok()
-            }
-        }
     }
 
     impl Default for ReplayTest {