Merge lp:~benji/lpsetup/bug-1017973-add-smoke-tests into lp:lpsetup

[Benji York]

add a basic smoke test for the subcommands

This branch adds a basic smoke test for the subcommands by running each with --help and asserting that no exceptions are raised and a non-error (zero) exit code is returned.

A little refactoring was required to get the tests to work, which was discussed with Francesco by way of mid-implementation call.

Lint: pocketlint reports no lint

Tests: the obvious test was added and I also tested the test by introducing SyntaxError and NameError in subcommand code and verified that the new test failed.

[Francesco Banconi (frankban)]

Benji, the code looks good, and thanks for your changes to the previous weird exception handling in *StepsBasedSubcommand.handle*.

Just an observation:

54 + try:
55 + args = parser.parse_args(args)
56 + except SystemExit:
57 + return 0

Now the main function is consistent and just returns an error, an exit code or None, and that's great. However, in my opinion we should return the error or the exit code here too, otherwise lp-setup exits with 0 even if wrong arguments are provided. Something like::

    except SystemExit as err:
        return err # or err.code

review: Approve

[Benji York (benji)]

> Now the main function is consistent and just returns an error, an exit code or
> None, and that's great. However, in my opinion we should return the error or
> the exit code here too, otherwise lp-setup exits with 0 even if wrong
> arguments are provided. Something like::
>
> except SystemExit as err:
> return err # or err.code

Indeed! I have made that change (using err.code).

[Brad Crittenden (bac)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Brad Crittenden (bac)]

Attempt to merge into lp:lpsetup failed due to conflicts:

text conflict in lpsetup/tests/test_argparser.py

[Brad Crittenden (bac)]

The attempt to merge lp:~benji/lpsetup/bug-1017973-add-smoke-tests into lp:lpsetup failed. Below is the output from the failed tests.

nose.plugins.cover: ERROR: Coverage not available: unable to import coverage module
...............................usage: nosetests [-h] {subcmd,help} ...
nosetests: error: unrecognized arguments: --foo eggs
E...........................................
======================================================================
ERROR: test_failing_step (lpsetup.tests.test_argparser.StepsBasedSubCommandWithErrorsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/var/cache/tarmac/lpsetup/trunk/lpsetup/tests/test_argparser.py", line 174, in test_failing_step
    self.parse_and_call_main('--foo eggs')
  File "/var/cache/tarmac/lpsetup/trunk/lpsetup/tests/utils.py", line 72, in parse_and_call_main
    namespace = self.parse(*args)
  File "/var/cache/tarmac/lpsetup/trunk/lpsetup/tests/utils.py", line 64, in parse
    namespace = self.parser.parse_args((self.sub_command_name,) + args)
  File "/var/cache/tarmac/lpsetup/trunk/lpsetup/argparser.py", line 157, in parse_args
    return super(ArgumentParser, self).parse_args(*args, **kwargs)
  File "/usr/lib/python2.7/argparse.py", line 1691, in parse_args
    self.error(msg % ' '.join(argv))
  File "/usr/lib/python2.7/argparse.py", line 2347, in error
    self.exit(2, _('%s: error: %s\n') % (self.prog, message))
  File "/usr/lib/python2.7/argparse.py", line 2335, in exit
    _sys.exit(status)
SystemExit: 2

----------------------------------------------------------------------
Ran 75 tests in 0.636s

FAILED (errors=1)

[Brad Crittenden (bac)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'lpsetup/argparser.py'
--- lpsetup/argparser.py	2012-06-26 14:56:11 +0000
+++ lpsetup/argparser.py	2012-06-26 18:36:19 +0000
@@ -489,7 +489,4 @@
             # Run the step using a dynamic dispatcher.
             step_runner = getattr(
                 self, 'call_' + step_name, default_step_runner)
-            try:
-                step_runner(namespace, step, args)
-            except subprocess.CalledProcessError as err:
-                return err
+            step_runner(namespace, step, args)

=== modified file 'lpsetup/cli.py'
--- lpsetup/cli.py	2012-06-22 20:50:41 +0000
+++ lpsetup/cli.py	2012-06-26 18:36:19 +0000
@@ -9,6 +9,7 @@
     'main',
     ]
 
+import sys
 from lpsetup import (
     __doc__ as description,
     argparser,
@@ -24,17 +25,27 @@
     version,
     )
 
-
-def main():
+subcommands = [
+    ('install', install.SubCommand),
+    ('inithost', inithost.SubCommand),
+    ('get', get.SubCommand),
+    ('update', update.SubCommand),
+    ('lxc-install', lxcinstall.SubCommand),
+    ('branch', branch.SubCommand),
+    ('version', version.SubCommand),
+    ]
+
+
+def main(args=None):
+    if args is None:
+        args = sys.argv[1:]
     parser = argparser.ArgumentParser(description=description)
-    parser.register_subcommand('install', install.SubCommand)
-    parser.register_subcommand('inithost', inithost.SubCommand)
-    parser.register_subcommand('get', get.SubCommand)
-    parser.register_subcommand('update', update.SubCommand)
-    parser.register_subcommand('lxc-install', lxcinstall.SubCommand)
-    parser.register_subcommand('branch', branch.SubCommand)
-    parser.register_subcommand('version', version.SubCommand)
-    args = parser.parse_args()
+    for subcommand, callable in subcommands:
+        parser.register_subcommand(subcommand, callable)
+    try:
+        args = parser.parse_args(args)
+    except SystemExit as err:
+        return err.code
     try:
         return args.main(args)
     except (exceptions.ExecutionError, KeyboardInterrupt) as err:

=== modified file 'lpsetup/tests/subcommands/test_inithost.py'
--- lpsetup/tests/subcommands/test_inithost.py	2012-06-26 10:17:27 +0000
+++ lpsetup/tests/subcommands/test_inithost.py	2012-06-26 18:36:19 +0000
@@ -46,5 +46,3 @@
         )
     expected_steps = (initialize_step, setup_apt_step)
     needs_root = True
