btm / minitest-handler-cookbook

Apache License 2.0
59 stars 36 forks source link

error codes #2

Closed jperry closed 12 years ago

jperry commented 12 years ago

Hi,

I am running minitest-handler and was wondering if tests fail should the report handlers be reporting an error code? I ask because I am using vagrant to provision a machine and I have tests fail but vagrant doesn't report back a failure. Should this happen? Are the test results put anywhere? I need this for a continuous integration environment where the return codes are important.

Thanks, Jay

acrmp commented 12 years ago

Hi Jay,

I'm doing some work for Opscode on testing cookbooks at the moment and hit the same problem.

By default exception and reporting handlers swallow any errors, so the exit code is never set to a non-zero result when tests fail. https://github.com/opscode/chef/blob/0.10.8/chef/lib/chef/handler.rb#L201

I've implemented a couple of approaches, neither of which are very elegant. You can see both on my fork of minitest-chef-handler here: https://github.com/kotiri/minitest-chef-handler/commit/71fc2c94398e9ffc2eafeb8bdc37e24ef5797296

  1. Override run_report_safely which all Chef::Handler subclasses inherit so that it doesn't catch the exception. The downside here is other handlers may not be run as it will exit immediately.
  2. Monkey-patch Chef::Client to allow the exit code to be set from the handler. This is safer but a lot more invasive and likely to break in the future.

A third approach which I haven't taken is to move the MiniTest run into the main converge (rather than running as a handler). The downside here is you can't ensure your tests are running after the converge and all notifications have completed.

If the minitest-chef-handler is only going to be used in CI then I'd probably suggest the first approach for now.

Cheers,

Andrew.

jperry commented 12 years ago

Hi Andrew,

The latest version of CI server which we'll be installing in a few weeks has the ability to fail a build based on keywords or patterns in the output so I think I may just rely on that for now. Or have my rakefile output to stdout and a file and parse the file for failed tests. Do you know if there is a way to have the tests write to a file through minitest-handler or through minitest?

Thanks, Jay

acrmp commented 12 years ago

MiniTest does let you re-assign the output stream: https://github.com/seattlerb/minitest/blob/e3b8ef877855de0f47193801eb07833e5c034472/lib/minitest/unit.rb#L719-725

jperry commented 12 years ago

This would probably mean changing the minitest-handler-cookbook though to call minitest handler with those options right?

acrmp commented 12 years ago

It's a class variable so you should be able to call it whenever (from another cookbook if you prefer) and the handler should use it. It's not the cleanest approach though.

jperry commented 12 years ago

Yeah if for some reason upgrading our ci server is delayed I may have to take on the approaches above. Thanks for the responses.

bryanwb commented 12 years ago

I know that Jim Hopp and Max Horbul were working on this issue at chefconf. I believe that Max resolved it somehow. likely the code is in here https://github.com/mhorbul/minitest-chef-handler-ci-integration. I will check w/ max directly

jperry commented 12 years ago

We upgraded our CI server so it now supports failing the build based on certain messages found in the output. The next thing would be able to get minitest-reporters working with minitest-chef-handler since we are using TeamCity.

acrmp commented 12 years ago

Just a note that this should now be working out of the box as minitest-chef-handler was updated in version 0.5.1 (courtesy of @cgriego) to exit with a non-zero exit code if there are any test failures: https://github.com/calavera/minitest-chef-handler/blob/v0.5.1/History.txt

I haven't done any investigation to see how well this plays with other handlers but it will fail the build without the need for additional CI server config.

Cheers,

Andrew.

jperry commented 12 years ago

Great, I'll try out that version.

Thanks.

bryanwb commented 12 years ago

closed as upstream minitest-chef-handler has added support for this feature