avh4 / elm-testable

Makes Cmds and Tasks testable
http://package.elm-lang.org/packages/avh4/elm-testable/latest
41 stars 5 forks source link

Add Wrapped Cmds for Testing Ports and WebSockets #3

Closed rogeriochaves closed 7 years ago

rogeriochaves commented 8 years ago

This fixes #2 :)

rogeriochaves commented 8 years ago

@robertjlooby @avh4 did the refactoring, what do you think?

avh4 commented 8 years ago

Cool, I'll try to look at this this week. My to-do list is a bit of a mess right now, so please do ping me again with a new comment if I don't follow up by the end of the week.

rogeriochaves commented 8 years ago

@avh4 ping :)

avh4 commented 8 years ago

I'm not sure I agree wtih @rogeriochaves suggested refactorings... do we really understand enough about how this works to make this generally apply to all Cmds?

I am still trying to understand a few things:

robertjlooby commented 8 years ago

I would be surprised if containsCmd/assertCalled worked as expected. I mean if comparing Platform.Cmds worked then this whole package wouldn't be needed. I think it would make sense to remove those methods and really just not track WrappedCmd in the EffectsLog at all.

To reiterate from my last comment, I don't think the value in this WrappedCmd type is from making things testable. Ports in particular I don't think can ever be testable given the current implementation in the language.

The value is that you'd still be able to use elm-testable pretty cleanly even if you had to use a port or some other random Platform.Cmd somewhere in your project.

Currently if anywhere in your app you use a port (or some other non-implemented Cmd like a websocket) then you can't use elm-testable the way it was meant to be used (have all your update functions return Testable.Cmds and then use Testable.init/Testable.update in your main). Right now the best you could do is split your update function in to two pieces. One that contains all the untestable Cmds and one that contains the Testable.Cmds. Then you can wrap the second one in the first using Testable.cmd, but this is 1) messier 2) forces everything up the chain to deal with Platform.Cmds.

rogeriochaves commented 8 years ago

@robertjlooby comparing two resulting Cmds does seem to work fine, you can try on the repl:

> import Spelling -- which has a `check` port
> Spelling.check "foo"
{ type = "leaf", home = "check", value = "foo" } : Platform.Cmd.Cmd a
> Spelling.check "bar"
{ type = "leaf", home = "check", value = "bar" } : Platform.Cmd.Cmd a
> (Spelling.check "foo") == (Spelling.check "bar")
False : Bool
> (Spelling.check "foo") == (Spelling.check "foo")
True : Bool

@avh4

  1. yes, they are getting compared correctly, I've added a negative case to EffectsLogTests to check that. I couldn't add it to the cats/dogs on the spelling test example because we don't have a way to make negative tests in elm-testable yet (like assertNotHttpRequest, or assertNotCalled in this case), but yeah, if I change the value going to the port/websocket, the test breaks like it should.
  2. yes, websockets does work, the same way, I've added the example from elm-architecture-tutorial on the PR
  3. main hooking works the same way as the others, I've added the main to the files, and if you run elm make you can see that they all work normally
  4. Well, I guess we could rewrite Testable.Cmd.none to be just an alias of Testable.Cmd.wrap Platform.Cmd.none, then reason to keep that would be just for better usage experience :)
avh4 commented 8 years ago

Okay, thanks! I'll take another look.

I guess I'm wondering about the bigger picture for this package now... If we can just wrap arbitrary Platform Cmds, do we even need Testable.Cmd?

rogeriochaves commented 8 years ago

@avh4 we can test equality for those Cmds because they only contain simple data, since they are communicating with the outside world, but for the other cases, we still can't do that:

> import Http
> import Task
> import Json.Decode as Json
> type Msg = Test

> a = Http.get (Json.at [] Json.string) (Http.url "foo" [])
<task> : Task.Task Http.Error String
> b = Http.get (Json.at [] Json.string) (Http.url "foo" [])
<task> : Task.Task Http.Error String
> a == b
Error: Trying to use `(==)` on functions. There is no way to know if functions are "the same" in the Elm sense. Read more about this at http://package.elm-lang.org/packages/elm-lang/core/latest/Basics#== which describes why it is this way and what the better version will look like.

