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

Jump to bottom

Update pumactl to remove development as default environment #2035

Merged
merged 3 commits into from Oct 18, 2019
Merged

Update pumactl to remove development as default environment #2035

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b7b2ed
Update pumactl to remove development as default environment
AndrewSpeed Oct 15, 2019
1b1c1a8
Extract with_config_file test helper method
AndrewSpeed Oct 16, 2019
1fa9eca
Extract config file base test class, use with_env for pumactl tests
AndrewSpeed Oct 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions History.md
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@

* Bugfixes
* Fix Errno::EINVAL when SSL is enabled and browser rejects cert (#1564)
* Fix pumactl defaulting puma to development if an environment was not specified (#2035)

## 4.2.1 / 2019-10-07

Expand Down
6 changes: 4 additions & 2 deletions lib/puma/control_cli.rb
View file
Open in desktop
Expand Up @@ -22,7 +22,7 @@ def initialize(argv, stdout=STDOUT, stderr=STDERR)
@control_auth_token = nil
@config_file = nil
@command = nil
@environment = ENV['RACK_ENV'] || "development"
@environment = ENV['RACK_ENV']

@argv = argv.dup
@stdout = stdout
Expand Down Expand Up @@ -82,8 +82,10 @@ def initialize(argv, stdout=STDOUT, stderr=STDERR)
@command = argv.shift

unless @config_file == '-'
environment = @environment || 'development'

if @config_file.nil?
@config_file = %W(config/puma/#{@environment}.rb config/puma.rb).find do |f|
@config_file = %W(config/puma/#{environment}.rb config/puma.rb).find do |f|
File.exist?(f)
end
end
Expand Down
49 changes: 44 additions & 5 deletions test/test_pumactl.rb
View file
Open in desktop
Expand Up @@ -29,16 +29,54 @@ def test_config_file
assert_equal "t3-pid", control_cli.instance_variable_get("@pidfile")
end

def test_environment
ENV.delete 'RACK_ENV' # remove from travis
def test_rack_env_without_environment
ENV.update("RACK_ENV" => "test")
control_cli = Puma::ControlCLI.new ["halt"]
assert_equal "development", control_cli.instance_variable_get("@environment")
assert_equal "test", control_cli.instance_variable_get("@environment")
end

def test_environment_without_rack_env
ENV.delete "RACK_ENV" # remove from travis
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of mutating the environment, which could lead to order-dependent test failures if there is another test that unexpectedly relies on a specific environment variable being set, it might be nice to reuse the with_env helper.

Or, if we are OK with adding an additional dependency, at thoughtbot we often use climate_control for this sort of thing. It has the advantage of being threadsafe, which might help if we plan to parallelize these tests.

Copy link
Member

Choose a reason for hiding this comment

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

Let's just use with_env for now, but keep our eyes on climate_control for the future.

control_cli = Puma::ControlCLI.new ["halt"]
assert_nil control_cli.instance_variable_get("@environment")
control_cli = Puma::ControlCLI.new ["-e", "test", "halt"]
assert_equal "test", control_cli.instance_variable_get("@environment")
end

def test_environment_with_rack_env
ENV.update("RACK_ENV" => "production")
control_cli = Puma::ControlCLI.new ["halt"]
assert_equal "production", control_cli.instance_variable_get("@environment")
control_cli = Puma::ControlCLI.new ["-e", "test", "halt"]
assert_equal "test", control_cli.instance_variable_get("@environment")
end

def test_config_file_exist
ENV.delete 'RACK_ENV' # remove from travis
def test_environment_specific_config_file_exist
ENV.delete "RACK_ENV"
port = 6002
Dir.mktmpdir do |tmp_dir|
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some of this file manipulation makes the test a bit more difficult to read. What do you think about extracting a helper like:

def with_config_file(path_to_config, port)
  path = Pathname.new(path_to_config)
  Dir.mktmpdir do |tmp_dir|
    Dir.chdir(tmp_dir) do
      FileUtils.mkdir(path.dir)
      File.open(path, "w") { |f| f << "port #{port}" }
      yield
    end
  end
end

def test_environment_specific_config_file_exist
  port = 6002
  path_to_config = "config/puma.rb"

  with_config_file(path_to_config, port) do
    control_cli = Puma::ControlCLI.new ["-e", "production", "halt"]
    assert_equal path_to_config,
      control_cli.instance_variable_get("@config_file")
  end
end

Dir.chdir(tmp_dir) do
FileUtils.mkdir("config")
File.open("config/puma.rb", "w") { |f| f << "port #{port}" }
control_cli = Puma::ControlCLI.new ["-e", "production", "halt"]
assert_equal "config/puma.rb",
control_cli.instance_variable_get("@config_file")
end
end

Dir.mktmpdir do |tmp_dir|
Dir.chdir(tmp_dir) do
FileUtils.mkdir_p("config/puma")
File.open("config/puma/production.rb", "w") { |f| f << "port #{port}" }
control_cli = Puma::ControlCLI.new ["-e", "production", "halt"]
assert_equal "config/puma/production.rb",
control_cli.instance_variable_get("@config_file")
end
end
end

def test_default_config_file_exist
ENV.delete "RACK_ENV" # remove from travis
port = 6001
Dir.mktmpdir do |d|
Dir.chdir(d) do
Expand All @@ -49,6 +87,7 @@ def test_config_file_exist
control_cli.instance_variable_get("@config_file")
end
end

Dir.mktmpdir do |d|
Dir.chdir(d) do
FileUtils.mkdir_p("config/puma")
Expand Down