elm-html-test icon indicating copy to clipboard operation
elm-html-test copied to clipboard

Query.has matches children, not just root element

Open SamGerber-zz opened this issue 7 years ago • 2 comments

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" ] ]

SamGerber-zz avatar Aug 23 '17 23:08 SamGerber-zz

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

eeue56 avatar Jan 21 '18 22:01 eeue56

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?

rtfeldman avatar Jan 21 '18 22:01 rtfeldman