Add rack_url_scheme to Puma::DSL, allows setting of rack.url_scheme header #2586
Conversation
|
I would've appreciated the test refactoring as a separate (or several) separate PRs 😫 |
test/helpers/svr_in_proc.rb
Outdated
| sleep 0.01 | ||
| end | ||
|
|
||
| def rack_env_to_body |
There was a problem hiding this comment.
This is a test fixture, and as such should be defined in the test that uses it, IMO
There was a problem hiding this comment.
See below. After these are in the repo, other test files can be rewritten (and simplified) by using these fixtures.
test/test_rack_env.rb
Outdated
|
|
||
| def rack_url_scheme(val) | ||
| start_server rack_env_to_body | ||
| assert_includes fast_connect_get_body.lines, "rack.url_scheme: #{val}\n" |
There was a problem hiding this comment.
We should have a higher-level assert:
assert_body_includes "rack.url_scheme"
...so test writers don't have to understand fast_connect_get_body.lines
test/helpers/svr_in_proc.rb
Outdated
| } | ||
| end | ||
|
|
||
| def ci_test |
There was a problem hiding this comment.
Looks like this is not used in this pull request?
There was a problem hiding this comment.
svr_in_proc.rb is a generic fixture file. See below.
test/helpers/sockets.rb
Outdated
|
|
||
| # Generates the request/response time distribution string | ||
| # | ||
| def time_info(threads, clients_per_thread, time_ary, req_per_client = 1) |
|
If you could remove everything from this PR that's not actually used in this PR, that would help me a lot to understand what's going on. The whole approach and changes are 💯 but I can't follow everything when there's so much to sort through. |
|
All the files in test/helpers are intended to be test fixture files. These are part of the test rewrite I've been using for while, and I've also mentioned. This PR includes some of them, and uses them to for the new test file 'test_rack_env.rb'. PR #2595 'Improve client response code, chunked bodies', also uses Re the helper files:
So, these fixtures can be used to simplify test_puma_server, test_puma_server_ssl, etc. I've got a few test files I've converted to the new system, but those files are not in this PR. A file that isn't in this PR is So, rather than a huge PR with a whole new test system, I thought adding the fixtures operating on one test file would be a good first step. I expect they will evolve, have more comments added, etc. I think I started a 'Test Fixture Overview` markdown file also. EDIT: Sorry I thought I rebased this recently. Just rebased... |
MSP-Greg
force-pushed
the
rack-url-scheme
branch
2 times, most recently
from
5aaf852
to
78c7836
Compare
|
#2620 is a new PR with only the test fixture files in it. If you're okay with merging it, I'll remove them from this PR? |
|
Removed the additional test files, added two tests to test_puma_server.rb... |
nateberkopec
added
waiting-for-review
…eader (puma#2586) * Add rack_url_scheme to Puma::DSL, allows setting of rack.url_scheme header * Clarify comment on DSL re: rack URL * Add tests to test_puma_server.rb Co-authored-by: Nate Berkopec <nate.berkopec@gmail.com>
Description
Closes #2569.
Your checklist for this pull request
[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.