New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return non-zero status when tests fail #12

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@Olical

Olical commented Mar 7, 2018

Fix #9

@puredanger

This comment has been minimized.

Contributor

puredanger commented Mar 23, 2018

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

@Olical

This comment has been minimized.

Olical commented Mar 24, 2018

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.

@lxsli

This comment has been minimized.

lxsli commented May 11, 2018

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

@lwhorton

This comment has been minimized.

lwhorton commented May 21, 2018

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

This comment has been minimized.

Contributor

puredanger commented May 21, 2018

Fail = assertion failed
Error = threw exception

@aviflax

This comment has been minimized.

aviflax commented May 23, 2018

Any chance this might get merged soon?

@Olical

This comment has been minimized.

Olical commented May 24, 2018

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.

@Olical Olical force-pushed the Olical:error-exit-codes branch from 427a16c to 7c4f5bd May 24, 2018

@aviflax

This comment has been minimized.

aviflax commented May 24, 2018

@Olical

This comment has been minimized.

Olical commented May 24, 2018

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

This comment has been minimized.

aviflax commented May 24, 2018

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

This comment has been minimized.

Olical commented May 24, 2018

@aviflax

This comment has been minimized.

aviflax commented May 24, 2018

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

This comment has been minimized.

Olical commented May 24, 2018

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 😭 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

This comment has been minimized.

aviflax commented May 24, 2018

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).

mainej added a commit to mainej/test-runner that referenced this pull request Jul 6, 2018

Return non-zero exit code when tests fail or error
Fixes cognitect-labs#9

Based on cognitect-labs#12, with the
following differences:

- Returns non-zero exit code on either failure or error.
- Ensures `shutdown-agents` runs before exiting.
  - This commit avoids a problem with PR 12, which calls `System/exit`
    before the finally block. That approach skips the finally block (1),
    unless `System/exit` is configured to throw a `SecurityException`
    (2), which I don't believe is the default in Clojure.

1) https://stackoverflow.com/questions/65035/does-finally-always-execute-in-java
2) https://stackoverflow.com/questions/1410951/how-does-javas-system-exit-work-with-try-catch-finally-blocks
@levand

This comment has been minimized.

Contributor

levand commented Jul 13, 2018

Fix in another PR. Thanks!

@levand levand closed this Jul 13, 2018

@Olical Olical deleted the Olical:error-exit-codes branch Jul 17, 2018

@Olical Olical restored the Olical:error-exit-codes branch Jul 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment