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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HTTP status name to output of tests #22935

Merged
merged 1 commit into from
Jan 12, 2016

Conversation

cllns
Copy link
Contributor

@cllns cllns commented Jan 6, 2016

Earlier today I came across a test failure, that said something along the lines of:

Expected response to be a <success>, but was a <401>

I didn't recognize 401 as being Unauthorized at first, so I had to go look it up.

Rails, through Rack, knows what a 401 means. This patch leverages that. It turns the above output into something more helpful:

Expected response to be a <2XX: success>, but was a <401: Unauthorized>

At first I thought I'd only provide the name if the name was asserted (e.g. :not_found would show :not_authorized, but 404 would show 401) but I can't think of a case where it's not helpful to have both.

The code to implement simply that is done in the first commit. The remainder of the commits are refactorings which move the logic of moving between symbol and code into an AssertionResponse class, which also simplifies the logic of assert_response, which was relatively gnarly.

@rails-bot
Copy link

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

code_or_name = RESPONSE_PREDICATES.invert["#{code_or_name}?".to_sym]
end

AssertionResponse.new(code_or_name).code_and_name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line means that we're creating a new AssertionResponse for each time code_with_name is called. This happens twice in generate_response_message, once for the expected and once for the actual. It's not ideal to create another response object for the Actual, since we already have one, but if we didn't we'd have to add the code for converting between the response code and the response name into the actual Response object.

@cllns
Copy link
Contributor Author

cllns commented Jan 12, 2016

Just fixed the tests :) and rebased on master.

@@ -95,6 +95,7 @@ module Session
autoload :TestProcess
autoload :TestRequest
autoload :TestResponse
autoload :AssertionResponse
Copy link
Member

Choose a reason for hiding this comment

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

Instead of autoloading I think it is better to require the file where it is needed.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, actually it is better to autoload. 👍

@rafaelfranca
Copy link
Member

Could you squash your commits?

@cllns
Copy link
Contributor Author

cllns commented Jan 12, 2016

All set @rafaelfranca! Also added a CHANGELOG note.

Also, refactor logic to convert between symbol and response code,
via the AssertionResponse class
rafaelfranca added a commit that referenced this pull request Jan 12, 2016
Add HTTP status name to output of tests
@rafaelfranca rafaelfranca merged commit 12f4976 into rails:master Jan 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants