From 0fe665e509340fa5ed88632e2e32692b867b08fc Mon Sep 17 00:00:00 2001
From: Etan Kissling <etan@status.im>
Date: Mon, 22 Jan 2024 07:27:34 +0100
Subject: [PATCH] only run `teardown` if `setup` completed (#36)

* only run `teardown` if `setup` completed

With #35, it is no longer possible in `teardown` to refer to variables
introduced in `setup`. To avoid having to rewrite tests, address the
issue in #35 by wrapping `body` with an additional exception catching
layer. This then allows to re-use the previous `teardown` semantics
where `teardown` only ran if `setup` fully completed and also has access
to the variables introduced in `setup`.

* workaround shadowing bug

* Add test

---------

Co-authored-by: jangko <jangko128@gmail.com>
---
 tests/tunittest.nim | 15 +++++++++++++++
 unittest2.nim       | 42 +++++++++++++++++-------------------------
 2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/tests/tunittest.nim b/tests/tunittest.nim
index 12ded76..1ed528f 100644
--- a/tests/tunittest.nim
+++ b/tests/tunittest.nim
@@ -46,6 +46,20 @@ suite "PR #35":
     # emulate exception
     raise newException(ValueError, "error")
 
+suite "PR #36":
+  # ensure variables declared in setup section
+  # still accessible from teardown section
+  setup:
+    var server: ref string
+
+  teardown:
+    if server.isNil.not:
+      testStatusIMPL = TestStatus.OK
+
+  test "test body":
+    server = new(string)
+    server[] = "hello"
+    
 #------------------------------------------------------------------------------
 # Regular tests
 #------------------------------------------------------------------------------
@@ -213,3 +227,4 @@ when defined(testing):
 
 # Also supposed to work outside tests:
 check 1 == 1
+      
\ No newline at end of file
diff --git a/unittest2.nim b/unittest2.nim
index c8a7cfc..d6b2005 100644
--- a/unittest2.nim
+++ b/unittest2.nim
@@ -1103,36 +1103,28 @@ template runtimeTest*(nameParam: string, body: untyped) =
     let suiteName {.inject, used.} = suiteName
     let testName {.inject, used.} = testName
 
-    try:
-      when declared(testSetupIMPLFlag): testSetupIMPL()
-      block:
-        body
-
-    except CatchableError as e:
-      let eTypeDesc = "[" & $e.name & "]"
-      checkpoint("Unhandled error: " & e.msg & " " & eTypeDesc)
+    template fail(prefix: string, eClass: string, e: auto): untyped =
+      let eName = "[" & $e.name & "]"
+      checkpoint(prefix & "Unhandled " & eClass & ": " & e.msg & " " & eName)
       var stackTrace {.inject.} = e.getStackTrace()
       fail()
 
-    except Defect as e: # This may or may not work dependings on --panics
-      let eTypeDesc = "[" & $e.name & "]"
-      checkpoint("Unhandled defect: " & e.msg & " " & eTypeDesc)
-      var stackTrace {.inject.} = e.getStackTrace()
-      fail()
-    except Exception as e:
-      let eTypeDesc = "[" & $e.name & "]"
-      checkpoint("Unhandled exception that may cause undefined behavior: " & e.msg & " " & eTypeDesc)
-      var stackTrace {.inject.} = e.getStackTrace()
-      fail()
-    finally:
+    template failingOnExceptions(prefix: string, code: untyped): untyped =
       try:
-        when declared(testTeardownIMPLFlag):
-          testTeardownIMPL()
+        code
+      except CatchableError as e:
+        prefix.fail("error", e)
+      except Defect as e: # This may or may not work dependings on --panics
+        prefix.fail("defect", e)
       except Exception as e:
-        let eTypeDesc = "[" & $e.name & "]"
-        checkpoint("Exception when calling teardown: " & e.msg & " " & eTypeDesc)
-        var stackTrace {.inject.} = e.getStackTrace()
-        fail()
+        prefix.fail("exception that may cause undefined behavior", e)
+
+    failingOnExceptions("[setup] "):
+      when declared(testSetupIMPLFlag): testSetupIMPL()
+      defer: failingOnExceptions("[teardown] "):
+        when declared(testTeardownIMPLFlag): testTeardownIMPL()
+      failingOnExceptions(""):
+        body
 
     checkpoints = @[]