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.

Sign up for GitHub

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 Rack::Test.default_host= to change the default_host more easily. #317

Closed
wants to merge 1 commit into from
Closed

add Rack::Test.default_host= to change the default_host more easily. #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions lib/rack/test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,14 @@ module Rack
module Test
# The default host to use for requests, when a full URI is not
# provided.
DEFAULT_HOST = 'example.org'.freeze
def self.default_host
Copy link
Author

Choose a reason for hiding this comment

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

We could maybe leave in the constant DEFAULT_HOST for people using a work around?

@@default_host ||= 'example.org'.freeze
Copy link
Member

@ioquatix ioquatix Aug 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should we consider the following implementation:

DEFAULT_HOST = 'example.org'.freeze
class << self
  @default_host = DEFAULT_HOST
  attr_accessor :default_host
end

I'm not sure if we can remove DEFAULT_HOST.

Consider that ||= isn't thread safe and we could in theory have multiple threads accessing this field.

We've followed this model elsewhere: https://github.com/rack/rack/blob/main/lib/rack/request.rb#L17-L41

Don't have a strong opinion about it, just wanted to make a suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely cannot accept the use of class variables. Class variables in Ruby are broken and should never be used.

This approach:

class << self
  @default_host = DEFAULT_HOST
  attr_accessor :default_host
end

Is broken. It sets the instance variable in the singleton class instead of the class. However, with that fixed, because Rack::Test is a module and not a class, the approach seems acceptable (it wouldn't be acceptable for a class, but then it breaks in a subclass unless you copy the instance variable during subclassing).

Copy link
Author

Choose a reason for hiding this comment

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

Yeah using the approach above failed 1-44 tests in a somewhat random nature. Moving to this:

@default_host = DEFAULT_HOST
class << self
  attr_accessor :default_host
end

makes all the tests passing again. Since it is a module, I am not sure I see the difference.

end

# Sets the default host used when a full URI is not provided.
def self.default_host=(host)
@@default_host = host.freeze
end

# The default multipart boundary to use for multipart request bodies
MULTIPART_BOUNDARY = '----------XnJLe9ZIbbGUYtzPQJ16u1'.freeze
Expand All @@ -54,7 +61,7 @@ class Session
extend Forwardable
include Rack::Test::Utils

def self.new(app, default_host = DEFAULT_HOST) # :nodoc:
def self.new(app, default_host = Rack::Test.default_host) # :nodoc:
Copy link
Member

Choose a reason for hiding this comment

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

Just thinking about alternatives, is there no easy way to inject the default host argument on a per-test/spec basis?

What about if we made this default_host: options and provided some kind of app_options or something similar so you could write:

let(:app_options) {{default_host: '...'}}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think per test/spec is possible. You can override the URI provided for each request, but the CookieJar always uses the default host for the session, and Rack::Test::Methods does not provide a way to set the default_host, without overriding build_rack_test_session.

I think it may be better to modify build_rack_test_session. If the current scope responds to default_host, it calls it and uses it as the second argument. Otherwise, it continues to pass one argument (app). That way you can allow for global, per-suite, and per-test/spec setting of default_host, with minimal change to the current implementation. That seems like a better approach to me, what do you think? If you like that approach, I can implement it.

if app.is_a?(self)
# Backwards compatibility for initializing with Rack::MockSession
app
Expand Down Expand Up @@ -96,7 +103,7 @@ def self.new(app, default_host = DEFAULT_HOST) # :nodoc:
# submitted in #last_request. The methods store a Rack::MockResponse based on the
# response in #last_response. #last_response is also returned by the methods.
# If a block is given, #last_response is also yielded to the block.
def initialize(app, default_host = DEFAULT_HOST)
def initialize(app, default_host = Rack::Test.default_host)
@env = {}
@app = app
@after_request = []
Expand Down
4 changes: 2 additions & 2 deletions lib/rack/test/cookie_jar.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class Cookie # :nodoc:
# name=value format is name and value are provided.
attr_reader :raw

def initialize(raw, uri = nil, default_host = DEFAULT_HOST)
def initialize(raw, uri = nil, default_host = Rack::Test.default_host)
@default_host = default_host
uri ||= default_uri

Expand Down Expand Up @@ -133,7 +133,7 @@ def default_uri
class CookieJar # :nodoc:
DELIMITER = '; '.freeze

def initialize(cookies = [], default_host = DEFAULT_HOST)
def initialize(cookies = [], default_host = Rack::Test.default_host)
@default_host = default_host
@cookies = cookies.sort!
end
Expand Down
7 changes: 7 additions & 0 deletions spec/rack/test_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,13 @@
last_request.env['SERVER_NAME'].must_equal 'example.org'
end

it 'default_host can be changed' do
Rack::Test.default_host = 'rack.github.io'
request '/'
last_request.env['SERVER_NAME'].must_equal 'rack.github.io'
Rack::Test.default_host = 'example.org'
end

it 'yields the response to a given block' do
request '/' do |response|
response.must_be :ok?
Expand Down