Export add integration test

[msabramo]

This PR:

• Adds integration tests for the export functionality (4b33c2c)
• Tweaks a few things so that tests pass consistently. E.g.:
  • Sort dictionary keys to get consistent ordering from run to run (e32be90)
  • Use items in place of iteritems in upstart template for Python 3 compatibility (3e9c2d9)
  • Apply fix from https://github.com/nickstenning/honcho/pull/102 so that we don't get spurious test failures that confuse things. (5148677)

$ tox -e py26,py27,py32,py33,py34,lint
...
  py26: commands succeeded
  py27: commands succeeded
  py32: commands succeeded
  py33: commands succeeded
  py34: commands succeeded
  lint: commands succeeded
  congratulations :)

[nickstenning]

I'm actually not a fan of the integration tests we currently have. They're far too detailed and actually make it much harder to iterate on the internals of honcho.

I've been planning to strip them back for ages but never got round to it.

I'm happy to add an integration test for export, but I think this is overly detailed. A good level of integration test would be something like:

• run honcho export for upstart to a tmpdir (like you're doing)
• check that the right files exist

Checking the contents of the files is probably out of scope and would be better achieved in a unit test.

[msabramo]

OK, thanks for the feedback. That does make things considerably easier.

[msabramo]

@nickstenning: Thanks for the comments!

Checking the contents of the files is probably out of scope and would be better achieved in a unit test.

OK, I just rebased so that the integration test only tests that the files exist and does not check the file contents.

Please rereview when you get a chance.

[msabramo]

$ tox -e py26,py27,py32,py33,py34,lint
...
  py26: commands succeeded
  py27: commands succeeded
  py32: commands succeeded
  py33: commands succeeded
  py34: commands succeeded
  lint: commands succeeded
  congratulations :)

[nickstenning]

Looking better. Get rid of the tests of the CLI parsing, too. There's no need for us to test argparse :)

[msabramo]

Starting to feel guilty now about making my tests less rigorous... :smile:

Aren't the contents of the generated files part of the visible behavior of the system (i.e.: not implementation details) and this something that should be tested?

It sounds like you've had some pain with honcho and fragile tests so you can possibly talk me down from the ledge ... :smile:

[nickstenning]

Aren't the contents of the generated files part of the visible behavior of the system (i.e.: not implementation details) and this something that should be tested?

Yes, they are, but if we need to test the export functionality, then it should be with unit tests, not integrated tests. There are simply too many moving parts here for integrated testing to be a good idea. We are simultaneously testing:

• argparse
• the command-line interface of honcho
• the export package
• the contents of the templates

If any one of these items changes, the integrated tests will likely all start failing.

So, in this case, the job of writing templates to files is done in honcho.export.base. We don't currently have unit tests for this module, but we should. Testing that honcho.export.base.BaseExport correctly writes files can be done in isolation from the rest of the system.

Once that's done, there's little value in testing for the content of the files in an integrated test, because either the template is wrong (let's just fix the template) or the unit tests are insufficient.

I'm not arguing against testing, I'm just arguing against tests that touch so much of the system at once.

(You might enjoy this talk on this subject.)

[msabramo]

Ok, removed the usage message test in https://github.com/msabramo/honcho/commit/d24185d3fac17c38a63029fd285ef81a0dd84042

[nickstenning]

Still not sure about the CLI tests, but I'll have a look at that another time! Thanks for this.

[slafs]

I think the assertions here are too restrictive. For example notice that the test_export_unknown_format test will fail locally (using nosetest without tox) when you have a custom exporter installed on your sys.path . This is because choices for an export format are generated dynamically. In this case the end of an error message will look something like this:

...
(choose from 'myexport', 'supervisord', 'upstart')