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.

Sign up for GitHub

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

Use Query instead of Selector #1

Open
wants to merge 2 commits into
base: NS-class-selectors
Choose a base branch
from
Open

Use Query instead of Selector #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d287917
Use Query instead of Selector
tesk9 Jan 20, 2022
fdbbd1e
Generalize tests
tesk9 Jan 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 4 additions & 32 deletions src/Test/Html/Internal/ElmHtml/Query.elm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,7 @@ import Test.Html.Internal.ElmHtml.InternalTypes exposing (..)
type Selector
= Id String
| ClassName String
| ClassNameNS String
| ClassList (List String)
| ClassListNS (List String)
| Tag String
| Attribute String String
| BoolAttribute String Bool
Expand Down Expand Up @@ -241,22 +239,12 @@ hasBoolAttribute attribute value facts =

hasClass : String -> Facts msg -> Bool
hasClass queryString facts =
List.member queryString (classnames facts)


hasClassNS : String -> Facts msg -> Bool
hasClassNS queryString facts =
List.member queryString (classnamesNS facts)
List.member queryString (classnames facts ++ classes facts)
Copy link
Owner

@Confidenceman02 Confidenceman02 Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I both love this, and have questions.

❤️ I love how clever and cleaner it is to my implementation. Awesome job!

🤔 We are touching how hasClass works and this might break peoples tests in weird and wonderful ways. Might using a more verbose svg specific abstraction that leaves current code paths intact be safer?

🤔 hasExactClassName will mysteriously not work for svgs but work for html. Might that cause me confusion and make it difficult to reason about my test considering hasClass and hasClasses work fine for both html and svg?

Copy link

@rtfeldman rtfeldman Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this might break peoples tests

Hm, I don't think this could break any existing tests, could it? 🤔

It's doing a List.member on a strictly larger list, so is there any way it could fail when previously it succeeded?

Edit: hasNot would do it. So yeah, this could cause regressions in tests.

What would such a regression look like though?

    |> Query.hasNot [ class "blah" ]

So in order for this to cause a regression, the broken test would have to:

  • Include a Query.hasNot using a class selector
  • Have it be for an element that has that exact class, but it's set as a class property when it should be an attribute (or vice versa - I forget which, but you get the idea)

