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

Allow to trigger events on the view, update the app and make assertions on the view #9

Merged
merged 16 commits into from Jun 18, 2017

Conversation

rogeriochaves
Copy link

@rogeriochaves rogeriochaves commented May 4, 2017

This can only work after the PR #18 on elm-html-test is merged. EDIT: merged.

So, my goal with this is being able to test the views in a fully integrated way, since now we can get events from elm-html-test, we can use those events to trigger updates on elm-testable and assert things.

For the first iteration, I've just created a function where you have to query the html and call eventResult to return the Msg result from the node, this Msg is then passed to the update function, the API #1 looks like this:

test "triggers events" <|
 \() ->
     htmlProgram
         |> TestContext.updateWith
             (Query.find [ Selector.tag "button" ]
                 >> Events.eventResult Events.Click
             )
         |> TestContext.expectView
         |> Query.has [ Selector.tag "p" ]

This works fine, but it is kinda ugly, and I wanted to write something more natural, so instead of updateWith, I've created functions like query and queryAll that accepts a Query type from elm-html-test, the API #2 looks like this:

import Test.View exposing (..)

test "query for multiple nodes" <|
    \() ->
        htmlProgram
            |> query (find [ Selector.class "first-button" ])
            |> trigger Events.Click
            |> queryToAll (findAll [ Selector.tag "p" ])
            |> expectViewAll (count (Expect.equal 1))
, test "triggers multiple events" <|
    \() ->
        htmlProgram
            |> query (find [ Selector.class "first-button" ])
            |> trigger Events.Click
            |> query (find [ Selector.class "second-button" ])
            |> trigger Events.Click
            |> queryToAll (findAll [ Selector.tag "p" ])
            |> expectViewAll (count (Expect.equal 2))

So, this works, and at the first implementation I've made it in a way that it throws an error if something that was expecting a Multiple query sends a Single query and vice-versa. But on the next commits I've changed the types to tell that you are using a wrong function on compilation time rather than runtime.

But, this is still ugly, and queryToAll and expectViewAll are the worst names imo. So, I've decided to wrap the elm-html-test changed to the API #3:

import Test.View exposing (..)

test "triggers events" <|
    \() ->
        htmlProgram
            |> find [ Selector.class "first-button" ]
            |> trigger Events.Click
            |> has [ Selector.tag "p" ]
            |> count (Expect.equal 1)
, test "triggers multiple events" <|
    \() ->
        htmlProgram
            |> find [ Selector.class "first-button" ]
            |> trigger Events.Click
            |> find [ Selector.class "second-button" ]
            |> trigger Events.Click
            |> findAll [ Selector.tag "p" ]
            |> count (Expect.equal 2)

I loved it, it is totally readable. The downside if having to wrap all elm-html-test query functions (which are not very much).

The other downside/maybe unexpected behaviour, which also happens on API #2, is that after calling trigger the query is reseted, because when update is called it has to update the view, and the view lives inside elm-html-test queries to, so we have to reset it.

For example, on the last test of last example, after clicking the first-button the query is reseted and because of this I could query the second-button, which is not inside first-button as one might think.
At least personally this is not a problem, still think it reads very well, the "trigger" really does look like a break.

So, what you think?

@rogeriochaves
Copy link
Author

rogeriochaves commented May 7, 2017

The last commit can only be used if elm-html-in-elm PR #8 gets merged, if not, we can revert it

@avh4
Copy link
Owner

avh4 commented May 15, 2017

