Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add examples for the new CmdMsg pattern allowing unit testability of Cmds #418

Merged
merged 6 commits into from
May 24, 2019

Conversation

TimLariviere
Copy link
Member

@TimLariviere TimLariviere commented May 22, 2019

Closes #402 (this PR includes the tryFindViewElement helper function)

After much discussion, we came to the conclusion that a simple pattern can allow complete unit testing when using Cmd<'msg> despite it not being unit testable.
(See full discussion in #320 and #309)

This pattern is completely up to the developer, nothing has been changed in Fabulous to support it.

The fact is that Cmd<'msg> is not unit testable because it's essentially just an array of functions.
Functions returning Cmds won't be fully eligible for unit testing, that include init and update, which would otherwise be perfect for unit testing...

The idea of this pattern is to remove the dependency to Cmd<'msg> from init and update by using an intermediate CmdMsg discriminated union.
This union will only state intents instead of actually executing a Cmd.

Updated with Program.mkProgramWithCmdMsg

type Msg =
    | FieldUpdated of string
    | FieldValidated of bool

type Model =
    { Text: string
      IsValidated: bool }

// The new discriminated union replacing direct usage of Cmd<'msg>
type CmdMsg =
    | ValidateField of string

// Some long running validation process that will eventually dispatch a Msg.FieldValidated
let validateFieldAsync value = async { ... }

/// unit -> Model * CmdMsg list
let init () =
    { Text = ""; IsValidated = false }, [] // Cmd.none is replaced by an empty list of CmdMsg

/// Msg -> Model -> Model * CmdMsg list
let update msg model =
    match msg with
    | FieldUpdated value -> { model with Text = value }, [ (ValidateField value) ] // Ask to validate the field value - equivalent of Cmd.ofAsync (validateFieldAsync  value)
    | FieldValidated validation -> { model with IsValidated = validation }, []

let view model dispatch = (...)

// A new function that will map a CmdMsg to the actual Cmd to execute
let mapCommands cmdMsg =
    match cmdMsg with
    | ValidateField value -> Cmd.ofAsync (validateFieldAsync value)

// Finally starts the program
type App() =
    Program.mkProgramWithCmdMsg init update view mapCommands
    |> Program.run...

init and update now have a full data-only return type.
Greatly simplifying unit testing.

[<Test>]
let updatingFieldShouldUpdateModelAndTriggerValidation () =
    let initialModel = { Text = "Previous Text"; IsValidated = false }
    let expectedReturn = { Text = "New Text"; IsValidated = false }, [ (ValidateField "New Text") ]
    App.update (FieldUpdated "New Text") initialModel |> should equal expectedReturn

@TimLariviere
Copy link
Member Author

TimLariviere commented May 23, 2019

Interestingly enough, this also enables code reuse between Fabulous and Elmish.
init and update 100% not specific to Fabulous.

@TimLariviere
Copy link
Member Author

@cmeeren & @aspnetde : Don't hesitate to give your opinion on this PR

@cmeeren
Copy link

cmeeren commented May 23, 2019

Looks good! You might want to consider including helper functions that accept a mapCommands function and an update function and returns a convertedUpdate function, and similar for init. That way, users won't have to implement the (allegedly trivial) mapping themselves, and it's possible to simply inline it into the Program.mk... call.

Again though, it's so trivial that it's not really needed. Just a courtesy.

@aspnetde
Copy link

I'm fine with it, too.

(Side note: In my small sample app I ended up with a similar approach like the one used in the ElmishContacts sample, having pages with Msg + ExternalMsg to communicate with their parents. I also had one top level command to show alerts. Implementing this approach shown here would have meant to have Msg + ExternalMsg + CmdMsg + ExternalCmdMsg. That had been a bit too much so I kept going with Msg.Foo and Msg.CmdFoo.)

@TimLariviere
Copy link
Member Author

TimLariviere commented May 23, 2019

I also had one top level command to show alerts. Implementing this approach shown here would have meant to have Msg + ExternalMsg + CmdMsg + ExternalCmdMsg.

@aspnetde That's interesting.
What are exactly your alerts? Are they alert dialogs or something else?

I'm trying to imagine when it would make sense to introduce the concept of ExternalCmdMsg.
Personally I would only use ExternalMsg to handle all external things. Like navigation or triggering commands on other pages.
This way there's no need to an ExternalCmdMsg. CmdMsg stays specific to its page.

type Msg =
    | SomeMsg

type ExternalMsg =
    | TriggerCmdOnOtherPage

type CmdMsg =
    | TriggerCmdOnThisPage

let update msg model =
    match msg with
    | SomeMsg -> model, (Some TriggerCmdOnOtherPage), [ TriggerCmdOnThisPage ]


/// The other page
type Msg =
    | PageATriggeredCmd

type CmdMsg =
    | TriggerCmdOnThisPage

let update msg model =
    match msg with
    | PageATriggeredCmd -> model, None, [ TriggerCmdOnThisPage ]

@aspnetde
Copy link

For example I had a small function I didn't want to duplicate x times across the app. Like:

let private showErrorAlert errorMessage = async {
    do! Application.Current.MainPage
            .DisplayAlert("Error", errorMessage, "Okay") |> Async.AwaitTask
    return None
}

I guess that one could also be moved in some kind of helper module. But it's not hard to imagine a scenario where one would return a specific message type etcetera.

Anyway: No showstopper here!

childElements
|> Seq.map (fun e -> e |> tryFindViewElement automationId)
|> Seq.filter (fun e -> e.IsSome)
|> Seq.map (fun e -> e.Value)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this mapping useful? You are only interested in the first element after all. So you could take the first and return the value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm doing this 2nd map here because the type returned by Seq.filter is Seq<ViewElement option>, so Seq.tryHead will return a ViewElement option option.

Without this map, I would need to match on the ViewElement option option and in the case tryHead found something, convert the ViewElement option to ViewElement.
I think it's faster to read and write the way I did.

Also the advantage of Seq here is that it won't evaluate the whole collection (like IEnumerable and LINQ in C#), it'll try each item one by one until it finds a matching one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works like this then I didn't say anything. Was just curious about it because if it would work like LINQ it would be bad for performance.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use Seq.choose here instead of Seq.map and Seq.filter, which would auto-unwrap the option for you and reduce the option nesting.

@cmeeren
Copy link

cmeeren commented May 24, 2019

Excellent explanation in 922fa90. Might borrow from that (and the rest of this PR) for Elmish.WPF.

I suggest that you explicitly state that mkProgramWithCmdMsg does not do anything "magic" and that the user would get the same result by writing trivial code to convert update and init themselves. IMHO that could help drive the point home that this is indeed just a general pattern where Fabulous just supplies a trivial helper out of courtesy.

@cmeeren
Copy link

cmeeren commented May 24, 2019

Wonderful! 👍

@TimLariviere TimLariviere changed the title [WIP] Add examples for the new CmdMsg pattern allowing unit testability of Cmds Add examples for the new CmdMsg pattern allowing unit testability of Cmds May 24, 2019
@TimLariviere TimLariviere marked this pull request as ready for review May 24, 2019 13:56
@TimLariviere
Copy link
Member Author

/azp help

@azure-pipelines
Copy link

Supported commands
     help:
          Get descriptions, examples and documentation about supported commands
          Example: help "command_name"
     list:
          List all pipelines for this repository using a comment.
          Example: "list"
     run:
          Run all pipelines or a specific pipeline for this repository using a comment. Use
          this command by itself to trigger all related pipelines, or specify a pipeline
          to run.
          Example: "run" or "run pipeline_name"
     where:
          Report back the Azure DevOps orgs that are related to this repository and org
          Example: "where"

See additional documentation.

@TimLariviere
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:
PR Build
Full Build

@TimLariviere
Copy link
Member Author

/azp run pr build

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TimLariviere TimLariviere merged commit 712bc86 into fabulous-dev:master May 24, 2019
@TimLariviere TimLariviere deleted the cmdmsg branch May 24, 2019 21:10
This was referenced May 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: A more robust way to unit test views
5 participants