-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Update pumactl to remove development as default environment #2035
Update pumactl to remove development as default environment #2035
Conversation
e021a7f
to
6b7b2ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
test/test_pumactl.rb
Outdated
end | ||
|
||
def test_environment_without_rack_env | ||
ENV.delete "RACK_ENV" # remove from travis |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
test/test_pumactl.rb
Outdated
def test_environment_specific_config_file_exist | ||
ENV.delete "RACK_ENV" | ||
port = 6002 | ||
Dir.mktmpdir do |tmp_dir| |
There was a problem hiding this comment.
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
@@ -0,0 +1,16 @@ | |||
class TestConfigFileBase < Minitest::Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seemed like the most appropriate place for this file, but let me know if there's another convention I should be following
@@ -1,26 +1,10 @@ | |||
# frozen_string_literal: true | |||
|
|||
require_relative "helper" | |||
require_relative "helpers/config_file" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me!
Solid PR, thanks for the contribution 👍 |
* Update pumactl to remove development as default environment * Extract with_config_file test helper method * Extract config file base test class, use with_env for pumactl tests
What changed?
This PR:
development
frompumactl
pumactl
environment
option overRACK_ENV
topumactl
, if providedfixes #2023