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.

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

Committed improvements #35

Merged
merged 12 commits into from
Apr 10, 2018
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## master

* Add `:pumactl` option
* Add `:restart_timeout` option
* Don't stop Puma if it was not started by Guard
* Don't notify about start when no start
Copy link
Owner

Choose a reason for hiding this comment

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

👍

* Improve `guard-compat` using (https://github.com/guard/guard-compat#migrating-your-api-calls)
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ end
* `:quiet` runs the server in quiet mode, suppressing output (default `true`).
* `:debugger` runs the server with the debugger enabled (default `false`). Required ruby-debug gem.
* `:timeout` waits this number of seconds when restarting the Puma server before reporting there's a problem (default `20`).
* `:restart_timeout` waits this number of seconds before the next restarting the Puma server (default `1`).
* `:config` is the path to the Puma config file (optional)
* `:bind` is URI to bind to (tcp:// and unix:// only) (optional)
* `:control_token` is the token to use as authentication for the control server(optional)
Expand Down
4 changes: 4 additions & 0 deletions lib/guard/puma.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ def self.default_env
:start_on_start => true,
:force_run => false,
:timeout => 20,
:restart_timeout => 1,
:debugger => false,
:notifications => %i[restarting restarted not_restarted stopped]
}
Expand All @@ -26,6 +27,7 @@ def initialize(options = {})
@options = DEFAULT_OPTIONS.merge(options)
@options[:port] = nil if @options.key?(:config)
@runner = ::Guard::PumaRunner.new(@options)
@last_restarted = Time.now
end

def start
Expand All @@ -38,6 +40,8 @@ def start
end

def reload
return if Time.now - @last_restarted < options[:restart_timeout]
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add parens to make the order of operations more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sure.

@last_restarted = Time.now
Compat::UI.info "Restarting Puma..."
if options[:notifications].include?(:restarting)
Compat::UI.notify(
Expand Down
42 changes: 37 additions & 5 deletions spec/lib/guard/puma_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,19 @@
end

describe "#reload" do
let(:zero_restart_timeout) { { restart_timeout: 0 } }
let(:options) { zero_restart_timeout }

before do
expect(Guard::Compat::UI).to receive(:info).with('Restarting Puma...')
expect(Guard::Compat::UI).to receive(:info).with('Puma restarted')
allow(guard.runner).to receive(:restart).and_return(true)
allow_any_instance_of(Guard::PumaRunner).to receive(:halt)
end

context "with default options" do
it "restarts and show the message" do
expect(Guard::Compat::UI).to receive(:info).with('Restarting Puma...')
expect(Guard::Compat::UI).to receive(:info).with('Puma restarted')

expect(Guard::Compat::UI).to receive(:notify).with(
/restarting on port 4000/,
hash_including(title: "Restarting Puma...", image: :pending)
Expand All @@ -124,9 +128,12 @@
end

context "with config option set" do
let(:options) { { config: "config.rb" } }
let(:options) { { config: "config.rb" }.merge!(zero_restart_timeout) }

it "restarts and show the message" do
expect(Guard::Compat::UI).to receive(:info).with('Restarting Puma...')
expect(Guard::Compat::UI).to receive(:info).with('Puma restarted')

expect(Guard::Compat::UI).to receive(:notify).with(
/restarting/,
hash_including(title: "Restarting Puma...", image: :pending)
Expand All @@ -142,9 +149,13 @@
end

context "with custom :notifications option" do
let(:options) { { notifications: [:restarted] } }
let(:options) do
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a fan of the Semantic Rule so let's change to {} instead of do/end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, no problem.

{ notifications: [:restarted] }.merge!(zero_restart_timeout)
end

it "restarts and show the message only about restarted" do
allow(Guard::Compat::UI).to receive(:info)

expect(Guard::Compat::UI).not_to receive(:notify).with(/restarting/)
expect(Guard::Compat::UI).to receive(:notify)
.with(/restarted/, kind_of(Hash))
Expand All @@ -154,15 +165,36 @@
end

context "with empty :notifications option" do
let(:options) { { notifications: [] } }
let(:options) { { notifications: [] }.merge!(zero_restart_timeout) }

it "restarts and doesn't show the message" do
allow(Guard::Compat::UI).to receive(:info)

expect(Guard::Compat::UI).not_to receive(:notify)

guard.reload
end
end

context "with :restart_timeout option" do
let(:restart_timeout) { 1.0 }
let(:options) { { restart_timeout: restart_timeout } }

before { sleep restart_timeout }

it "doesn't restarts during restart timeout" do
allow(Guard::Compat::UI).to receive(:info)
allow(Guard::Compat::UI).to receive(:notify)

expect(guard.runner).to receive(:restart).twice

guard.reload
sleep restart_timeout / 2
guard.reload
sleep restart_timeout
guard.reload
end
end
end

describe "#stop" do
Expand Down