germsvel / phoenix_test

PhoenixTest provides a unified way of writing feature tests -- regardless of whether you're testing LiveView pages or static (non-LiveView) pages.
https://hex.pm/packages/phoenix_test
MIT License
176 stars 22 forks source link

Support arbitrary status codes in `assert_has/3` and `refute_has/3` #36

Closed dabaer closed 8 months ago

dabaer commented 8 months ago

Currently these functions assumes a status code of 200 when testing static pages, but there are some situations where we would return a non-200 code (for example authentication failure or rate limits).

I played around with it locally and was able to add an optional status argument to these functions: https://github.com/dabaer/phoenix_test/commit/e11567597905128f2305d76c651a7670e9a0e9b6

Is this an appropriate direction to go as far as the library concerned? Or perhaps additional considerations for other functions that use render_html I haven't thought of?

Just wanted to get a feel before I made a pull request

Thanks!

germsvel commented 8 months ago

Hi @dabaer, thanks for opening this issue. I think we should allow for rendering pages that don't return a 200. (I'm thinking here of pages that are 404s, 500s, stuff like that.)

And thanks for sharing code! It's so helpful. 🙏

I took a look at it, and I'm not sure I want the assertion helpers to know about statuses. I think it'd be nice if we handled more transparently in therender_html portions. So that, for example, if the response returns a 404, we try to render that page, and the end-user of the library can assert against something they would expect to see on the page.

For example,

session
|> assert_has("h1", "I'm sorry. We couldn't find that")

What do you think? Is there a reason why we'd want to assert the exact status code?

dabaer commented 8 months ago

Thank you for taking a look @germsvel, I agree that assert_has shouldn't care about the status code. I guess that was the simplest way I could see to manipulate render_html.

I'm not 100% sure, but this might require adjusting the foundations of assert_has and friends to remove the reliance on a particular status code, and somehow assert the status code earlier in the chain.

Really, this was only a cludge to work around errors raised in render_html when my pages returned non-200 status codes, and keeping in line with the integration and feature testing nature we probably shouldn't need or want to assert on a status code. From an end-user's point of view, they wont know, see or care about what status code the page has returned.

At a glance, a possible solution would be to abuse html_response, and inject the status code directly from the conn struct inside of render_html, basically ignoring it's status assertions just so we can get the html body that we want to actually assert on.

What are your thoughts?

EDIT: Something like https://github.com/dabaer/phoenix_test/commit/4bd61a381e40587af0f05df2a0f937b464b0f0dd ?

germsvel commented 8 months ago

At a glance, a possible solution would be to abuse html_response, and inject the status code directly from the conn struct inside of render_html, basically ignoring it's status assertions just so we can get the html body that we want to actually assert on.

Yes! I think that's exactly what we'd want to do.

I looked at the new changes in https://github.com/dabaer/phoenix_test/commit/4bd61a381e40587af0f05df2a0f937b464b0f0dd and they're gold!

If you're up for it, I think that'd be a great change to make. I'd love to review a PR. Otherwise, I'm happy to make that change as soon as I can.

dabaer commented 8 months ago

@germsvel Wonderful! I've created a pull request for those changes