Skip to content

Conversation

nduitz
Copy link
Contributor

@nduitz nduitz commented Sep 2, 2025

This test currently asserts on the "Log in" text rendered in the root layout's default navigation.

IMHO this should assert on actual page content instead of nav bar content.

Maybe it was meant to assert on this "Log in":

But that would require the user to be already logged in, not just to be confirmed and would require a different test that I tried to implement.
However the problem with that test is that the assertion is also satisfied by the nav bar content.

@nduitz nduitz changed the title test on page content not page nav content [phx.gen.auth] test on page content not page nav content Sep 2, 2025
@nduitz nduitz changed the title [phx.gen.auth] test on page content not page nav content [phx gen.auth] test on page content not page nav content Sep 2, 2025
@josevalim josevalim requested a review from SteffenDE September 23, 2025 11:47
Copy link
Contributor

@SteffenDE SteffenDE left a comment

Choose a reason for hiding this comment

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

We can also have more specific assertions. The only downside is that they need to communicate with the view, so they are probably slightly slower than the simple string match.

Instead of

refute html =~ "Confirm my account"
assert html =~ "Log in"

you could do

refute has_element?(lv, "button", "Confirm my account")
assert has_element?(lv, "button", "Log in")

This should fail for content that is only part of the root layout, has has_element? searches the current LiveView's HTML only.

Preferences, @josevalim?

@josevalim
Copy link
Member

Yes, I like the button assertions!

@SteffenDE SteffenDE merged commit fbca54a into phoenixframework:main Oct 10, 2025
5 of 6 checks passed
@SteffenDE
Copy link
Contributor

I'll follow this up with another PR!

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 this pull request may close these issues.

3 participants