Great! (Will wait for eeue56/elm-html-in-elm#8 to get merged.)

I think the fully-wrapped Html.Query API is too heavy for elm-testable. I think I'd like to stick with

simulate : List Selector -> Event -> TestContext model msg -> TestContext model msg
expectView : TestContext model msg -> Html.Query.Single

I do think a cleaner API like you've proposed could be valuable, but I think that'd be better as a separate package that builds on elm-testable, and that elm-testable itself should strive for a small API rather than the most concise one.

@eeue56
Copy link

eeue56 commented May 15, 2017

FYI I've merged all those PRs and published everything

@rogeriochaves
Copy link
Author

@avh4 actually, for the simulate function I couldn't simply accept a List Selector because you may want to navigate deeper than that to trigger an event (find, children, etc).

So it end up like this:

simulate : (Test.Html.Query.Single msg -> Test.Html.Query.Single msg) -> Event -> TestContext model msg -> TestContext model msg
expectView : TestContext model msg -> Test.Html.Query.Single msg

The usage actually ended up very neat! I think I really liked more this approach:

htmlProgram
    |> TestContext.simulate (find [ tag "button" ]) Click
    |> TestContext.expectView
    |> Query.has [ tag "p" ]

But in case I change my mind, the other suggestions are in the revert commits, I might create another lib on top of elm-testable as you said later :)

@rogeriochaves
Copy link
Author

Should we also change expectView signature so we can use it in a similar way to simulate? 🤔

htmlProgram
    |> TestContext.simulate (find [ tag "button" ]) Click
    |> TestContext.expectView (has [ tag "p" ])

@avh4
Copy link
Owner

avh4 commented May 16, 2017

Ah, cool!

I think leaving expectView as-is is good for now. I originally tried having it take a function like that, and it turned out to be ugly if you needed to do a more complicated expectation or if you wanted to do a few nested finds.

@avh4 avh4 self-assigned this May 16, 2017
@avh4
Copy link
Owner

avh4 commented May 16, 2017

I think this looks good overall!

The only thing missing is some test coverage for the error cases in tests/ViewTests.elm. The three I can think of are:

  • when the query doesn't find an element
  • when the query finds multiple elements
  • when the found element doesn't handle the specified event

@avh4 avh4 removed their assignment May 16, 2017
@rogeriochaves
Copy link
Author

rogeriochaves commented May 16, 2017

@avh4 done! And thanks for that, indeed it was swallowing errors for not found events and elements.

If the query finds multiple elements the error is produced by elm-html-test the same way of when it finds zero elements, so I didn't include this and other extra cases cause I think we don't need to re-cover all the elm-html-test cases.

Also one nice thing is that the type system doesn't allow the user to simulate an event on a findAll for example.

\() ->
htmlProgram
|> TestContext.simulate (Query.find [ Selector.tag "button" ]) Events.DoubleClick
|> expectError "Failed to decode string"
Copy link
Owner

Choose a reason for hiding this comment

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

"Failed to decode string" seems like a bad error message. Does a change to this message need to be made in elm-html-test?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is due to this line: https://github.com/rogeriochaves/elm-html-test/blame/master/src/Test/Html/Events.elm#L123

I don't know why it is there, I'll make a PR to remove it, the error message is much better without it: "The event dblclick does not exist on the found node"

@avh4
Copy link
Owner

avh4 commented May 30, 2017

@rogeriochaves Thanks! I'm happy with this :D

I'm going to wait to merge until #9 (comment) is fixed in elm-html-test.

Also, FYI there are going to be some merge conflicts here, since I'm upgrading elm-format, and elm-test. But don't worry about those; I will fix them.

@rogeriochaves
Copy link
Author

@avh4 done, it was not hard to fix the conflicts, telling git to ignore whitespaces during merge made things very easy.

Expect.fail "TestContext should have an error an it doesn't"

TestContextInternal.TestError { error } ->
Expect.equal True (String.contains expectedError error)
Copy link
Author

Choose a reason for hiding this comment

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

doing this way to make tests less brittle to changes in error messages

@rogeriochaves
Copy link
Author

@avh4 can we merge this?

@rogeriochaves
Copy link
Author

@avh4 ping

@avh4 avh4 merged commit fa48805 into avh4:rewrite-native Jun 18, 2017
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.

None yet

3 participants