Better error message if response.body is blank or not parseable by Nokogiri

[ghiculescu]

Fixes rails/rails#44634

Open to ideas, but I think it's best to raise aggressively here since currently (or at least, before rails/rails#44554) your tests might be incorrectly passing because of this behaviour.

[brenogazzola]

When my tests broke and a puts @response.body gave me nothing, I was confused because I knew that it should be "You are being redirected", so I had to hunt down which PR changed this to figure out what I was expected to do.

Getting a message of "No node/document found. Did you return a blank body in your response?" wouldn't help me much, because I'd still be in a situation of saying "No, there was a 'You are being redirected' message here, where is it?"

Would mentioning something about redirects and maybe the follow_redirect here be considered too specific, even though this is code for test assertions? If we have access to the status code at that point, maybe detecting a 3XX and saying "No node/document found. Did you forget to follow the redirect?"

[rafaelfranca]

Instead of raising an exception we should just create a nodeset with an empty document, so all the assertions will return false.

[ghiculescu]

Yep I can do that. It won't resolve @brenogazzola feedback, but I'm not sure how much we want to couple this to the response code.

[rafaelfranca]

I don't think we need to. His codebase failing to follow the redirect has nothing to do with the change we had in Rails or even the bug we are fixing here. His tests were just wrong, it always was. They are passing by mistake and isn't Rails job to be telling that he was asserting something that would always pass. With this change we will continue to have the tests passing like before.

If he deleted the view he was expecting to be rendered after the redirect the tests would also pass. Should Rails detect that? I don't think so. This is the classic problem of people writing tests that never failing and not making sure this test would fail if the code wasn't there.

[ghiculescu]

Alright, I pushed an alternative version.

The only issue left is if you do assert_select count: 0 then that will still pass (whereas with the raise approach it always failed). Which is the example here. It's hard to know if it's "correct" or not. Technically it is correct (no nodes match) but not for the reasons you'd expect!

isn't Rails job to be telling that he was asserting something that would always pass

If Rails knows for a fact that your assertions are wrong, shouldn't it try to help you out by telling you?

[rafaelfranca]

assert_select count: 0 that should pass. The body is empty, so there is nothing to select here.

Rails doesn't know the reason why the assertion is incorrect, there are many reasons why a body can be empty. It could be a 200 response with an empty body, and sometimes that is exactly what people are expecting.

[brenogazzola]

We were using assert 0 instead of the more correct assert redirected to and follow/assert on our old tests, from when we were still getting used to integration tests, and thought assert 0 was a good way to ensure a user could not view something, either because it was not on the screen or he was redirected to another page (like a login page)

A warning message on an 300 status would be a teaching opportunity in cases like this (hey, that’s not how you test a redirect) but I agree with Rafael’s arguments that it’s not really Rails job to do this.