To me that sounds more likely to expose an implementation bug (e.g. they thought the test was covering this situation but actually it wasn't because they were using an attribute when they should have used a property in the implementation) than to break a valuable test!

Copy link

@rtfeldman rtfeldman Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also, what is hasExactClassName? 😅

Edit: oh - did you mean exactClassName?

Copy link
Owner

@Confidenceman02 Confidenceman02 Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

[ test "does not find class on svg elements" <|
            \() ->
                let
                    svgClass =
                        "some-NS-class"
                in
                Svg.svg
                    [ SvgAttribs.class svgClass ]
                    [ Svg.circle [ SvgAttribs.cx "50", SvgAttribs.cy "50", SvgAttribs.r "40" ] [] ]
                    |> Query.fromHtml
                    |> Query.hasNot [ class svgClass ]

This is a test I wrote to assert that the svg class was not being found. With these changes that would break.

Despite how useful we think this test is, the fact is, the changes would break previously passing tests.

Also, what is hasExactClassName? 😅

I meant exactClassName 😅

exactClassName is an existing function that does not split the class names. So it will assert the entire class string. Here is NS equivalent I wrote to mimic the behaviour.

test "exactClassNameNS selector finds the exact class value on svg" <|
            \() ->
                let
                    svgClass =
                        "some-NS-class another-NS-class"
                in
                Svg.svg
                    [ SvgAttribs.class svgClass ]
                    [ Svg.circle [ SvgAttribs.cx "50", SvgAttribs.cy "50", SvgAttribs.r "40" ] [] ]
                    |> Query.fromHtml
                    |> Query.has [ exactClassNameNS svgClass ]

Copy link

Choose a reason for hiding this comment

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

This is a test I wrote to assert that the svg class was not being found. With these changes that would break.

Right - certainly it can break, but I think it's relevant to consider the likelihood that this actually comes up.

Can we think of a real-world example where all of the following are true?

  1. Someone wrote code using hasNot with class
  2. That query applies to a node which has that particular class specified as a property, not an attribute (so the test passes today but would break with this change)
  3. If the test failed, they would be upset because 1. and 2. are exactly the behavior they wanted to get out of the test - as opposed to being happy because the test failing led them to realize that the test was actually passing by accident (because they had specified a property, not an attribute, unintentionally - meaning the browser saw the class but the selector didn't, at least the way the selector works today) and leading them to change their code for the better.

Personally I'm finding it hard to imagine how all 3 of those could be true in someone's code base today, but I could be missing something! 😄

Copy link
Owner

Choose a reason for hiding this comment

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

Im certainly not going to make assumptions about how people test their code, just raising the point that we are touching a path that changes test behaviour on nodes other than svg, which was the focus of the fix. Whether this has other weird and wonderful side effects, not sure?

It seems you are favouring this approach? I am happy to support yours and @tesk9 's intuition on this.

@tesk9 Would you be keen to implement the exactClassName implementation?

Copy link
Author

Choose a reason for hiding this comment

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

If we're sure we want exactClassName to match both attributes and properties, I'm happy to implement!



hasClasses : List String -> Facts msg -> Bool
hasClasses classList facts =
containsAll classList (classnames facts)


hasClassesNS : List String -> Facts msg -> Bool
hasClassesNS classList facts =
containsAll classList (classnamesNS facts)
containsAll classList (classnames facts ++ classes facts)


hasStyle : { key : String, value : String } -> Facts msg -> Bool
Expand All @@ -271,8 +259,8 @@ classnames facts =
|> String.split " "


classnamesNS : Facts msg -> List String
classnamesNS facts =
classes : Facts msg -> List String
classes facts =
Dict.get "class" facts.stringAttributes
|> Maybe.withDefault ""
|> String.split " "
Expand All @@ -296,18 +284,10 @@ nodeRecordPredicate selector =
.facts
>> hasClass classname

ClassNameNS classname ->
.facts
>> hasClassNS classname

ClassList classList ->
.facts
>> hasClasses classList

ClassListNS classList ->
.facts
>> hasClassesNS classList

Tag tag ->
.tag
>> (==) tag
Expand Down Expand Up @@ -343,18 +323,10 @@ markdownPredicate selector =
.facts
>> hasClass classname

ClassNameNS classname ->
.facts
>> hasClassNS classname

ClassList classList ->
.facts
>> hasClasses classList

ClassListNS classList ->
.facts
>> hasClassesNS classList

Tag tag ->
always False

Expand Down
86 changes: 6 additions & 80 deletions src/Test/Html/Selector.elm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module Test.Html.Selector exposing
( Selector
, tag, text, containing, attribute, all
, id, class, classNS, classes, classesNS, exactClassName, exactClassNameNS, style, checked, selected, disabled
, id, class, classes, exactClassName, style, checked, selected, disabled
)

{-| Selecting HTML elements.
Expand All @@ -16,7 +16,7 @@ module Test.Html.Selector exposing

## Attributes

@docs id, class, classNS, classes, classesNS, exactClassName, exactClassNameNS, style, checked, selected, disabled
@docs id, class, classes, exactClassName, style, checked, selected, disabled

-}

Expand Down Expand Up @@ -85,32 +85,6 @@ classes =
Classes


{-| Matches svg elements that have all the given classes (and possibly others as well).

When you only care about one class instead of several, you can use
[`classNS`](#classNS) instead of passing this function a list with one value in it.

To match the element's exact class attribute string, use [`exactClassNameNS`](#exactClassNameNS).

import Svg.Html as SvgHtml
import Svg.Attributes as SvgAttr
import Test.Html.Query as Query
import Test exposing (test)
import Test.Html.Selector exposing (classesNS)


test "Svg has the classes svg-styles and svg-large" <|
\() ->
SvgHtml.svg [ SvgAttr.class "svg-styles svg-large" ] []
|> Query.fromHtml
|> Query.has [ classesNS [ "svg-styles", "svg-large" ] ]

-}
classesNS : List String -> Selector
classesNS =
ClassesNS


{-| Matches elements that have the given class (and possibly others as well).

To match multiple classes at once, use [`classes`](#classes) instead.
Expand All @@ -136,36 +110,11 @@ class =
Class


{-| Matches svg elements that have the given class (and possibly others as well).

To match multiple classes at once, use [`classesNS`](#classesNS) instead.

To match the element's exact class attribute string, use [`exactClassNameNS`](#exactClassNameNS).

import Svg.Html as SvgHtml
import Svg.Attributes as SvgAttr
import Test.Html.Query as Query
import Test exposing (test)
import Test.Html.Selector exposing (classNS)


test "Svg has the class svg-large" <|
\() ->
SvgHtml.svg [ SvgAttr.class "svg-styles svg-large" ] [ ]
|> Query.fromHtml
|> Query.has [ classNS "svg-large" ]

-}
classNS : String -> Selector
classNS =
ClassNS


{-| Matches the element's exact class attribute string.
{-| Matches the element's exact "className" property string.

This is used less often than [`class`](#class), [`classes`](#classes) or
[`attribute`](#attribute), which check for the _presence_ of a class as opposed
to matching the entire class attribute exactly.
to matching the entire class property exactly.

import Html
import Html.Attributes as Attr
Expand All @@ -180,37 +129,14 @@ to matching the entire class attribute exactly.
|> Query.fromHtml
|> Query.has [ exactClassName "btn btn-large" ]

There's a difference between properties and attributes (read more [here](https://github.com/elm/html/blob/master/properties-vs-attributes.md)).

-}
exactClassName : String -> Selector
exactClassName =
namedAttr "className"


{-| Matches the svg element's exact class attribute string.

This is used less often than [`classNS`](#classNS) or [`classesNS`](#classesNS),
which check for the _presence_ of a class as opposed to matching the entire
class attribute exactly.

import Svg.Html as SvgHtml
import Svg.Attributes as SvgAttr
import Test.Html.Query as Query
import Test exposing (test)
import Test.Html.Selector exposing (exactClassNameNS)


test "Svg has the exact class 'svg-styles svg-large'" <|
\() ->
SvgHtml.svg [ SvgAttr.class "btn btn-large" ] []
|> Query.fromHtml
|> Query.has [ exactClassNameNS "svg-styles svg-large" ]

-}
exactClassNameNS : String -> Selector
exactClassNameNS =
namedAttr "class"


{-| Matches elements that have the given `id` attribute.

import Html
Expand Down
14 changes: 0 additions & 14 deletions src/Test/Html/Selector/Internal.elm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@ import Test.Html.Internal.ElmHtml.Query as ElmHtmlQuery
type Selector
= All (List Selector)
| Classes (List String)
| ClassesNS (List String)
| Class String
| ClassNS String
| Attribute { name : String, value : String }
| BoolAttribute { name : String, value : Bool }
| Style { key : String, value : String }
Expand Down Expand Up @@ -42,15 +40,9 @@ selectorToString criteria =
Classes list ->
"classes " ++ quoteString (String.join " " list)

ClassesNS list ->
"classesNS " ++ quoteString (String.join " " list)

Class class ->
"class " ++ quoteString class

ClassNS class ->
"classNS " ++ quoteString class

Attribute { name, value } ->
"attribute "
++ quoteString name
Expand Down Expand Up @@ -145,15 +137,9 @@ query fn fnAll selector list =
Classes classes ->
List.concatMap (fn (ElmHtmlQuery.ClassList classes)) elems

ClassesNS classes ->
List.concatMap (fn (ElmHtmlQuery.ClassListNS classes)) elems

Class class ->
List.concatMap (fn (ElmHtmlQuery.ClassList [ class ])) elems

ClassNS class ->
List.concatMap (fn (ElmHtmlQuery.ClassListNS [ class ])) elems

Attribute { name, value } ->
List.concatMap (fn (ElmHtmlQuery.Attribute name value)) elems

Expand Down
Loading