exit status
dan-manges opened this issue · 8 comments
Should Hydra tasks exit with a non-0 exit status if tests fail? We are relying on that for our CI builds. Putting this at the end of Master#initialize fixed the problem for us, although it's quite hacky.
failed = @event_listeners.any? { |l| l.instance_variable_get("@errors") }
exit(failed ? 1 : 0)
Hey Dan,
Yeah this is a known issue! We don't use hydra for CI, so I haven't built it in yet.
If you implement something you're happy with and are willing to share, I'll pull in the changes.
Do you have preferences for the implementation and the testing?
Regarding implementation, the exit could go in the Hydra::Task or in Hyrda::Master. rspec puts the exit in the at_exit hook, but Hydra isn't using that approach. If it goes in Hydra::Task, we would need Hydra::Master to return the exit code. This would require moving the logic from initialize to another method since initialize is only going to return the instance -- unless the instance has some sort of state on it that the task can check. But that might be a good change anyway, since typically you expect initialize to only instantiate an object and not to have so much behavior.
Regarding testing, if the exit goes in the task there could be a test that shells out to run a predefined hydra task that fails to make sure the exit status is 1. It looks like there aren't any tests for the tasks right now though. If the exit goes in Hydra::Master, then either every test needs to handle it, or the exit needs to go in a separate method so that most tests don't need to deal with it. For handling the exit, it could either be stubbed, or we could wrap the logic in a fork and check the exit status of the forked process.
Any suggestions? We're happy to contribute a patch if you can give us some direction.
I like the idea of pulling the run-logic out of initialize, and leaving initialize to be configuration. Then having run return the exit status. Then have the task return that exit status.
Please don't use at_exit. Undoing at_exit was 50% of the work of hydra. It sucks.
Other than that all your ideas above make a lot of sense! Go for it!
http://github.com/braintreeps/hydra/commit/e74b9ef4b4d0530d7a1f8f25a534aa9ddc573097
I had problems running the entire test suite, if you could double check that everything passes. I set up ssh so that I could 'ssh localhost' with pubkey auth, but a couple tests were still failing (before making my changes). I ended up having run return true/false instead of the exit status. Testing tools normally exit with an exit code, but they're usually run in a separate process by rake using sh. Since Hydra isn't implemented that way, I thought it would be better to return true or false from run and then have rake fail if it's false, since we don't want Rake to exit 0 it tests pass.
It looks like the only way in Hydra to know if a test fails is to see if output == "." in the listeners - is that correct? If so, I pulled up error detection into the abstract listener and updated subclasses to call super.
Let me know how this looks.
Yeah checking for output == "." is correct. It's hacky but it's the best I could do between test suites.
I'll check out your changes when I get a chance and see if the test suite runs for me. (Note, this may be a while, I have a lot of pending hydra stuff to get to).
-Nick
Hi Nick- have you had a chance to check this out yet?
I have not.
I have a lot on my plate at work, and I am speaking at 6 conferences in the next two months, so my weekends are really booked too.
Once the conferences get going I may hack on hydra at the conferences. Also, if you are attending any, we can work on it together.
Lone Star Ruby Conf
Acts As Conference
Windy City Rails
Central PA Open Source
Ruby Hoedown
-Nick
Hydra Version 0.22.1 returns the proper exit status when there are failing/erroring tests.
Enjoy!