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

Default headers, removed in controller actions, will not be reapplied to the test response #18423

Merged
merged 1 commit into from
Jan 9, 2015
Merged

Default headers, removed in controller actions, will not be reapplied to the test response #18423

merged 1 commit into from
Jan 9, 2015

Conversation

jone
Copy link
Contributor

@jone jone commented Jan 9, 2015

Default headers, removed in controller actions, were reapplied to the test response, although it works perfectly in production.
This change disables merging headers with the default headers a second time in the test response.

/cc @senny

@senny
Copy link
Member

senny commented Jan 9, 2015

@jone thank you 💛

@senny senny merged commit 0739480 into rails:master Jan 9, 2015
senny added a commit that referenced this pull request Jan 9, 2015
Default headers, removed in controller actions, will not be reapplied to the test response
senny added a commit that referenced this pull request Jan 9, 2015
Default headers, removed in controller actions, will not be reapplied to the test response

Conflicts:
	actionpack/CHANGELOG.md
senny added a commit that referenced this pull request Jan 9, 2015
Default headers, removed in controller actions, will not be reapplied to the test response

Conflicts:
	actionpack/CHANGELOG.md
@jeremy
Copy link
Member

jeremy commented Feb 25, 2015

This assumes that a TestResponse is always instantiated with an existing response's headers, which include the default headers. Is that accurate? If you create a TestResponse with headers = {}, when are the default headers merged if this method is overridden to ignore them?

@senny
Copy link
Member

senny commented Feb 25, 2015

@jeremy you're right, that was the assumption. I see this is not the case for controller tests.

I'll write tests to show the introduced bug and move the patch into a new subclass only used for integration tests.

Thank you for pointing this out.

@kaspth
Copy link
Contributor

kaspth commented Feb 25, 2015

@42001983 to stop getting these emails you have to unwatch the Rails repository on GitHub.

senny added a commit to senny/rails that referenced this pull request Feb 25, 2015
After rails#18423 default headers were no longer assigned to any test response.
That fix should be constrained to the response used during integration tests.
jeremy added a commit that referenced this pull request Feb 25, 2015
Fixes regression in #18423. Merge default headers for new responses,
but don't merge when creating a response from the last session request.

hat tip @senny ❤️
@jeremy
Copy link
Member

jeremy commented Feb 25, 2015

Fixed by optionally merging the default headers rather than disabling the merge. Added a regression test for functional tests to demonstrate (thanks @senny!)

jeremy added a commit that referenced this pull request Feb 25, 2015
Fixes regression in #18423. Merge default headers for new responses,
but don't merge when creating a response from the last session request.

hat tip @senny ❤️
jeremy added a commit that referenced this pull request Feb 25, 2015
Fixes regression in #18423. Merge default headers for new responses,
but don't merge when creating a response from the last session request.

hat tip @senny ❤️
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants