Welcome! Please see the About page for a little more info on how this works.

0 votes
in Spec by

When trying to manually wire clojure.test and spec for fdef'ed functions, I realized that spec.test/check returns a map with (link: :failure false) for failing tests. I'm (almost) sure it should return true, because spec.test/abbrev-result receives as argument the return of spec.test/check and tests the value of :failure. I couldn't produce a case where spec.test/check returned (link: :failure true) for failing tests. For information it returns a map without the key :failure for passing test.

Here's a simple reproduction case in the REPL :

(defn foo-fn (link: x) "bar")

(s/fdef foo-fn

    :args (s/cat :x any?)
    :ret string?)

(stest/check `foo-fn) ;; => no error, normal small map printed, no :failure entry

(defn foo-fn (link: x) 1)

(stest/check `foo-fn) ;; => full error map, with :failure equals false. :failure shown below

(-> (stest/check `foo-fn) first :failure) ;; => false

9 Answers

0 votes
by

Comment made by: gfredericks

This is not a bug, though it is confusing.

{{stest/check}} is passing through (at least part of) the return value from {{clojure.test.check/quickcheck}}, which returns the value returned by the property, which is evaluated for thruthiness to determine pass vs fail. Since a non-truthy value can only be {{false}} or {{nil}}, you don't learn very much by looking at it (though it's possible that the {{:failure}} key could also have an exception under it, which would be more informative).

The next release of test.check should have a more useful and less confusing way of extracting information from a test, but I don't know for sure how that would look when using {{stest/check}}.

0 votes
by

Comment made by: djebbz

I still don't understand why you don't consider it a bug. The docstring of stest/check says ":failure optional test failure". I expect to have a :failure key only when the tests fail (hence the wording optional), with some valuable info.

I'm using the stest/abbrev-result to display the output of stest/check, which expects :failure to be truthy to display all the valuable failure information. If stest/check returns false, there's a mismatch I consider a bug. The docstring of stest/abbrev-result explicitly says it receives as argument the result of check, so I'm not forcing a square peg into a round hole.

Thank you for answering me so fast and for your time.

0 votes
by
_Comment made by: grzm_

The {{false}} value for {{:failure}} is definitely confusing. The {{stest/abbrev-result}} is *very* confounding in the {{{:failure false}}} as it doesn't provide additional information that failures usual do,as [~djebbz] points out.

{code:title=https://github.com/clojure/spec.alpha/blob/2824ad49df8deadcb4b75acdf624e732a85b4ac7/src/main/clojure/clojure/spec/test/alpha.clj#L438-L446}
(defn abbrev-result
  "Given a check result, returns an abbreviated version
suitable for summary use."
  [x]
  (if (:failure x)
    (-> (dissoc x ::stc/ret)
        (update :spec s/describe)
        (update :failure unwrap-failure))
    (dissoc x :spec ::stc/ret)))


Here's an example showing how misleading it can be:


(require '[clojure.spec.alpha :as s]
         '[clojure.spec.test.alpha :as stest])

(alias 'stc 'clojure.spec.test.check)

(defn adder [a b]
  (+ a b))

(s/fdef adder
        :args (s/cat :a int? :b int?)
        :ret string?)

(-> (stest/check `adder) first stest/abbrev-result)
;; => {:sym ex.check-test/adder, :failure false}

;; Writing an alternative version of `abbrev-result` that
;; checks for `true`

(defn- failure-type [x] (::s/failure (ex-data x)))
(defn- unwrap-failure [x] (if (failure-type x) (ex-data x) x))

(defn- abbrev-result [x]
  (let [failure (:failure x)]
    (if-not (or (true? failure)
                (nil? failure))
      (-> (dissoc x ::stc/ret)
          (update :spec s/describe)
          (update :failure unwrap-failure))
      (dissoc x :spec ::stc/ret))))

(-> (stest/check `adder) first abbrev-result)
;; => {:spec (fspec :args (cat :a int? :b int?) :ret string? :fn nil),
;;     :sym ex.check-test/adder,
;;     :failure false}


Again, note that *any* truthy {{:failure}} value is going to provide the additional details, while falsey {{:failure}} values will not.

I understand the motivation for not changing the value of the {{:failure}} key. If that value is going to be maintained, I think {{stest/abbrev-result}} should be updated to likewise test explicitly for {{nil}} and {{true}}, rather than truthy for consistency with the result of {{stest/check}}.

0 votes
by

Comment made by: djebbz

Thanks a lot Michael. In the end I went with a small variation, where I don't (dissoc x ::stc/ret) but keep the whole check result because it includes the stack trace, the spec/explain-data map, the shrunk failing case etc.

0 votes
by

Comment made by: grzm

Looks like {{abbrev-result}} should use {{result-type}} to test whether the check passed. Attaching a patch which does this. If you'd like tests to accompany it, I'm happy to resubmit.

Edit: Nope: that was too hasty. Will investigate and resubmit. Sorry for the noise.

Edit 2: Second guessing myself incorrectly. I'm pretty sure I was correct the first time. I think the patch is good.

0 votes
by

Comment made by: gfredericks

note that there are unreleased changes on the master branch of test.check that may influence the best thing to do here. (link: https://github.com/clojure/test.check/blob/master/src/main/clojure/clojure/test/check/results.cljc text: this stuff) in particular.

0 votes
by
_Comment made by: grzm_

Thanks, [~gfredericks]. The Result protocol is something to keep in mind. I think using {{result-type}} is the right abstraction at the level of {{abbrev-result}}. I would think {{Result/passing?}} would be used when decorating the {{quick-check}} results in {{make-check-result}} in lieu of the [{{(true? result)}}|https://github.com/clojure/spec.alpha/blob/2824ad49df8deadcb4b75acdf624e732a85b4ac7/src/main/clojure/clojure/spec/test/alpha.clj#L319] call. Does that make sense to you?
0 votes
by

Comment made by: gfredericks

Yes I think so. But now that I've reviewed the spec code and see how many incidental details about the return value are being relied on (which is probably true for other test.check users, and shouldn't surprise me), I think I will be saving the Result protocol details for a {{quick-check-2}} ((link: TCHECK-142 text: https://dev.clojure.org/jira/browse/TCHECK-142)).

0 votes
by
Reference: https://clojure.atlassian.net/browse/CLJ-2246 (reported by alex+import)
...