ccaum/puppet — comment on commit 0ec11238666fdfb2aed67ffb5e47649df5ff97a6
File:  line 0
By mmrobins:

Interesting that you've decided to generalize the retry stuff into Util. Is there somewhere else in the code that you think this would apply? It's nice if we're going to generalize a solution to actually use it in more than one place.

Either way, you could use some more testing. You don't have a single test that doesn't raise an error. There should also be a test that raises an error the first time, and doesn't on retry. You can do with with stubs by chaining returns on the end, or you could just use a variable inside the block that gets set the first time, then prevents the raise the second time.

Also, if we're going to have this generalized retry, it might be nice to have it give up based on the number of times tried instead of just time based. I see you're already keeping a failure count, so that shouldn't be hard to add.