return a map from match? #110

dchelimsky · 2020-04-06T17:41:27Z

No description provided.

dchelimsky · 2020-04-06T17:41:45Z

Addressing #108

dchelimsky · 2020-04-06T17:42:45Z

@sovelten I still need to add the report->actual fn before I submit this as "not a draft". Feel free to review now or wait.

sovelten · 2020-04-06T18:29:03Z

src/state_flow/assertions/matcher_combinators.clj

-                     (state/ensure-step ~actual))]
+                     (state/ensure-step ~actual))
+        report#    (state/return (matcher-combinators/match ~expected actual#))]
+       (state/modify update :match/results (fnil conj []) (assoc report# :match/desc flow-desc#))


Are we going to modify the state or the meta?
I was thinking that state-flow specific stuff could be on the meta, while user specific stuff on the state, so as not to conflict with whatever the user wants to have as a state.

On another note, this statement is a bit too big, maybe it deserves its own function

Side note: this doesn't even need to be here yet. I added it (yes, to the wrong place - will put on meta) with reporting in mind, so the runner can analyze the match results and print failure information when appropriate. I may end up removing it from here for now and restore it when we get to that decoupling.

sovelten · 2020-04-06T18:32:00Z

src/state_flow/assertions/matcher_combinators.clj

  (m/fmap (comp :value last)
-          (probe/probe state


You want to fetch the result of all tries instead of only value of the last try here.

@sovelten I removed the bit about storing these results in the state (or meta) from this PR, so I don't think that matters (for this PR). Agree?

I changed my mind. The return of match? now looks like this when (> times-to-try 1)

{:probe/sleep-time 110 :probe/times-to-try 3 :probe/results [{:check-result false :value 0} {:check-result true :value 2}] :match/expected 2 :match/actual 2 :match/result :match}

FYI - adding this to the meta of the state will allow us to move the cljtest reporting to the cljtest namespace

Looks good to me 🎉

One thing I'm not sure about is whether we should document the keys in that map or just say "a map with information about the success or failure of the match, the details of which are used internally by state-flow and subject to change". I want to discourage anybody, for now, from treating that map's shape as an API the you can program against. I might see this differently once we have the decoupling design worked out.

I agree, I would keep this model as internal for now (or ever I don't know)

dchelimsky · 2020-04-06T23:06:25Z

@sovelten I added the report->actual fn we discussed in #108. I'm not in love with the name, but I don't know that I have a better choice. WDYT about extract-actual, or something cute like <<actual?

[actual (<<actual (match? 3 (+ 1 2)))]

…he actual value

match? was deciding whether to use probe to try 1 or more times, and then probe was making the same decision. Now it only happens in probe, letting us remove duplicate conditional logic from match?

sovelten · 2020-04-07T15:58:39Z

src/state_flow/assertions/matcher_combinators.clj

+  [report-or-match]
+  `(m/do-let
+    [report# (state/ensure-step ~report-or-match)]
+    (state/return (:match/actual report#))))


I'm not a fan of this approach. I'm not a fan of the so called "true union types" like things can be one thing or the other.
I find it quite confusing, not well-typed 😬 .

What are the problems you see with just a function? Is it to avoid exposure to fmap? fmap is beautiful ✨ . But also avoidable with just a function by using :let.

Also I don't understand the need for a macro here.

match? supports a step or a value for actual. I was just going along w/ that idea, but I appreciate your perspective here. Changing it to a function.

Yeah, maybe it is just a longing for orderliness but that is also one reason why I was defending a different match? for probing.

But that may be a different case, because you cannot write match-probe? in terms of match? but you can compose report->actual with fmap.

Just rambling, don't mind me.

OK @sovelten - it's a fn now and there's even fmap in the test.

Also move match? tests to a matcher-combinators-test namespace and clean some things up.

sovelten · 2020-04-07T19:27:56Z

test/state_flow/cljtest_test.clj

-            [matcher-combinators.matchers :as matchers]
-            [state-flow.assertions.matcher-combinators :as assertions.matcher-combinators]
-            [state-flow.test-helpers :as test-helpers :refer [this-line-number]]
+            [state-flow.assertions.matcher-combinators :as mc]


avoid acronyms?

Totally agree 99.9999% of the time. assertions.matcher-combinators/xxx all over the place was making my eyes bleed.

Also, matcher-combinators/xxx would be confusing, since it's not coming from the matcher-combinators library. I guess assertions could work, but that's also confusing :) So I went with a plua (you know, a poor, lonely, unwanted acronym).

how about matcher?

I'd rather just go back to assertions.matcher-combinators, which won't be confused with things in other parts of the test suite.

I could just refer match? and report->actual, but then we'd end up with:

(testing "with explicit matcher for expected" (let [[ret state] (state/run (match? (matchers/equals 3) 3) {})] (is (match? {:match/result :match} ret))))

The two match?s are different. They both work because one is a function and the other is a symbol used to dispatch a multimethod, but they are confusing together!

I'm going with the acronym for this PR. We can always revisit this later.

sovelten · 2020-04-07T19:32:47Z

test/state_flow/test_helpers.clj

  (state/modify (fn [state]
                  (future (do (Thread/sleep delay-ms)
-                              (swap! (:value state) + 2)))
+                              (apply f state args)))


That only works for atoms though. Applying a pure function would be kinda pointless.

Maybe make all of state an atom? Then we could have (apply swap! state f args)

I think we expect state to be a map. If the whole state is an atom there is no point of passing it around, the atom is always the same. But this is test code, I'm not really concerned. Just wondering why you took that route.

I find that every time I look at these tests I have to go look at the definition of delayed-add-two, so I wanted to refactor it so I can see what's happening in the test while still avoiding some of the duplication.

src/state_flow/assertions/matcher_combinators.clj

sovelten

Looks good to me 💯

I would just make sure to bump a major version to send a clear warning that this is a breaking change.
Also point out how to adapt old code.

Nice work :)

CHANGELOG.md

src/state_flow/assertions/matcher_combinators.clj

CHANGELOG.md

sovelten · 2020-04-07T21:43:03Z

CHANGELOG.md

+;; if you were doing this before
+[actual (match? <expected> <step-that-produces-actual>)]
+;; you can do this, now
+[actual (fmap report->actual (match? <expected> <step-that-produces-actual>)]


[actual (clojure.cats.core/fmap report->actual (match? <expected> <step-that-produces-actual>))]

^-- missing a parenthesis at the end as well 😬

also add this other option?

[report (match? <expected> <step-that-produces-actual>) :let [actual (report->actual report)]]

fernando-nubank

👍

sovelten reviewed Apr 6, 2020

View reviewed changes

dchelimsky marked this pull request as ready for review April 6, 2020 23:03

dchelimsky requested a review from a team April 6, 2020 23:03

dchelimsky added 3 commits April 7, 2020 07:49

return a map from match? including the actual value instead of just t…

e225059

…he actual value

don't store report on state

3cdd969

add report->actual helper fn

d55da92

dchelimsky force-pushed the dc/return-map-from-match branch from 26536cc to d55da92 Compare April 7, 2020 12:49

dchelimsky added 2 commits April 7, 2020 08:25

just use probe

8bc80f3

match? was deciding whether to use probe to try 1 or more times, and then probe was making the same decision. Now it only happens in probe, letting us remove duplicate conditional logic from match?

improve match report

3f803ad

sovelten reviewed Apr 7, 2020

View reviewed changes

dchelimsky added 2 commits April 7, 2020 11:00

doc the return of match? without detail

3d80ebf

make report->actual a function instead of a macro

61f74cc

Also move match? tests to a matcher-combinators-test namespace and clean some things up.

dchelimsky requested a review from a team April 7, 2020 19:25

sovelten reviewed Apr 7, 2020

View reviewed changes

src/state_flow/assertions/matcher_combinators.clj Outdated Show resolved Hide resolved

sovelten approved these changes Apr 7, 2020

View reviewed changes

dchelimsky added 3 commits April 7, 2020 15:07

cleaner test-helper

2bb7d13

include probe info all the time (no need for conditional)

2ef8709

update changelog and bump version

121326a

rafaelzlisboa approved these changes Apr 7, 2020

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

src/state_flow/assertions/matcher_combinators.clj Show resolved Hide resolved

dchelimsky commented Apr 7, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

6515f27

sovelten reviewed Apr 7, 2020

View reviewed changes

Changelog

5d42392

markomafs approved these changes Apr 8, 2020

View reviewed changes

fernando-nubank approved these changes Apr 8, 2020

View reviewed changes

dchelimsky merged commit d37d756 into master Apr 8, 2020

dchelimsky deleted the dc/return-map-from-match branch April 8, 2020 15:16

dchelimsky mentioned this pull request Apr 8, 2020

The match? assertion should return either a boolean or other indication of success/failure #108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

return a map from match? #110

return a map from match? #110

dchelimsky commented Apr 6, 2020

dchelimsky commented Apr 6, 2020

dchelimsky commented Apr 6, 2020

sovelten Apr 6, 2020

sovelten Apr 6, 2020

dchelimsky Apr 6, 2020

sovelten Apr 6, 2020

dchelimsky Apr 6, 2020

dchelimsky Apr 6, 2020

dchelimsky Apr 7, 2020

dchelimsky Apr 7, 2020

sovelten Apr 7, 2020

dchelimsky Apr 7, 2020

sovelten Apr 7, 2020

dchelimsky commented Apr 6, 2020

sovelten Apr 7, 2020

dchelimsky Apr 7, 2020

sovelten Apr 7, 2020

dchelimsky Apr 7, 2020

sovelten Apr 7, 2020

dchelimsky Apr 7, 2020

dchelimsky Apr 7, 2020

sovelten Apr 7, 2020

dchelimsky Apr 7, 2020

dchelimsky Apr 7, 2020

dchelimsky Apr 7, 2020

sovelten Apr 7, 2020

dchelimsky Apr 7, 2020

sovelten Apr 7, 2020

dchelimsky Apr 7, 2020

sovelten left a comment

sovelten Apr 7, 2020

fernando-nubank left a comment

return a map from match? #110

return a map from match? #110

Conversation

dchelimsky commented Apr 6, 2020

dchelimsky commented Apr 6, 2020

dchelimsky commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchelimsky commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sovelten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernando-nubank left a comment

Choose a reason for hiding this comment