Testable Cmds - Proposal 2 - Changed Sub<'msg> to return an optional message asynchronously

[TimLariviere]

I like this one better than #309

The need stays the same. We want to be able to unit test Cmd.
While Cmd.none and Cmd.ofMsg are easy to test, it becomes more complex for Cmd.ofAsyncMsg (Fabulous hides the fact that it's asynchronous).
Cmd.ofMsgOption and Cmd.ofAsyncMsgOption are plain untestable because they may never dispatch anything, and we have no way to know when or if they will dispatch.

My proposal here is to change the way we handle Cmd.

Instead of Cmd being a list of functions Dispatch<'msg> -> unit (doesn't return anything right now, but may dispatch a message at a later time)

I changed it to a list of functions unit -> Async<'msg voption> (return a promise that at some point an optional message will need to be dispatched)

That way, it makes testing a lot more easier.

let triggerSomething flag = async {
     if flag = true then
        do! Async.Sleep 5000
        return (Some OtherMsg)
     else
        return None
}

let update msg model =
     match msg with
     | MyMessage flag -> model, Cmd.ofAsyncMsg (triggerSomething flag)

let executeAsync (cmd: Cmd<'msg>) =
     cmd |> List.map (fun sub -> sub ()) |> Async.Parallel

[<Test>]
let ``Given the message MyMessage with flag true, Something should be triggered``() = async {
     let initialModel = ...
     let expectedModel = ...

     let actualModel, actualCmd = App.update (MyMessage true) initialModel

     let! actualMsgOpt = executeAsync actualCmd

     actualModel |> should equal expectedModel
     actualMsgOpt |> should equal [ (ValueSome OtherMessage) ]
}

[<Test>]
let ``Given the message MyMessage with flag false, Something should not be triggered``() = async {
     let initialModel = ...
     let expectedModel = ...

     let actualModel, actualCmd = App.update (MyMessage true) initialModel

     let! actualMsgOption = executeAsync actualCmd

     actualModel |> should equal expectedModel
     actualMsgOption |> should equal [ ValueNone ]
}

[dsyme]

I think this is great - at least conceptually.

I've always found the "loss of the async modality" in the inner "dispatch" of Cmd implementation a little odd - but couldn't identify where/how that should be addressed or even why it was really a problem.

At one point I suggested making Cmd = AsyncSeq<msg> which is similar to the above (though allows for multiple messages), though that would lead to a dependency on the AsyncSeq library or an IAsyncEnumable equivalent.

My only question is about cases where a Cmd dispatches multiple messages over time. Are we now saying that this should never really happen, and that any Cmd dispatching multiple messages should mediate back via a model update message, kicking off a new command?

[ghost]

@TimLariviere I like the idea of changing Sub type to unit -> Async<'msg voption>. I read the commit and it feels like it is a little easier to reason about what a Cmd is now (maybe because I have been reading Fabulous code base for a while now :D).

Aside from solving the unit testing problem this approach is attractive because:

• Using Async makes code more concise, less callbacks to deal with
• Actually running async computations Async.StartImmediate was moved to the cmdDispatch function and Cmd.ofAsync is just building the computation now. The only downside that I can see is that now there is a little more overhead because the computation starts running only when Fabulous dispatches the Cmd, compared to immediately starting the computation in the "user space"

[TimLariviere]

Are we now saying that this should never really happen, and that any Cmd dispatching multiple messages should mediate back via a model update message, kicking off a new command?

Hmm, yes, haven't thought of that.
It's sad to lose this ability, and force people to mediate back to update. Makes things like progress reporting harder.

Maybe we should have 2 differents concepts?
Cmd would return message at a single time, and something else would be able to dispatch messages when it wants (like Cmd today) but put it aside.

If I take the example of progress report, the Cmd would kick off to background process and return a message to acknowledge that.
The background process would use the global dispatch to send messages independently of Cmd.

[TimLariviere]

At one point I suggested making Cmd = AsyncSeq<msg> which is similar to the above (though allows for multiple messages), though that would lead to a dependency on the AsyncSeq library or an IAsyncEnumable equivalent.

Is taking a dependency on AsyncSeq something we should avoid?

[TimLariviere]

Actually running async computations Async.StartImmediate was moved to the cmdDispatch function and Cmd.ofAsync is just building the computation now. The only downside that I can see is that now there is a little more overhead because the computation starts running only when Fabulous dispatches the Cmd, compared to immediately starting the computation in the "user space"

Actually it's not really different than today.
The Async.StartImmediate was called inside the wrapping fuction, so would only trigger when Fabulous tries to evaluate the Cmd.

let cmd = [ (fun dispatch -> async { let! msg = p in dispatch msg } |> Async.StartImmediate) ]

// Fabulous main loop
for sub in cmd do
     sub dispatch

It's only moved to another place. Cmd is no longer responsible to start the asynchronous task, dispatch is.

[cmeeren]

I may misunderstand something, but is Cmd really something that should generally be unit-tested? I realize that in some cases it works fine, e.g. Cmd.ofMsg. But in a general sense, a Cmd may perform IO (e.g. HTTP calls, as exemplified by the docs) before returning/dispatching, and in that case we're talking integration tests, not unit tests.

[TimLariviere]

@cmeeren No, I don't think we should unit test every Cmds, only those that are meaningful for your app.
One way I wanted to approach this problem is through an equivalent of OOP's mocking.

Maybe there's no need to execute the Cmd but instead just check the function that will be called.
Though I don't know how to do that in F#, and if it really is a good approach.

[aspnetde]

Just for the record: What I am missing right now is the opportunity to test that a specific command has been fired, not necessarily the command itself (that I can achieve otherwise, e.g. by testing another function that is then being called by the command).

I think this is quite important for use cases like

When the sign-in process is being started
- Then IsBusy is set to true
- The SignIn Command is being fired

tl;dr I want to be able to make sure that a certain type of command with specific parameters has been issued.

Or am I missing something and that should be possible already, even for Cmd.ofAsyncMsg etcetera?

[cmeeren]

If it's important to unit test that update triggers specific commands, without resorting to integration tests, wouldn't it be easier to take a detour by using some CmdMsg DU with cases for each command (with necessary arguments for each case)? The update function could accept some parameter of type CmdMsg -> Cmd<Msg> (where Msg is your main message type), that actually performs the conversion. You can then supply a custom CmdMsg -> Cmd<Msg> parameter to update when testing, so that you can easily verify that a command has actually been invoked.

The actual production runtime CmdMsg -> Cmd<Msg> function would just be a simple mapping from CmdMsg to the required Cmd.xx call returning a Cmd<Msg> and wouldn't itself be interesting to test.

Did that make sense?

Sorry if not; I'm just having a hard time imagining how to unit test commands in a sane way, since as far as I have used Elmish, the update function calls eg. Cmd.OfFunc.perform doActualSignInOverHttp ..., so I can't see how to test anything there without integration testing.

Maybe it's a lack of experience or knowledge, but to me, the Cmd stuff has always been sticking out as a sore thumb when it comes to the testability of Elm architectures. Everything seems so simple to test, except Cmd - since it's just a list of functions, that's hard to test without using a roundabout way like this (which TBH doesn't seem that bad to me, though I haven't used it yet), or doing partial application of every single function you want to use with Cmd and also want to test.

[cmeeren]

Example:

module Http =

  let signIn (username, password) : Async<Result<Token, SignInError>> =
    async {
      use client = HttpClient ()
      ...
    }

type Msg =
  | SignIn of Username * Password
  | SignInComplete of Result<Token, SignInError>

[<RequireQualifiedAccess>]
type CmdMsg =
  | SignIn of Username * Password

let cmdMsgToCmd : CmdMsg -> Cmd<Msg> = function
  | CmdMsg.SignIn (u, p) -> Cmd.OfAsync.perform Http.signIn (u, p) SignInComplete

let update (toCmd: CmdMsg -> Cmd<Msg>) msg model =
  match msg with
  | SignIn (u, p) -> { model with IsBusy = true }, toCmd (CmdMsg.SignIn (u, p))
  | SignedIn res -> ...

[TimLariviere]

What I am missing right now is the opportunity to test that a specific command has been fired, not necessarily the command itself

@aspnetde Yes, that's what I initially wanted to do. Being able to check if the non-executed Cmd is the same as the one I expect it to be.
But it's really difficult, if not impossible, to do such thing without changing the architecture or applying a decoupling pattern that can be more verbose and harder to implement/use.

@cmeeren's example could be a good way to solve this issue gracefully (but is necessarily more verbose).
Though I think it would be better to split the update function in 2 like this:

let cmdMsgToCmd : CmdMsg -> Cmd<Msg> = function
    | CmdMsg.SignIn (u, p) -> Cmd.OfAsync.perform Http.signIn (u, p) SignInComplete

let updateTestable msg model =
    match msg with
    | SignIn (u, p) -> { model with IsBusy = true }, CmdMsg.SignIn (u,p)
    | SignedIn res -> ...

let update msg model =
     let newModel, cmdMsg = updateTestable msg model
     let cmd = cmdMsgToCmd cmdMsg
     newModel, cmd

Because otherwise you wouldn't be able to test the CmdMsg in a unit test, as the CmdMsg would be instantly converted to Cmd.

Going further, we could follow the same logic and apply the same pattern as Msg.
Make all update: 'msg -> 'model -> ('model * 'cmdMsg) and in Program.mkSimple supply a conversion function 'cmdMsg -> Cmd<'msg>
This would support the same composition pattern of Msg when dealing with multiple pages.

Without changing Fabulous, this would theoretically be possible

// Page A
type PageAMsg = | SignedIn | ...
type PageACmd = | SignedInCmd of data | ...

let convertCmdMsg cmd =
     match cmd with
     | SignedInCmd data -> Cmd.ofAsync ...

let update msg model =
     match msg with
     | SignedIn = { model with IsBusy = true }, SignedInCmd someData

// App
type AppMsg = | PageA of PageAMsg | PageB of PageBMsg
type AppCmdMsg = | PageA of PageACmd | PageB of PageBCmd

let convertCmdMsg cmd =
     match cmd with
     | PageACmd cmdMsg-> PageA.convertCmdMsg cmdMsg |> Cmd.map AppMsg
     | ...

let update msg model =
     match msg with
     | PageA msg ->
         let pageAModel, pageACmd = PageA.update msg model.PageAModel
         { model with PageA = pageAModel }, (PageA pageACmd)
     | ...

type App() =
     let convertedUpdate msg model =
          let newModel, newCmdMsg = App.update msg model
          let cmd = App.convertCmdMsg newCmdMsg
          (newModel, cmd)

     Program.mkSimple App.init convertedUpdate App.view
     |> Program.run...

[aspnetde]

It seems this is a common issue. I tend to like your propsal @TimLariviere:

• Con: It's clearly introducing new "cognitive load".
• Pro: On the other hand having a central DU that must contain all cases that may trigger a command could also be seen as helpful as it would improve the ability to reason about what's going on. Right now we can sprinkle commands right all over the place.

[cmeeren]

Because otherwise you wouldn't be able to test the CmdMsg in a unit test, as the CmdMsg would be instantly converted to Cmd.

I was thinking that you passed in a function that, when receiving the expected CmdMsg, caused the test to pass. But I agree that your approach is a lot better since no mocking is needed and you can simply compare output data.

Going further, we could follow the same logic and apply the same pattern as Msg.
Make all update: 'msg -> 'model -> ('model * 'cmdMsg)

It'll either have to be 'cmdMsg option, or everyone's CmdMsg will have to have a no-op case.

Without changing Fabulous, this would theoretically be possible

Yes, and I think this it's a good idea to keep the existing abstractions (the architecture is well-known and well-documented with lots of resources available on the net), and not force this on users. The CmdMsg pattern could be clearly documented as a convenient way to have testable Cmds. The convertedUpdate function is only 3 lines, trivial to implement (it would be documented anyway), and simple to understand.

The CmdMsg approach is a pattern that's simple to achieve using the current architecture; let's not make it a mandatory part of the architecture.

(This also allows the users to choose whether to use CmdMsg option or have a no-op case; I can imagine different people leaning different ways here.)

[forki]

since @aspnetde asked me on twitter. Here is a ugly version that works with elmish:

[cmeeren]

@forki I don't know what the twitter conversation was specifically about, but that only seems to allow you to verify that a resulting message was returned (and only after the function has run, i.e. integration testing Cmds). There are however valid use-cases for Cmds that do not result in messages.

[forki]

@cmeeren sure but if that's soo super important to test, then you can introduce a new message. and in the pattern match of then update function you make that a oneliner to redispatch

[cmeeren]

Haha, yes, I was just writing about that when your comment popped up, though I see it from another angle: I can't help but feeling like this whole CmdMsg stuff is somehow duplicating the existing Msg and update parts of the architecture. As you say, you can drop CmdMsg and instead have all the CmdMsg cases as dedicated Msg cases, and the update function only handles these by returning the previous model and actually performing the command.

However, the CmdMsg approach is IMHO still far easier to test (and I'd argue simpler to understand) since the testable update function is only returning data, not functions. I could easily imagine the added complexity being worth it in many cases.

---

By the way, here's another reason to not force a particular design on users: The general case would have to be CmdMsg list, not CmdMsg option. But that's a complication if you don't really need it.

[cmeeren]

Heck, you could even have a CmdMsg case that carries a Cmd<Msg> as data, which you can use for anything you don't actually want/need to test.

[forki]

the actual point I'm trying to make: simplify the update function to pattern match that basically only has oneliners for each case. at that point I would not test that level anymore

[aspnetde]

However, the CmdMsg approach is IMHO still far easier to test (and I'd argue simpler to understand) since the testable update function is only returning data, not functions.

If I am not mistaken right now, I think it's not much of a difference, is it? Having a CommandMessage type or simply having command cases in the normal Message type is effectively the same thing. And therefore equally testable.

However, I would prefer to have a dedicated CommandMessage in the proposed way (as optional feature not being enabled by default so simplicity of MVU is kept), as I think it makes things more clear in the long run if they are separated that way.

[cmeeren]

If I am not mistaken right now, I think it's not much of a difference, is it? Having a CommandMessage type or simply having command cases in the normal Message type is effectively the same thing. And therefore equally testable.

If you don't have a CmdMsg but use Msg for this, then the update function is still returning a Cmd which you have to run to (potentially) get one or more messages out. It's more direct (and safer, since you have no guarantees what a Cmd could do) to just return the CmdMsg.

Behaviour as data - part of what I love about FP. 😊

[aspnetde]

If you don't have a CmdMsg but use Msg for this, then the update function is still returning a Cmd which you have to run to (potentially) get a message out. It's more direct (and safer, since you have no idea what a Cmd could do) to just return the CmdMsg.

I've got this:

View.Button(
    text = "Sign in", 
    isVisible = (not model.IsBusy),
    command = (fun () -> dispatch LoginStarted),
    canExecute = model.Login.IsCredentialsProvided)

| LoginStarted ->
    let credentials = { UserName = model.Login.UserName; Password = model.Login.Password }
    { model with IsBusy = true }, logInCmd credentials

Now I want to make sure that not only logInCmd is being issued but the correct credentials are being passed as argument. That's what I test all day long with my C# stuff.

If I understand @forki he proposes something like:

type Msg =
| LoginStarted
// Commands
| CmdLoginStarted of Credentials

match msg with
| LoginStarted ->
    let credentials = { UserName = "Foo"; Password = "Bar" }
    // model, CmdLoginStarted(credentials) <- Edit: That's plain nonsense
    model, Cmd.ofMsg (CmdLoginStarted(credentials)) 
| CmdLoginStarted credentials -> model, logInCmd credentials

So I could make sure CmdLoginStarted is being returned having the right credentials in its payload. And CmdLoginStarted does effectively what convertCmdMsg as proposed before would do. This is where both approaches would end as testability is hard here anyway.

Right?

[cmeeren]

match msg with
| LoginStarted ->
    let credentials = { UserName = "Foo"; Password = "Bar" }
    model, CmdLoginStarted(credentials) 
| CmdLoginStarted credentials -> model, logInCmd credentials

I don't get this. The first case is returning Model * Msg. The second one returns (I assume) Model * Cmd<Msg>.

This is what you avoid with CmdMsg. Then all cases return Model * CmdMsg (or CmdMsg option or CmdMsg list.)

[aspnetde]

Sorry, that was too quick and dirty:

match msg with
| LoginStarted ->
    let credentials = { UserName = "Foo"; Password = "Bar" }
    model, Cmd.ofMsg (CmdLoginStarted(credentials)) 
| CmdLoginStarted credentials -> model, logInCmd credentials

[cmeeren]

Yes, and my point is that when testing LoginStarted, you have a model (easy to test), and a Cmd which is a list of functions that you must invoke, and you have no (compile-time) guarantees that invoking the functions does not perform side-effects that are unwanted when testing. You are depending on developer discipline to make sure that all Cmds you want to test are created using Cmd.ofMsg. With the CmdMsg approach, the compiler helps you since you only return data.

[aspnetde]

You are depending on developer discipline to make sure that all Cmds you want to test are created using Cmd.ofMsg. With the CmdMsg approach, the compiler helps you since you only return data.

I agree. But you will end up needing some kind of agreement on how to do it in a particular application within a team (or with your future self) anyway, as no one will stop you to do it the "wrong" way. So developer discipline is needed anyway.

That being said I can only repeat myself at this point that I would be happy to see the CmdMsg approach being added. I would definitely use it.

[aspnetde]

For now this works for me:

type ``When a login is being started``() =            
    [<Fact>]
    let ``The Log in Command is being issued and the Credentials are being passed``() =
        let userName = Guid.NewGuid().ToString()
        let password = Guid.NewGuid().ToString()
        
        let model = initialModel()
        let model = { model with Login = { model.Login with UserName = userName; Password = password } }
            
        let _, cmd = App.update App.Msg.LoginStarted model
        
        let credentials = { UserName = userName; Password = password }
        
        cmd
        |> dispatchesMessage (App.Msg.CmdLoginStarted(credentials))
        |> should be True

Key is dispatchesMessage, a small helper function based on @forki's sample above:

let dispatchesMessage (msg:App.Msg) (cmd:Cmd<App.Msg>) =
    let messages = ResizeArray<_>()
    cmd |> Seq.iter (fun f -> f(fun x -> messages.Add x))
    messages |> Seq.exists (fun m -> m = msg)

[cmeeren]

I agree. But you will end up needing some kind of agreement on how to do it in a particular application within a team (or with your future self) anyway, as no one will stop you to do it the "wrong" way. So developer discipline is needed anyway.

I'm not sure I understand. As long as one agrees to have the update function be Msg -> Model -> Model * CmdMsg instead of Msg -> Model -> Model * Cmd<Msg>, which is an up-front discussion, then the compiler will enforce that everyone does that. There is no way to return Cmd<Msg>. It requires team buy-in, sure, but no discipline in remembering to only return Cmd.ofMsg commands.

In any case, it seems we agree. :)

[aspnetde]

I'm not sure I understand. As long as one agrees to have the update function be Msg -> Model -> Model * CmdMsg instead of Msg -> Model -> Model * Cmd, which is an up-front discussion, then the compiler will enforce that everyone does that. There is no way to return Cmd. It requires team buy-in, sure, but no discipline in remembering to only return Cmd.ofMsg commands.

Assuming you have only one single program (remember the scaling discussion ;)), you are absolutely right. And the longer I think about it, the more I like it (the approach discussed here).

👍

[TimLariviere]

I'm quite satisfied with @cmeeren's CmdMsg and returning Model * CmdMsg option instead of Model * Cmd<msg>.

The cons are fairly acceptable (it's the sames as Msg, which is widely accepted).
And the pros are good:

• No change in Fabulous (no breaking changes, nor Fabulous' specifics)
• Not forced on the developers
• Simple pattern to follow (should be easy to migrate one way or another if we want so)
• Clearer intents in update

So I'm inclined to finally close this PR and open a new one with only docs and samples (no need to change Fabulous)

If you want to continue your discussion, feel free to open an issue. :)

[aspnetde]

I'm fine with it :)

[cmeeren]

I'm quite satisfied with @cmeeren's CmdMsg and returning Model * CmdMsg option instead of Model * Cmd<msg>.

Great! But the general case is returning CmdMsg list; you might want to document that. (CmdMsg option is similar to a CmdMsg.NoOp case, but there is no way to trigger multiple commands unless you can return CmdMsg list. Except for having a CmdMsg case that accepts a list of CmdMsg and also making the cmdMsgToCmd function recursive, but that's unnecessarily complex when you can just return CmdMsg list.)