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

`Query.contains` gives unexpected output when using event handlers

Open rtfeldman opened this issue 7 years ago • 3 comments

Originally reported in https://github.com/eeue56/elm-html-test/issues/37

Suppose we write this test code. It should fail because it's requesting an onInput handler that isn't there:

import Html exposing (div, li, text, ul)
import Html.Attributes exposing (class)
import Html.Events exposing (onInput)
import Test exposing (Test, test)
import Test.Html.Query as Query


fails : Test
fails =
    test "The list has two li: one with the text \"third item\" and another one with \"first item\"" <|
        \() ->
            div []
                [ ul [ class "items active" ]
                    [ li [] [ text "first item" ]
                    , li [] [ text "second item" ]
                    , li [] [ text "third item" ]
                    ]
                ]
                |> Query.fromHtml
                |> Query.contains
                    [ li [ onInput (always ()) ] [ text "third item" ]
                    , li [] [ text "first item" ]
                    ]

Here's the error we get:

    ▼ Query.fromHtml

        <div>
            <ul class="items active">
                <li>
                    first item
                </li>
                <li>
                    second item
                </li>
                <li>
                    third item
                </li>
            </ul>
        </div>


    ▼ Query.contains

    	✗ /1\ missing descendants:

        1)    <li>
                  third item
              </li>

This error message is surprising because it says it couldn't find the given descendant, even though it was there.

I can think of a few ways of resolving this:

  1. Make Query.contains take List (Html Never) instead of List (Html msg). This would be less surprising, but would also make the function nearly useless in practice. We'd also still have to explain that an event handler was involved when giving an error that the output was different. This doesn't seem like a good option.
  2. Make Query.contains do a referential equality check on event handlers, and succeed if that succeeds, but fail otherwise. This would make it Just Work in quite a few cases, but in the cases where it failed—e.g. because you switched your implementation from onInput SetText to onInput (String.toLower >> SetText)—it would be really confusing to explain why it failed, and how you'd need to adjust (read: rewrite) your test not to run afoul of that. Probably in this world it would become best practice to recommend using plain queries instead of Query.contains.
  3. Make Query.contains silently ignore event handlers - both in the Html you give it as well as in the Html it checks. Even if we put this in the documentation, it's not intuitive that it would consider some values of type Html.Attribute and silently ignore others, so this will likely lead to some surprising outcomes. It's also not hard to imagine that in this world it would also become best practice to recommend using plain queries instead of Query.contains.
  4. Remove Query.contains from the API. Maybe it is a footgun that seems like it will save your team time, but is actually likely to end up costing your team time overall due to its surprising behavior around event handlers. Maybe it's better to have the only way to describe DOM structures be to use queries that were designed to be good at that.

Removing Query.contains from the API is the option that appeals to me most. In retrospect, think it was probably a mistake for me to have added it in in the first place.

rtfeldman avatar Feb 03 '18 20:02 rtfeldman

I'd prefer 1. Why would it make it less valuable? If you're looking for a particular element, being able to create <li>Something</li> and search for that is very handy. I'd actually like some kind of fuzzy matching along these lines, like:

someView 
  |> contains a li with the text "Something"

where <li>Something</li> and <li data-blah="fish">Something</li> both match

eeue56 avatar Feb 17 '18 00:02 eeue56

@eeue56 Fortunately, that use case can be covered by containing! (PR: https://github.com/eeue56/elm-html-test/pull/60)

Query.findAll
    [ tag "li"
    , containing [ text "Something" ]
    ]

containing also covers the use case of #53, so I think it makes contains obsolete!

Thoughts?

rtfeldman avatar Mar 08 '18 23:03 rtfeldman

containing also covers the use case of #53, so I think it makes contains obsolete!

Yes! I think in general it would be nice to have less query functions and more selectors.

stoeffel avatar Mar 09 '18 08:03 stoeffel