> c = Task.perform (always Test) (always Test) a
{ type = "leaf", home = "Task", value = T <task> } : Platform.Cmd.Cmd Repl.Msg
> d = Task.perform (always Test) (always Test) a
{ type = "leaf", home = "Task", value = T <task> } : Platform.Cmd.Cmd Repl.Msg
> c == d
Error: Trying to use `(==)` on functions. There is no way to know if functions are "the same" in the Elm sense. Read more about this at http://package.elm-lang.org/packages/elm-lang/core/latest/Basics#== which describes why it is this way and what the better version will look like.

So elm-testable is still very important :)

rogeriochaves commented 8 years ago

Ping @avh4

rogeriochaves commented 8 years ago

@avh4 I've rebased with master upgrading to elm-test 2, can we get this merged?

rogeriochaves commented 8 years ago

@avh4 @robertjlooby friendly ping

avh4 commented 7 years ago

Can someone verify that Cmds are still comparable in the same way in Elm 0.18.0, and I'll plan to get this merged

rogeriochaves commented 7 years ago

@avh4 yep, basically nothing changed (except that repl printing of http request improved):

» elm repl
---- elm-repl 0.18.0 -----------------------------------------------------------
 :help for help, :exit to exit, more at <https://github.com/elm-lang/elm-repl>
--------------------------------------------------------------------------------
> import Spelling
> Spelling.check "foo"
{ type = "leaf", home = "check", value = "foo" } : Platform.Cmd.Cmd msg
> Spelling.check "bar"
{ type = "leaf", home = "check", value = "bar" } : Platform.Cmd.Cmd msg
> (Spelling.check "foo") == (Spelling.check "bar")
False : Bool
> (Spelling.check "foo") == (Spelling.check "foo")
True : Bool
>
>
> import Http
> import Task
> import Json.Decode as Json
> type Msg = Test
>
> a = Http.get "foo" (Json.at [] Json.string)
Request { method = "GET", headers = [], url = "foo", body = EmptyBody, expect = { responseType = "text", responseToResult = <function> }, timeout = Nothing, withCredentials = False }
    : Http.Request String
> b = Http.get "foo" (Json.at [] Json.string)
Request { method = "GET", headers = [], url = "foo", body = EmptyBody, expect = { responseType = "text", responseToResult = <function> }, timeout = Nothing, withCredentials = False }
    : Http.Request String
> a == b
Error: Trying to use `(==)` on functions. There is no way to know if functions are "the same" in the Elm sense. Read more about this at http://package.elm-lang.org/packages/elm-lang/core/latest/Basics#== which describes why it is this way and what the better version will look like.
>
> c = Http.send (always Test) a
{ type = "leaf", home = "Task", value = Perform <task> }
    : Platform.Cmd.Cmd Repl.Msg
> d = Http.send (always Test) a
{ type = "leaf", home = "Task", value = Perform <task> }
    : Platform.Cmd.Cmd Repl.Msg
> c == d
Error: Trying to use `(==)` on functions. There is no way to know if functions are "the same" in the Elm sense. Read more about this at http://package.elm-lang.org/packages/elm-lang/core/latest/Basics#== which describes why it is this way and what the better version will look like.
>

these are the same examples as above

rogeriochaves commented 7 years ago

@avh4 what about now?

avh4 commented 7 years ago

Okay, cool, then the only blocker is upgrading to Elm 0.18, which I am looking at now.

rogeriochaves commented 7 years ago

@avh4 you could merge it, make a new release to 0.17, and then upgrade the codebase to 0.18, right? Is it possible? (maybe elm-package blocks new stuff for 0.17, idk)

avh4 commented 7 years ago

Yeah, I think elm-package won't let me publish for 0.17 anymore. (Though feel free to try publishing your own fork if you want to give it a try.)

avh4 commented 7 years ago

FYI, I've been doing a rewrite this weekend that uses native code so that you can directly test Programs without needing to wrap all your Cmds; and I've been discussing getting that merged into the elm-test runner once it's working.

https://github.com/avh4/elm-testable/tree/rewrite-native

(It already works with Ports and I am working on finishing up HTTP support.)

rogeriochaves commented 7 years ago

@avh4 awesome! Looking forward to that rewrite. In the mean time I've published a fork then: http://package.elm-lang.org/packages/rogeriochaves/elm-testable/latest

it did allow me to publish 0.17, but I might as well upgrade it to 0.18, after all, I like having a pure-elm solution.

i'm closing the PR then, thank you

rogeriochaves commented 7 years ago

Done, released an elm-only version of elm-testable for elm 0.18, check it out http://package.elm-lang.org/packages/rogeriochaves/elm-testable/latest