report failed assertions in return value of `run` function

[dchelimsky]

When a match? assertion fails, the failure info is printed to the repl, and the return value of run doesn't include any information about it:

(run
  (flow "interact with db"
    [db (state/gets :db)]
    (state/wrap-fn #(register-user db {:name "Phillip"}))
    (match? "user got added"
            (state/wrap-fn #(fetch-users db))
            #{{:name "Philip"}})) ;; <- different spelling
  {:db (atom {:users #{}})})
;; => #<Pair [#{{:name "Phillip"}} {:db #atom[{:users #{{:name "Phillip"}}} 0x4c2a27a6]}]>

;; --- printed to repl ---
;; FAIL in () (form-init13207122878088623810.clj:256)
;; interact with db (line 253) -> user got added
;; expected: (match? #{{:name "Philip"}} actual__12555__auto__)
;; actual: #{{:name (mismatch "Philip" "Phillip")}}
;; --- /printed to repl ---

This is fine if you're typing in the repl, but if you're using an dev environment in which you type in source files and send expressions to a connected repl, things get a bit disconnected.

I'm not sure about the format yet, but I think we should report assertion failures in the state, e.g.

#<Pair [#{{:name "Phillip"}}
        {:db #atom[{:users #{{:name "Phillip"}}} 0x4c2a27a6]
         :failed-assertions
         [{:location "form-init13207122878088623810.clj:256"
            :name "interact with db (line 253) -> user got added"
            :expected (match? #{{:name "Philip"}} actual__12555__auto__)
            :actual #{{:name (mismatch "Philip" "Phillip")}}}]}]>

Then, perhaps, the run function could inspect state for failures and include them in the return - possibly wrapping them in a Failure object to align with runtime exceptions.

[philomates]

This sounds like the idea that @sovelten has been pushing forward that test frameworks should avoid being effectul until the very last moment possible. As in, they should decouple evaluating tests with reporting tests.

I think implementing this here might be a nice way to see whether it is a worthwhile idea to push forward in an actual test framework, either by adapting an existing one or writing a new one.

This should be relatively straightforward to implement with matcher-combinators because currently state-flow uses the standalone match? construct to do probing and only uses the clojure.test version for failure reporting.

I want to cc @plexus on this because he has a bunch of experience running and reporting tests via the kaocha work. I've dug into some of the kaocha code for midje and clojure.test and I can only imagine that having the run/report split would make writing a test runner much easier. What do you think of this idea @plexus?

[plexus]

In Kaocha we basically have both, there's the functional view where a test run means transforming a test config into a test result. While that's happening we emit events which reporters can listen to to provide real-time feedback. That's the theory: it's just data in data out, but because users need some kind of progress reporting during a test run we have these imperative events, as a layer of observability on that.

It gets murky though because all you get from typical clojure.test assertions are the events they emit, they don't return data as such, just side effects. So we then have to record these events and use them to build up the test result data structure, which is kind of backwards, but we do this so API consumers (e.g. plugins) can pretend everything is just data-in data-out.

[caioaao]

I think this is coupling a lot of things that shouldn't be coupled. If you're using clojure.test as the test runner, then it's clojure.test behavior that is what needs to be changed. If it's midje, then it's midje's behavior. You can't expect to use a test framework and getting a different result from using it.

I think a cleaner solution is to decouple the test running. As mentioned by @plexus, Kaocha already solves that in a way. Capturing match results is coupling state-flow to matcher-combinators implementation, and I don't think that's a good thing.

[dchelimsky]

Capturing match results is coupling state-flow to matcher-combinators implementation, and I don't think that's a good thing.

That is already the case, so perhaps we need to think about decoupling that.

[caioaao]

yeah, I'm already not a fan of the state-flow.cljtest/match? implementation for some reasons to be honest. It's a helper function that's completely unrelated to this project and quite misleading (would be a lot better if it were something like probe-match? on the matcher-combinators library instead IMO)

[dchelimsky]

yeah, I'm already not a fan of the state-flow.cljtest/match? implementation for some reasons to be honest

Can you elaborate?

[caioaao]

Can you elaborate?

Besides what I said about it not being related at all to state-flow, the other issues I have are:

1. It's not a match. It's a match with a retry. This is not apparent to the user and I just found out when the test started failing and I didn't want it to retry.
2. This is actually an implementation bug, but if actual parameter is a function, it will be called several times, since part of the macro expansion of (cljtest/match? "bla" (possibly-side-effectful-f) {}) is like this:

   (if (state/state? (possibly-side-effectful-f))
     (match-probe (possibly-side-effectful-f) {} nil)
     (state/return (possibly-side-effectful-f)))

3. I had better results by writing the probe logic myself and using it with state-flow.labs.cljtest/testing. I prefer having this logic exposed in an explicit way rather than implicit and a bit convoluted imo

[philomates]

1. It's not a match. It's a match with a retry. This is not apparent to the user and I just found out when the test started failing and I didn't want it to retry.

I've been tempted recently to set the default retry count to 0; it generally isn't useful in the single-service integration test context

3. I had better results by writing the probe logic myself and using it with state-flow.labs.cljtest/testing. I prefer having this logic exposed in an explicit way rather than implicit and a bit convoluted imo

Do you find yourself writing a bit more var binding boiler-plate to get results of subflows with the testing approach? That is one of the main benefits of match? that I see.

[dchelimsky]

This is actually an implementation bug, but if actual parameter is a function, it will be called several times,

That's an undocumented feature, not a bug :)

I've been tempted recently to set the default retry count to 0

There is times-to-try, not e.g. retry-count, so it'd have to be 1. That said, making it so would be a breaking change.

It's not a match. It's a match with a retry

a better docstring would help a lot.

Another problem is that match? reverses the argument order: (match? desc actual expected) (whereas matcher-combinators.matchers/match? is (match? expected-or-matcher actual)). I'd proposed introducing a new name, expect, so we could take care of arg order, and we could also deal w/ better docs.

probe-match?

I like the idea of making that explicit, so we'd have match? and probe-match? (or expect and probe-expect). One benefit of that is that we could still supply a monadic value to the non-probing one, vs defaulting the probing one to one try, etc.

[caioaao]

Do you find yourself writing a bit more var binding boiler-plate to get results of subflows with the testing approach? That is one of the main benefits of match? that I see.

I do, but I'd rather have that boilerplate and make things more explicit. It's weird for me to match some monadic value with a non-monadic one without any hints from the matcher function (and I didn't even knew that was a feature before this discussion btw). I like the idea of expect and probe-expect, but it still doesn't make it clear it takes a monadic value as an argument. Maybe something like state/expect instead of cljtest/expect would be better, but I don't know how one would go about doing it

[caioaao]

I've created a PoC on #75 that I believe is a cleaner solution to this. And as I said, I'd rather have probe and match to be something external to the state-flow library and not create this unnecessary coupling