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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an assertion helper for checking an element's children #27

Closed
PJUllrich opened this issue Feb 15, 2024 · 16 comments 路 Fixed by #52
Closed

Add an assertion helper for checking an element's children #27

PJUllrich opened this issue Feb 15, 2024 · 16 comments 路 Fixed by #52

Comments

@PJUllrich
Copy link

PJUllrich commented Feb 15, 2024

Hey German 馃憢

First of all, thank you very much for this great library! 馃殌

I have a feature request: I work a lot with lists and tables and have to assert often that e.g. the first row in a table contains a certain text. Until now, I write custom CSS selectors like e.g.:

# Assert that the 2nd row in a table contains the string "My second row"
assert has_element?(view, ~s(table#my-table > tr:nth-of-type\(2\)), "My second row")

# Assert that the 3 row in a table has a specific data attribute
assert has_element?(view, ~s(table#my-table > tr:nth-of-type\(3\)[data-id="#{record.id}"]))

Do you think we could maybe add a helper that makes this easier to the library?

@germsvel
Copy link
Owner

Hey @PJUllrich. Good to hear from you! Thanks for giving PhoenixTest a try an opening an issue.

Right now, all the functions are working with CSS selectors because I find that to be very flexible because -- since it allows us to specify anything with CSS selectors. But I'd be interested to hear more about what you're looking for.

Are you saying you'd be looking for something like one of these? Or do you have some other type of assertion in your mind?

|> assert_has?(within("#mytable"), "tr", "My second row")

# or

|> within("#mytable", fn session -> 
  assert_has?(session, "tr", "My second row")
end)

# or

assert_has?(child("#mytable", "tr"), "My second row")

@PJUllrich
Copy link
Author

PJUllrich commented Feb 16, 2024

hey hey, so I'm just dreaming here and am not sure whether it's feasible to implement this, but what would be great if I could "step" through the children of an element in general. So, imagine I have the following HTML:

<table id="mytable">
  <tr data-id="row-1">
    <td>Hello World</td>
  </tr>
  <tr data-id="row-2">
    <td>Good bye</td>
  </tr>
</table>

Now, common test cases that I have to write for such HTML are:

  1. Test that the first row of the table has the data-id="row-1".
  2. Test that the first row of the table contains the text Hello World

I guess, in pseudo code, this could be:

row = nth_child("#mytable", "tr", 1)
assert_has_attribute(row, "data-id", "row-1")
assert_contains(row, "Hello World")

This test would get the table and its first tr-child. The first assertion is that the row element has the data-id="row-1" attribute. Then it checks whether the html of that child contains the string Hello World.

I don't think this could be done with CSS, but I'm unsure. Maybe Floki woudl be a better option here? That parses the HTML, so it might be easier to navigate through the children of an element.

@sodapopcan
Copy link
Contributor

sodapopcan commented Feb 17, 2024

I have a within in my project that seems to change a lot鈥擨 know I want it but also not quite sure what it should look like.

The nth_child hits on something that I actually just ran into myself and came to investigate the issues to see if anyone was talking about it. So TL;DR, this would solve been able to test sorting!

In a LiveViewTest I've always done it like this:

build(:foo, name: "Bender", position: 3)
build(:foo, name: "Fry", position: 2)
build(:foo, name: "Leela", position: 1)
build(:foo, name: "Amy", position: 4)

# ...

assert lv
       |> element("ul.my-list")
       |> render() =~ ~r/Leela.*Fry.*Bender.*Amy/s

It would be great to be able to do something like:

|> within("ul.my-list", fn session ->
  session
  |> assert_has(nth_child("li", "Leela", 1))
  |> assert_has(nth_child("li", "Fry", 2))
  |> assert_has(nth_child("li", "Bender", 3))
  |> assert_has(nth_child("li", "Amy", 4))
end)

Just spitballing on the syntax there (though I am personally a fan of that closure version).
:nth_child could also be an option to assert_has. In any event, it would be so much better than the regex version.

I'm not certain if it's a goal of this lib to be able to use completely in place of LiveViewTest, but oh boy it was a little painful having to bring it in just for that :) (Nothing against LVT, of course, this lib is just so nice!)

@sodapopcan
Copy link
Contributor

So I threw together a quick prototype and I realized that it really doesn't buy all that much since this is all possible with CSS. If you're interested it's here but all it does is provide the within DSL in the manner I showed above without any special nth_child handling. It lets you do this:

|> within("ul.my-list", fn element ->
  element
  |> assert_has("li:nth-child(1)", "Leela")
  |> assert_has("li:nth-child(2)", "Fry")
  |> assert_has("li:nth-child(3)", "Bender")
end)

As soon as I got this far I was like, "Waitammit, this is all already possible albeit slightly more verbose":

|> assert_has("ul.my-list li:nth-child(1)", "Leela")
|> assert_has("ul.my-list li:nth-child(2)", "Fry")
|> assert_has("ul.my-list li:nth-child(3)", "Bender")

@PJUllrich your example should be achievable like so:

assert_has("#my-table tr[data-id=row-1]:nth-child(1)", "Hello world")

I think this is somewhat of the same thought process I went through implementing my own within but I was never using Floki directly so I didn't quite reach this far.

In any event, perhaps something like this would still be desirable for people who are CSS-adverse. Asserting on order is also pretty common so maybe a helper that does that would be in ahem order. Something like:

|> assert_ordered("#my-table tr", "td.name", ["Leela", "Fry", "Bender"])

The first arg would be the selector to add nth-child(n) to, the second would be an optional additional selector, and the third (or second) a list of text matches.

@sodapopcan
Copy link
Contributor

sodapopcan commented Feb 18, 2024

While this is getting a bit off topic, I did a quick prototype of assert_order/3 and it's pretty low-touch. It's as simple as:

def assert_order(session, child_selector, texts) when is_list(texts) do
  texts
  |> Enum.with_index()
  |> Enum.each(fn {text, index} ->
    assert_has(session, "#{child_selector}:nth-child(#{index + 1})", text)
  end)
end
assert_order(session, "ul li", ~w(Leela Fry Bender))

I could be made a little more efficient but that would require more intrusive changes and not sure it would make that big of a difference. Not sure the function is even worth it but was getting into thinking about all of this so I thought I'd try it out.

@germsvel
Copy link
Owner

Love the discussion here! 馃コ

I think for now, I'm hesitant to introduce a lot more assertion helpers like assert_order unless I can figure out how they match the rest of the library. I'd want to explore other options first because it seems like too unique a use-case to warrant its own assertion helper. Or put differently, if we have an assertion helper for that, what else will we need one for?

And even though I like the ergonomics of within that scopes the rest of the queries to an element, I hesitate to introduce that because then everything else has to work in the within block -- clicks, assertions, etc.

But I feel your pain. I've had to assert against lists before, and there's always a desire to make it easy to say "assert that the second element has text this text". So, I think it's something we should try to improve.

I think having CSS selector helpers could be an interesting idea because we keep the flexibility of CSS, keep the existing workflow, and make them an optional layer that we can put on top of the existing library.

What if we were to translate your CSS into something that feels easier to user?

# before
assert has_element?(view, ~s(table#my-table > tr:nth-of-type\(2\)), "My second row")

# after
session
|> assert_has(nth_child("#my-table", "tr", n: 2), "My second row")

@sodapopcan
Copy link
Contributor

I actually wasn't really suggesting assert_order get included in the lib, I was just getting caught up in the discussion and spitting out some ideas about testing. I'm all for lean libraries and personally I'm perfectly happy using the CSS selectors. This topic actually made me realize I could as up until now I used regexes to assert order (see here).

nth_child could be good, though. I'm not sure the keyword list (ie, :n option) would be necessary as nth_child only takes one arg in CSS (well, aside from the tag, of course). Alternatively, if you were ok with adding options to assert_has it could be either:

assert_has("#my-table tr", "My second row", nth_child: 2)

or, sort of taking inspiration from Wallaby

assert_has("#my-table tr", "My second row", css: [nth_child: 2])

Of course all of these ideas (including your function idea) could be a slippery slope of people asking for all of CSS as options.

@sodapopcan
Copy link
Contributor

Sooooo now that I think more about this, a helper more like what you most recently suggested @germsvel could actually be pretty dang nice. I'm not 100% on a name but I'm thinking a simple 2-arity function that constructs an nth-child selector.

|> assert_has(nth_child("#my-table tr", 2), "My second row")

It feels gratuitous to have have an extra parameter to drill down into children when it's literally just a space in CSS. The value I'm starting to see in this function is NOT to hide CSS from people, but simply to have cleaner, more expressive tests.

Other possibilities for names could be:

  • sibling/2
  • position/2

WDYT?

@germsvel
Copy link
Owner

germsvel commented Feb 22, 2024

@sodapopcan yeah, if we go down the route of what I'm going to call CSS selector aliases (thin wrappers around CSS selectors), the questions then become:

  1. Which CSS selectors do we support?
  2. What should we name them? Should it be 1-1 with CSS names or do we try to make them a little nicer in some cases (but potentially more confusing)?

Off the top of my head:

Helper. CSS Selector
id("title") "#title"
class("post") ".post"
data(role: "user") "[data-role='user']"
descendant("#user", ".post") or within("#user", ".post") "#user .post"
child("#user", ".post") "#user > .post"
child("#user", ".post", n: 2) "#user > .post:nth-of-type(2)"

I think that would cover most of my use cases. At least, I think it would.

And since the helpers just render text, we could theoretically combine them:

assert_has(within(id("user"), class("post")), "User's post")

@sodapopcan
Copy link
Contributor

I think this hits the problem I mentioned above where this becomes a slippery slop. For me there is zero value in providing a function just to do .post, especially when class(".title") is more characters. That's why I was trying to emphasize that I really don't think this should be about hiding CSS from people, I was really looking to find a cleaner way to assert on the position of a child. I did forget all about the nth-of-type selector so perhaps the child version would be useful. I can also see value in the data as that sort of lines up with JS's dataset but otherwise I think I'm back to not pushing for any change here. I think you might be just creating pain for yourself when you find yourself maintaining an embedded Floki wrapper :)

@PJUllrich
Copy link
Author

Sorry for my absence here. I just wanted to chime in that I like this suggestion by @sodapopcan the best:

|> assert_has(nth_child("#my-table tr", 2), "My second row")

However, the parameters of the nt_child/2 function could be confusing. If the selector is #my-table tr does this mean that I'm checking the second child of #my-table, which is a tr, or do I check the second child of the tr element? That's why I split these two into separate parameters in my previous suggestion.

|> assert_has(nth_child("#my-table", "tr", 2), "My second row")

Something like that.


For the general discussion, I don't think CSS helpers are super important right now. The topic of this issue was more: "How can we extend the library to better test tables?". So, rather topical than general.

@germsvel
Copy link
Owner

Yeah, I agree that the id and class helpers probably wouldn't do much good. But having something like nth_child/3 could be a nice helper.

At that point, I might want to add something like within for a descendant selector and data for data attributes. But I can do that later if I want.

For the sake of this discussion, I think it could be interesting to add nth_child("#my-table", "tr", 2). @PJUllrich would that solve 99% of your table-testing problems? Or are there scenarios that it doesn't cover?

@PJUllrich
Copy link
Author

@germsvel I think so. Let's give it a try.

@sodapopcan
Copy link
Contributor

Just want to throw in that it would be nice to have nth_child/2 work as well. I was thinking that maybe the second arg could be useful for nth-of-type but then I realized that nth-of-type didn't quite work like I thought it did. But in any event, nth_child/3 still feels off to me, mostly because it feels like a potential catalyst of kicking off a little CSS builder library hidden within a testing library. I'm not sure how to articulate that super well, but just my 2 cents.

@germsvel
Copy link
Owner

@PJUllrich @sodapopcan curious what you think of the approach in #46

@germsvel
Copy link
Owner

@PJUllrich I ended up going a different route for this, but I think the solution would fit your problems.

assert_has/3 and refute_has/3 now take an at option to filter the assertion by a given position.

For example, if you want to assert that the second tr in your table has a given text, you could write it like this:

assert_has(session, "table#my-table tr", at: 2, text: "My second row")

It hasn't been released yet, but it should be available in main. I'll release this as soon as I can.

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 a pull request may close this issue.

3 participants