eeue56 / elm-html-test

Test elm-html in Elm!
http://package.elm-lang.org/packages/eeue56/elm-html-test/latest
BSD 3-Clause "New" or "Revised" License
67 stars 15 forks source link

Query.has matches children, not just root element #41

Open SamGerber-zz opened 7 years ago

SamGerber-zz commented 7 years ago

Query.has is documented as follows:

Expect the element to match all of the given selectors. http://package.elm-lang.org/packages/eeue56/elm-html-test/5.1.0/Test-Html-Query#has

Here's the example given:

    test "The list has both the classes 'items' and 'active'" <|
        \() ->
            div []
                [ ul [ class "items active" ]
                    [ li [] [ text "first item" ]
                    , li [] [ text "second item" ]
                    , li [] [ text "third item" ]
                    ]
                ]
                |> Query.fromHtml
                |> Query.find [ tag "ul" ]
                |> Query.has [ tag "ul", classes [ "items", "active" ] ]

https://github.com/eeue56/elm-html-test/blob/5.1.0/src/Test/Html/Query.elm#L394-L405

Based on this example, it sounds like it's making assertions against the node itself, but in practice, it looks like it traverses all of the node's children. Is this expected behavior? If so, perhaps the docs could be clearer.

Here's an example test that I'm not sure should fail or pass (it does pass currently):

        test "Query.has doesn't consider children" <|
            \() ->
                div []
                    [ div []
                        [ span [ class "nested-span" ]
                            [ text "hello" ]
                        ]
                    ]
                    |> Query.fromHtml
                    |> Query.has [ Selector.class "nested-span" ]

@stoeffel and I ran up against this earlier, as this behavior causes the test used as documentation for Query.children to be a false positive. Children was returning the ul, and the test was still passing because there were li elements within the ul:

test "The <ul> only has <li> children" <|
    \() ->
        div []
            [ ul [ class "items active" ]
                [ li [] [ text "first item" ]
                , li [] [ text "second item" ]
                , li [] [ text "third item" ]
                ]
            ]
            |> Query.fromHtml
            |> Query.find [ tag "ul" ]
            |> Query.children []
            |> Query.each (Query.has [ tag "li" ])

http://package.elm-lang.org/packages/eeue56/elm-html-test/5.1.0/Test-Html-Query#children

Also, it's not clear whether it enforces that a single element must match all the selectors, as this version of the example also passes:

    test "The list has both the classes 'items' and 'active'" <|
        \() ->
            div []
                [ ul []
                    [ li [] [ text "first item" ]
                    , li [] [ text "second item" ]
                    , li [ class "items active" ] [ text "third item" ]
                    ]
                ]
                |> Query.fromHtml
                |> Query.find [ tag "ul" ]
                |> Query.has [ tag "ul", classes [ "items", "active" ] ]
eeue56 commented 6 years ago

So I think we should probably add is and isNot, which match the root node. Then has and hasNot are for children only. What do we think? @rtfeldman

rtfeldman commented 6 years ago

Based on this example, it sounds like it's making assertions against the node itself, but in practice, it looks like it traverses all of the node's children. Is this expected behavior?

I think this is unexpected, and a bug.

I think it'll be nicest if it works like this:

  1. Make it so that has only applies to the root node.
  2. If you explicitly want to involve children or descendants, do a Query.children/Query.find/Query.findAll to get them, and then use Query.each (Query.has [ ... ])

This keeps the API the same size as it is today, and supports either behavior.

This would be a breaking bugfix though. I think it might be too surprising for a patch release.

Here's what I think we should do:

  1. Change has and hasNot to work this way.
  2. Clarify the documentation so it's more apparent this is how it works.
  3. Wait until the next major release to publish the change.

Thoughts?