marick / Midje

Midje provides a migration path from clojure.test to a more flexible, readable, abstract, and gracious style of testing
MIT License
1.68k stars 128 forks source link

Unexpected exceptions cause load failure and test run is not completed #461

Open Juholei opened 5 years ago

Juholei commented 5 years ago

If a test code calls code that throws an exception but the exception is not expected in the test (so it's a situation that should not happen), Midje does not run further tests. Instead there is a load failure loading the namespace where the exception originated and rest of the test run is not run and there are no results.

I think in case of an unexpected exception, it should be logged and the particular test should fail. The test run as a whole should continue and there should be results at the end of it.

Basic test namespace to reproduce this:

(ns load-failure.core-test
  (:require [midje.sweet :refer :all]))

(fact "testi"
  (throw (Exception. "exception"))
  (+ 1 1) => 2)

(fact " testi 2"
  (+ 1 1) => 2)

Here the exception is thrown directly in the test code, but it could also originate from your source code.

Running this with lein-midje ends up with the following:

LOAD FAILURE for load-failure.core-test clojure.lang.Compiler$CompilerException: Syntax error compiling at (load_failure/core_test.clj:4:1). data: #:clojure.error{:phase :compile-syntax-check, :line 4, :column 1, :source "load_failure/core_test.clj"} java.lang.Exception: exception load-failure.core-test/eval10638/fn/fn core_test.clj: 4 midje.util.thread-safe-var-nesting/with-altered-roots* thread_safe_var_nesting.clj: 32 load-failure.core-test/eval10638/fn core_test.clj: 4 ... midje.checking.facts/check-one/fn facts.clj: 32 midje.checking.facts/check-one facts.clj: 31 midje.checking.facts/creation-time-check facts.clj: 36 load-failure.core-test/eval10638 core_test.clj: 4 ... clojure.core/load/fn core.clj: 6126 clojure.core/load core.clj: 6125 clojure.core/load core.clj: 6109 ... clojure.core/load-one core.clj: 5908 clojure.core/load-lib/fn core.clj: 5948 clojure.core/load-lib core.clj: 5947 clojure.core/load-lib core.clj: 5928 ... clojure.core/apply core.clj: 667 clojure.core/load-libs core.clj: 5985 clojure.core/load-libs core.clj: 5969 ... clojure.core/apply core.clj: 667 clojure.core/require core.clj: 6007 (repeats 2 times) ... midje.repl/load-facts/fn/fn repl.clj: 208 midje.repl/load-facts/fn repl.clj: 208 midje.repl/load-facts repl.clj: 194 (repeats 2 times) ... user/eval10625 REPL Input ... clojure.main/load-script main.clj: 452 clojure.main/init-opt main.clj: 454 clojure.main/initialize main.clj: 485 clojure.main/null-opt main.clj: 519 clojure.main/main main.clj: 598 clojure.main/main main.clj: 561 ... clojure.main.main main.java: 37

philomates commented 5 years ago

Hello, thanks for logging this issue and the reproduction steps.

I definitely agree that the suggested behavior would lead to a better workflow and I tried to solve this already in https://github.com/marick/Midje/pull/441 (this same issue has been brought up in https://github.com/marick/Midje/issues/302 and https://github.com/marick/Midje/issues/336).

I ended up getting stuck with my attempt to fix the issue and abandoning it as "too hard / tricky". I wish I had written more details in the PR because I can't remember exactly was tricky about it.

That said, it will probably take me a while to revisit this issue, if ever because I'm more focused on other testing tools and I already failed once at a fix :)

philomates commented 5 years ago

I remembered (vaguely) what was so hard about this fix: I found it difficult to make reporting work with nested facts when you catch exceptions thrown in non-check expressions of a fact.

Juholei commented 5 years ago

I haven't looked at the source code that closely, but I presume it wouldn't then be possible to just wrap the body in the fact macro with a try-catch and then fail the test something is caught? I'd guess that the exceptions that are being checked in a test are caught by the checker?