The match? assertion should return either a boolean or other indication of success/failure
dchelimsky opened this issue · 13 comments
The state-flow.assertions.matcher-combinators/match? step (monad) returns a value:
(run
(flow "increment count"
(state/modify update :count inc)
(match? 1 (state/gets :count)))
{:count 0})
;; => [1 {:count 1}]The left value, 1, doesn't help us understand whether the match? succeeded or failed. Do know that, we have to look at the REPL or the output log.
I recommend we change it to return a map with the success/fail information matcher-combinators reports to clojure test.
match? should actually return the return value of the right value (now that the order is inverted).
This is important for probing. If you need to do a probe test and you also need to use whatever the probed step returns.
...
[actual (match? expected probe-step {:times 2})]
(some-request actual)
...If it wasn't for that you could do
[actual step
test-result (match? expected actual)]Probably one more reason for splitting match? and mach-probe?
Another solution is to store both in a map {:expected ... :actual ... :report ...}
...
[{the-thing-i-need :actual} (match? expected probe-step {:times 2})]
(some-request the-thing-i-need)
...breaking change though
match? uses probe internally, and can continue to do while still returning something different. Why would you do this:
...
[actual (match? expected probe-step {:times 2})]
(some-request actual)
...When you could just use probe directly, if that's your intent.
re: {:expected ... :actual ... :report ...}, I like that idea, though I think that should all be in the report returned from matcher-combinators.
match?usesprobeinternally, and can continue to do while still returning something different. Why would you do this:... [actual (match? expected probe-step {:times 2})] (some-request actual) ...When you could just use probe directly, if that's your intent.
You want to make a match? assertion and you also want the return value of the probe-step. Does that make sense? probe is set for internal use only right now.
breaking change though
I realize it's a change in "how it works" if you happen to know how match? works today, but it's return is not documented, so for me it's not a breaking change. Also, I'm going to guess that you're the only user using it this way :)
You want to make a match? assertion and you also want the return value of the probe-step.
That's coupling two concepts. What if we added something that could get the last actual value e.g.
(match? expected probe-step {:times 2})
[actual (value-of-last-probe)]That way the way we get that value is an implementation detail that we're free to change.
I realize it's a change in "how it works" if you happen to know how
match?works today, but it's return is not documented, so for me it's not a breaking change. Also, I'm going to guess that you're the only user using it this way :)
Unfortunately I'm not the only user :P
This was thought out from the beginning, by Rafael Ferreira suggestion, I'm not sure he even remembers that, though. I'm not against breaking changes though, just warning that some things will break. My motto is better break now than break later.
You want to make a match? assertion and you also want the return value of the probe-step.
That's coupling two concepts. What if we added something that could get the last actual value e.g.
(match? expected probe-step {:times 2}) [actual (value-of-last-probe)]
That sounds too stateful too me. I'd rather go to either match? returning a map that includes the return value of the probed step or having a specific probe matcher.
That sounds too stateful too me. I'd rather go to either match? returning a map that includes the return value of the probed step or having a specific probe matcher.
I'd rather return the map than add a separate function. Even if we did add match-probe? we'd need to deprecate the probe params on match? and then we'd have to think about aligning the outputs of match? and match-probe?
To me, the cleanest move is changing the return to the map you propose. There will be some redundant info in the :report value (expected and actual may appear there, too), but I can live with that.
I'd rather return the map than add a separate function. Even if we did add
match-probe?we'd need to deprecate the probe params onmatch?and then we'd have to think about aligning the outputs ofmatch?andmatch-probe?To me, the cleanest move is changing the return to the map you propose. There will be some redundant info in the
:reportvalue (expected and actual may appear there, too), but I can live with that.
The value doesn't need to be in that place, it just needs to be available. It could be in the report. Then it is just a matter of using a function to access it.
[actual (m/fmap report->actual (match? expected step))]
This is the design philosophy that I followed in the refactoring of probe, just return everything you have, the client can then pick whatever it wants. Same thing for with-delta. This prevents us to resort to stateful tricks.
If you follow through with that, the report should also have all the tries and the result of each try.
If you follow through with that, the report should also have all the tries and the result of each try.
I like that.
The value doesn't need to be in that place, it just needs to be available. It could be in the report. Then it is just a matter of using a function to access it.
I like the idea of the report->actual function, though I'd propose that it encapsulates the fmap, so you can do:
[actual (m/fmap report->actual (match? expected step))]WDYT?
[actual (m/fmap report->actual (match? expected step))]WDYT?
What is the difference?
Anyway, this is just my way of thinking, you can also do this:
[report (match? expected probe-step)
:let [actual (report->actual report)]]fmap goes away then. Important part is that there is a function for that, regardless of how it might be used.
What is the difference?
The difference is that I meant to leave out fmap in that example and forgot!
I think we're on the same page now.
Actually, almost the same page. I'm thinking of supporting both of these:
[report (match? expected probe-step)
:let [actual (report->actual report)]]
;; and
[actual (report->actual (match? expected probe-step))]That way if you don't need the report you don't need to bind it.
This was a addressed in #110 and released in 3.0.0