[Feature] Add the possibility to pause/unpause by sending OS signal #254

Merged
merged 1 commit into from Mar 14, 2012

Conversation

Projects
None yet
4 participants
Contributor

steakknife commented Mar 6, 2012

USR1/USR2 signals and doc.

Future suggestion: print guard's pid when is verbose enabled to help users identify multiple guard processes.

Owner

thibaudgg commented Mar 6, 2012

Thanks @steakknife, can you please add some specs :)

Contributor

steakknife commented Mar 10, 2012

Summary: Humour taken seriously may lead to awesome. :)

Although other pulls have tested signal handler code on each other, I'll see what's possible just for the sport of it.

Let me know if you find specs or helpers for rspec, rails, etc. that check signal handling.

Contributor

netzpirat commented Mar 13, 2012

You can simply send a signal in the spec with Process.kill:

Process.kill :USR1, Process.pid
This implements #241.
Added a signal handler spec which may not be correct.
Contributor

steakknife commented Mar 14, 2012

This is probably wrong in 20 different ways, but here's a start.

@netzpirat netzpirat merged commit 5c60354 into guard:master Mar 14, 2012

Contributor

netzpirat commented Mar 14, 2012

Thanks for adding the specs, however they failed and the signal handling was not working at all. I had to move the method call to #add_signal_handlers outside of the listener thread to make it work.

In addition I merged your separate signals_spec.rb into the listener_spec.rb to follow the common lib/spec pattern and slim down the spec, since you tested a way too extensively: You don't need to test for combination of method call sequences, just each program execution path within the method (two signals, two conditions). The internal state of the listener is a simple @pause instance boolean and is not a state machine :P

Thanks for your effort.

Contributor

steakknife commented Mar 16, 2012

I've never heard a complaint about too much testing unless test time slowed down production deployments.

Often, the problem is lack of testing.

Be careful what you wish for.

Contributor

netzpirat commented Mar 16, 2012

There are also other aspects about tests: They have to be maintained by someone and they have to test the right thing.

Your #add_signal_handlers has four execution paths: two signal blocks and in each an if condition. This should result in four contexts to be specified, but your signal spec has 12 different contexts. So you're testing three times to much as necessary. When focusing on the important logic of the method, I actually see only two contexts, because the purpose of the two if is only informational.

In short, the added method does the following: SIGUSR1 => pause and SIGUSR2 => run. As I mentioned in my previous comment, you're testing different sequences of possible method calls. Why do you want to ensure that a second USR1 would result in something other than the first call? Do you want to test if the if statement really behaves the same on subsequent calls? The signal handlers are idempotent, you do not have to test the Ruby language constructs.

I really don't understand your reaction. It was you who said: "this is probably wrong in 20 different ways", and when I give you a honest feedback, you want to teach me in a childish way with your comment here and in #263. I really have no problem when someone teaches me, but it should be well-considered and constructive.

I also had the choice of simply not merge you changes, because it doesn't work as advertised and the provided specs are failing, but I decided to support your intention to add signal handlers, even though I don't need them for my personal workflow. And what do I get for spending my time to support you? A response in a disparaging way.

So long, and thanks for all the fish.

Owner

rymai commented Mar 18, 2012

@steakknife If you'd known @netzpirat personally, you wouldn't have answered him like that.

Please talk to him with the respect he deserves.

Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment