Return non-zero status when tests fail

[Olical]

Fix #9

[puredanger]

What if it just returned the fail count as the exit code?

[Olical]

Hmm, kinda neat since you get more information as well as an elegant piece of code as a solution. Buuuut, isn't that abusing the exit status and overloading it with more information than it was intended for?

After reading this thread, I feel like it's kind of abusing it for the wrong reasons even more https://unix.stackexchange.com/questions/418784/what-is-the-min-and-max-values-of-exit-codes-in-linux

In my opinion, the only thing we're encoding is "it will be non-zero when something went wrong". By convention, this usually means 1. Theoretically we could have 2000 tests fail though, right? Which seems like it could go horribly wrong in some strange edge cases.

[elzibubble]

If you return the failed count as exit code, when 256 tests fail it'll wrap to 0.

[lwhorton]

The summary is currently spitting out

{:test 3, :pass 2, :fail 0, :error 1, :type :summary}

What's the distinction between a failing test and an error? Would we want a 1 exit code on (not (zero? (+ fail error)))?

[puredanger]

Fail = assertion failed Error = threw exception

[aviflax]

Any chance this might get merged soon?

[Olical]

I realised a while back that I'm killing the process which would be a pretty nasty breaking change for existing users that may run this from a REPL or something. So maybe I need to make some changes around that part.

I may have to check :fail as well as :error counts too, maybe just one isn't the right way. Like if :fail is 0 but there was an error it'll still think everything is okay and return 0.

[aviflax]

Ah yeah that makes sense. Thanks!

[Olical]

I've just rebased the branch ontop of master. If we want to check fail and error either myself or someone else can apply this patch:

diff --git a/src/cognitect/test_runner.clj b/src/cognitect/test_runner.clj
index b6e9bbf..96eaaac 100644
--- a/src/cognitect/test_runner.clj
+++ b/src/cognitect/test_runner.clj
@@ -114,7 +114,7 @@
       (if (-> args :options :test-help)
         (help args)
         (try
-          (let [{:keys [fail]} (test (:options args))]
-            (exit (if (zero? fail) 0 1)))
+          (let [{:keys [fail error]} (test (:options args))]
+            (exit (if (zero? (+ fail error)) 0 1)))
           (finally
             (shutdown-agents)))))))

[aviflax]

That looks good! I’m just a little confused about the patch? Why not just apply this change to your branch in your fork, so it’d be included in the PR?

[Olical]

Oh I can do, but I don't know if it's the right thing to do. So if it's fine in the current form someone can just hit merge. If that change is require it's easy to apply.

On Thu, 24 May 2018, 15:15 Avi Flax, notifications@github.com wrote:

That looks good! I’m just a little confused about the patch? Why not just apply this change to your branch in your fork, so it’d be included in the PR?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cognitect-labs/test-runner/pull/12#issuecomment-391730952, or mute the thread https://github.com/notifications/unsubscribe-auth/AATPXRJvIwDnZXeUmL0ahhroezeXn8hAks5t1sCIgaJpZM4ShOFR .

[aviflax]

Ah cool, yeah, that makes sense. Thanks for explaining!

BTW — this is no big deal at all, just thought you might be interested — I’ve been using your branch in my project, and my latest CI build failed because the git SHA I was using has now disappeared, since you rebased the branch. It’s interesting to consider that now that people might be using any commit in any repo in a Clojure project, perhaps we should think twice before rebasing or rewriting any pushed commits!

[Olical]

Oh no! That's really bad, I'm sorry about that! I don't usually rebase I just know a lot of projects prefer it, I usually just merge back into my branch if I need things from master.

I assumed the commit would hang around but not be referenced by anything, apparently I was wrong :sob: force pushing catches me out once again!

Note: I have updated my two posts that mentioned that old sha, they now point at the new one. Having commits vanish could definitely become annoying though. Although I shouldn't have just force pushed like I did, that was silly of me. I can't find any other mention of the old sha on Google.

[aviflax]

No worries! This is a new situation that we’re in the early stages of learning about and adapting to. I for one remain excited by the idea of referencing and including dependencies as code rather than artifacts, using git shas rather than version numbers (which IMHO should be reserved for marketing and reflection, not for identifying revisions).

[levand]

Fix in another PR. Thanks!