Add tests for streaming chunks.#52253
Conversation
|
I was investigating the no layout test failure, and found that for some reason, streaming is disabled when there was no layout. So, in that case, stream was not actually being respected and it was just returning an array of chunks. I changed this behaviour so that it would correctly stream the response. |
886e089 to
fa71302
Compare
| # that knows how to render the template. | ||
| def render_template(view, template, layout_name = nil, locals = {}) # :nodoc: | ||
| return [super.body] unless layout_name && template.supports_streaming? | ||
| return [super.body] unless template.supports_streaming? |
There was a problem hiding this comment.
This change (layout_name) was introduced in 29078ff but I'm not sure why. It prevents proper streaming for non-layout controller actions. The test appears to assume streaming should work, and so I've removed this condition.
e4b4d01 to
2900df4
Compare
|
|
||
| class StreamingTest < Rack::TestCase | ||
| test "rendering with streaming enabled at the class level" do | ||
| env = Rack::MockRequest.env_for("/render_streaming/basic/hello_world") |
There was a problem hiding this comment.
Why can't use keep using get anymore?
There was a problem hiding this comment.
I considered how I could simplify the tests.
Unfortunately, Rails is using it's own bespoke implementation for TestResponse:
module ActionDispatch
class TestResponse < Response
def self.from_response(response)
new response.status, response.headers, response.body
endThis assigns using Response#body=:
module ActionDispatch
class Response
# Allows you to manually set or override the response body.
def body=(body)
if body.respond_to?(:to_path)
@stream = body
else
synchronize do
@stream = build_buffer self, munge_body_object(body)
end
end
endUnfortunately, at this time, there is no way to bypass build_buffer self, munge_body_object(body). It's necessary to see the unbuffered "chunks" in order to verify streaming. Once this PR, and the other linked PR is merged, I'll try to rework these tests with an improved approach.
In other words, I do agree we should simplify the tests, but I don't want this PR to introduce significant changes to the implementation in order to do so.
8bdc3b3 to
11b7a82
Compare
|
@rafaelfranca any chance you can merge this? Thanks! If you want to delegate this effort, feel free to give me the commit bit so I can work on this independently. |
11b7a82 to
da75aa8
Compare
|
I've re-based on main. |
da75aa8 to
28440a3
Compare
28440a3 to
e56b252
Compare
|
@rafaelfranca it would be great to merge this before Rails 8 is released. |
|
I'm going to rebase this all into one PR as it's easier for me to deal with. |
Motivation / Background
The tests for streaming responses, as modified in #52094 are insufficient to determine whether the response included multiple chunks. We should ensure that this is the case, as well as ensuring that the response body conforms to an "Enumerable Body" as defined by the Rack specification when
stream: trueis requested.Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]