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

Reset RAW_POST_DATA between test requests #32673

Merged
merged 1 commit into from Apr 21, 2018

Conversation

Projects
None yet
6 participants
@eugeneius
Copy link
Member

eugeneius commented Apr 20, 2018

RAW_POST_DATA is derived from the rack.input header, which changes with each test request. It needs to be cleared in scrub_env!, or all requests in the same test will see the value from the first request.

Reset RAW_POST_DATA between test requests
`RAW_POST_DATA` is derived from the `rack.input` header, which changes
with each test request. It needs to be cleared in `scrub_env!`, or all
requests within the same test will see the value from the first request.
@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Apr 20, 2018

r? @georgeclaghorn

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

@eugeneius eugeneius force-pushed the eugeneius:raw_post_multiple_requests branch to 7a8d964 Apr 21, 2018

@georgeclaghorn georgeclaghorn merged commit 7452351 into rails:master Apr 21, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eugeneius

This comment has been minimized.

Copy link
Member Author

eugeneius commented Apr 22, 2018

Thanks, @georgeclaghorn. Do you think this could be backported?

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor

bogdanvlviv commented Apr 22, 2018

Good catch. Yeah, I think It would be ok to backport 5-2-stable since we still actively support 5.2 and it would be easier doing it with these changes since it'll prevent unexpected behavior in test cases on 5-2-stable.

georgeclaghorn added a commit that referenced this pull request Apr 22, 2018

Merge pull request #32673 from eugeneius/raw_post_multiple_requests
Reset RAW_POST_DATA between test requests
@georgeclaghorn

This comment has been minimized.

Copy link
Member

georgeclaghorn commented Apr 22, 2018

Backported it. 👍

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented May 3, 2018

This broke the behavior when you set the RAW_POST_DATA explicitly before doing the request. Not that it is hard to change in the application since you can just set the body params, but this means that it causes a regression in a stable release.

@georgeclaghorn

This comment has been minimized.

Copy link
Member

georgeclaghorn commented May 3, 2018

Thanks, @rafaelfranca. I don’t see a way to resolve that, so I’ll revert this change in 5-2-stable later today unless @eugeneius has a better idea.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented May 3, 2018

To be fair, I think it is fine to keep the change. I just wanted to bring it up in case someone find the same problem.

I had to change some tests on Shopify but that actually improved the code, so I think that is a good thing.

@eugeneius

This comment has been minimized.

Copy link
Member Author

eugeneius commented May 4, 2018

It looks like using RAW_POST_DATA was the only way to directly set the request body at some point:

https://stackoverflow.com/questions/2103977/how-to-send-raw-post-data-in-a-rails-functional-test

This makes me think that more applications' test suites will break in the same way Shopify's did, and we should probably revert on 5-2-stable.

I ran into this problem when upgrading our application to 5.0, so we already have a monkey patch to fix it; I don't particularly mind if we have to wait until 6.0 to drop it.

@jdelStrother

This comment has been minimized.

Copy link
Contributor

jdelStrother commented Jul 11, 2018

FWIW, we also hit this while upgrading 5.2.0 -> 5-2-stable - some of our tests set RAW_POST_DATA directly.

thomasdziedzic added a commit to thomasdziedzic/rubygems.org that referenced this pull request Aug 24, 2018

fix rubygems api controller test
rails/rails#32673

Rails 5.2.1 broke backwards compatibility with setting RAW_POST_DATA in
a test.

@thomasdziedzic thomasdziedzic referenced this pull request Aug 24, 2018

Closed

Rails 5.2 #1771

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.