jamtur01/puppet — comment on commit 1bf0a8c69f5597e4242c19fb125ed4324e1ceeb5
File:  line 0
By daniel-pittman:

The tests you have are good, but they don't cover the behaviour of the external script:
* what happens when the external script fails?
* what happens when the external script is `sleep 900`?
* what happens when the external script prints to STDOUT, and to STDERR?

You also don't test all the current permutations of invocation - I don't see testing of `failonfail` true and false differences in behaviour there. (I think that feature should go, but if it stays, it should be tested.)

I don't see tests that the places invoking the alert do the right thing - the public API needs more testing.

Otherwise, with some polish and a bit more thought about how it interacts with the logging subsystem, seems like a reasonable thing to me. We should probably run this around UX a bit and see what they think.