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 as to encode a request as a specific mime type. #21671

Merged
merged 1 commit into from Feb 10, 2016

Conversation

kaspth
Copy link
Contributor

@kaspth kaspth commented Sep 18, 2015

Turns longish integration test setup like

post articles_path(format: :json), params: { article: { title: 'Ahoy!' } }.to_json,
  headers: { 'Content-Type' => 'application/json' }

into

post articles_path, params: { article: { title: 'Ahoy!' } }, as: :json

And removes the need to add helpers like post_json etc.

I'm not sure if encoder is the term to use internally. Perhaps interpreter would be better. Suggestions welcome.

Perhaps we'd need more mime types. Suggestions welcome.

@kaspth kaspth self-assigned this Sep 18, 2015
@kaspth kaspth added this to the 5.0.0 milestone Sep 18, 2015
@kaspth kaspth force-pushed the integration-request-encoding-helpers branch from e8e5f46 to 749b74b Compare Sep 19, 2015
@kaspth
Copy link
Contributor Author

kaspth commented Sep 19, 2015

All right, added some tests and documentation.

content_type: 'text/wibble', param_encoder: -> params { params.to_s }

assert_encoded_as :wibble, content_type: 'text/wibble',
parsed_parameters: Hash.new # Unregistered MIME Type can't be parsed
Copy link
Contributor Author

@kaspth kaspth Sep 19, 2015

Choose a reason for hiding this comment

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

Am I right in making this claim? Definitely feels like it could use some shine, but don't have a fully formed idea of which direction to take it.

Copy link
Member

@rafaelfranca rafaelfranca Dec 31, 2015

Choose a reason for hiding this comment

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

I don't think so. Try to test it in a controller with curl.

@arthurnn
Copy link
Member

arthurnn commented Oct 18, 2015

I like the idea behind this. 👍 I remember having to do those setups before, and indeed we could make that setup easier.

@arthurnn
Copy link
Member

arthurnn commented Oct 18, 2015

dont forget to add a changelog for it.

def process(method, path, params: nil, headers: nil, env: nil, xhr: false)
def process(method, path, params: nil, headers: nil, env: nil, xhr: false, as: nil)
request_encoder = RequestEncoder.encoder(as)
location = URI.parse(path)
Copy link
Contributor Author

@kaspth kaspth Oct 18, 2015

Choose a reason for hiding this comment

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

Should probably do a test for how much of an impact this has on our integration test speed.

Copy link
Member

@rafaelfranca rafaelfranca Dec 31, 2015

Choose a reason for hiding this comment

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

Can we defer the path parsing to append_format_to? If we don't need to append a format and if the paths is not a full path there is no need to parsing.

@kaspth
Copy link
Contributor Author

kaspth commented Oct 18, 2015

@arthurnn cool 😁

@rafaelfranca rafaelfranca modified the milestones: 5.0.0, 5.0.0 [temp] Dec 31, 2015
Turns

```
post articles_path(format: :json), params: { article: { name: 'Ahoy!' } }.to_json,
  headers: { 'Content-Type' => 'application/json' }
```

into

```
post articles_path, params: { article: { name: 'Ahoy!' } }, as: :json
```
@kaspth kaspth force-pushed the integration-request-encoding-helpers branch from 749b74b to 52bb2d3 Compare Jan 4, 2016
ActionDispatch::IntegrationTest.register_encoder(:wibble, &:itself)

assert_encoded_as :wibble, content_type: 'text/wibble',
parsed_parameters: Hash.new # Unregistered MIME Type can't be parsed
Copy link
Contributor Author

@kaspth kaspth Jan 4, 2016

Choose a reason for hiding this comment

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

@rafaelfranca tried your suggestion to call a custom controller via curl, but couldn't get it to work.

When I byebug request_parameters it seems to fetch_header which calls @env.fetch which already seems to have an empty hash for that key. Would you have any ideas why? I'll keep digging 😁

@kaspth
Copy link
Contributor Author

kaspth commented Jan 4, 2016

Reworked this to use Mime types as well, which simplified the tiny public API some more.

@dhh
Copy link
Member

dhh commented Feb 10, 2016

Lovely! We have one of those annoying post_json helpers as well.

dhh added a commit that referenced this pull request Feb 10, 2016
…lpers

Add `as` to encode a request as a specific mime type.
@dhh dhh merged commit 688996d into rails:master Feb 10, 2016
@kaspth kaspth deleted the integration-request-encoding-helpers branch Feb 10, 2016
y-yagi added a commit to y-yagi/rails that referenced this pull request May 27, 2016
prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016
- Fixes rails#25183.
- The `as: :json` feature was added in
  rails#21671 and recommended to use for
  JSON endpoints so let's use it by default for API controller tests.
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

4 participants