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

[chrisftw]

As with the people in #15 AND #239 I was having issues setting/changing the DEFAULT_HOST.

Current work around is ugly:

require 'rack/test'
Rack::Test::DEFAULT_HOST='sub.example.org'

My tests have to check the app changes databases based on the subdomain, so an easy way to change the default host makes my tests much easier to read.

[chrisftw]

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

[ioquatix]

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.

[jeremyevans]

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).

[chrisftw]

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.

[ioquatix]

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: '...'}}

[jeremyevans]

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.

[ioquatix]

I took a look at the code, and there are a number of constants which might benefit from this transformation.

@jeremyevans what do you think about migrating all of these to class << self attr_accessors + methods?

https://github.com/rack/rack-test/blob/main/lib/rack/test.rb#L31-L43

This would be similar to what you did for Rack::Request.

[jeremyevans]

In general, I'm positive to supporting this feature in some way, but negative to this approach (adding Rack::Test.default_host).

[jeremyevans]

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).

[jeremyevans]

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.

[ioquatix]

@jeremyevans I don't fully understand why it's acceptable in Rack::Request but not here. Is it because of the @variable assignment?

In any case, maybe you can propose a good way to support this feature/functionality?

[jeremyevans]

@jeremyevans I don't fully understand why it's acceptable in Rack::Request but not here. Is it because of the @variable assignment?

In my comment (#317 (comment)), I stated if you changed where the variable was assigned, it would be acceptable. Rack::Request assigns the instance variable in the class, not in the singleton class, so it doesn't have the problem that your code in #317 (comment) does.

However, as I pointed out, I'm negative adding Rack::Test.default_host.

In any case, maybe you can propose a good way to support this feature/functionality?

I already proposed a way to get global, per suite, and per-test/spec default host: #317 (comment). What are you thoughts on that?

[ioquatix]

@jeremyevans I'm fine to defer to your judgement on this. I'd like to fully understand the implications of the singleton_class variable so that I don't repeat the mistakes.

Just so I understand, on this line:

rack-test/lib/rack/test/methods.rb

Line 45 in 112242e

Session.new(app)

app is referring to whatever is self.app which could be conceivably provided by let(:app) {...} in rspec or def app in minitest. You are proposing to add an additional hook, default_host in the same way. I'm fine with that.