-
-

=== modified file 'lpsetup/tests/subcommands/test_install.py'
--- lpsetup/tests/subcommands/test_install.py	2012-06-26 10:17:27 +0000
+++ lpsetup/tests/subcommands/test_install.py	2012-06-26 18:36:19 +0000
@@ -51,5 +51,3 @@
         setup_launchpad_step,
         )
     needs_root = True
-
-

=== added file 'lpsetup/tests/subcommands/test_smoke.py'
--- lpsetup/tests/subcommands/test_smoke.py	1970-01-01 00:00:00 +0000
+++ lpsetup/tests/subcommands/test_smoke.py	2012-06-26 18:36:19 +0000
@@ -0,0 +1,21 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Smoke tests for the sub commands."""
+
+import unittest
+
+from lpsetup.cli import (
+    main,
+    subcommands,
+    )
+
+
+class SmokeTest(unittest.TestCase):
+    """Tests which are intended to do a quick check for broken subcommands."""
+
+    def test_subcommand_smoke_via_help(self):
+        # Perform a basic smoke test by running each subcommand's --help
+        # function and verify that a non-error exit code is returned.
+        for subcommand, callable in subcommands:
+            self.assertEqual(main([subcommand, '--help']), 0)

=== modified file 'lpsetup/tests/test_argparser.py'
--- lpsetup/tests/test_argparser.py	2012-06-26 10:17:27 +0000
+++ lpsetup/tests/test_argparser.py	2012-06-26 18:36:19 +0000
@@ -4,6 +4,7 @@
 
 """Tests for the argparser module."""
 
+import subprocess
 import unittest
 
 from lpsetup import argparser
@@ -169,10 +170,8 @@
     def test_failing_step(self):
         # Ensure the steps execution is stopped if a step raises
         # `subprocess.CalledProcessError`.
-        with capture_output() as output:
-            error = self.parse_and_call_main('--foo', 'eggs')
-        self.assertEqual(1, error.returncode)
-        self.check_output(['step1 received eggs'], output)
+        with self.assertRaises(subprocess.CalledProcessError):
+            self.parse_and_call_main('--foo', 'eggs')
 
 
 class DynamicStepsBasedSubCommandTest(SubCommandTestMixin, unittest.TestCase):

=== modified file 'setup.py'
--- setup.py	2012-06-25 21:17:38 +0000
+++ setup.py	2012-06-26 18:36:19 +0000
@@ -22,12 +22,13 @@
 data_files = []
 for dirpath, dirnames, filenames in os.walk(project_name):
     for i, dirname in enumerate(dirnames):
-        if dirname.startswith('.'): del dirnames[i]
+        if dirname.startswith('.'):
+            del dirnames[i]
     if '__init__.py' in filenames:
         continue
     elif filenames:
         for f in filenames:
-            data_files.append(os.path.join(dirpath[len(project_name)+1:], f))
+            data_files.append(os.path.join(dirpath[len(project_name) + 1:], f))
 
 project = __import__(project_name)
 long_description = open(os.path.join(root_dir, 'README.rst')).read()