Merge lp:~nskaggs/juju-ci-tools/add-sane-args-defaults into lp:juju-ci-tools

[Nicholas Skaggs]

Make juju_bin, temp_env_name and logs all as optional arguments with sane defaults

[Nicholas Skaggs (nskaggs)]

Reviewers note; I'm tempted to add a timestamp to the environment name or otherwise make it more unique. Ideas welcome. Would you find that useful or annoying (especially since it may not be the exact same ts as the logging folder)?

[Martin Packman (gz)]

The general idea of having sane defaults for the positional arguments I'm down with, I think we could even default 'env' to 'lxd' or something.

Given this changes add_basic_testing_arguments, TestAddBasicTestingArguments should be changed.

$ rm -rf /tmp/logs
$ touch /tmp/logs
$ make test
...
FAILED (failures=2, errors=37)

Some other comments inline.

review: Needs Fixing

[Aaron Bentley (abentley)]

It seems strange to provide a default for the juju under test. It is, after all, the thing we are testing, and if we don't want to specify it, we might as well not run the test at all.

[Nicholas Skaggs (nskaggs)]

Replies.

[Aaron Bentley (abentley)]

You don't seem to have replied to me.

[Nicholas Skaggs (nskaggs)]

@abentley, I suppose that is the crux of the argument for /usr/bin/juju vs $GOPATH/bin/juju. Odds are you care enough to specify. However, in the case of someone wanting to run the tests locally, they might not particularly care beyond "use the juju I have".

However, I went full bore on Martin's idea of going ahead and removing env too and making a sane default for it. I think the ability to just be able to run our tests is useful. For those of us who will actually attempt to do CI, we'll always want to specify everything, but we're presumably not the primary audience here (other than our localized test writing).

[Aaron Bentley (abentley)]

> However, in the case of
> someone wanting to run the tests locally, they might not particularly care
> beyond "use the juju I have".

Who is that person? Are they a juju developer?

If so, I think they are probably
1. writing a test for a feature they have written, or
2. running a test to make sure their changes don't break it

In both cases, they most likely have a custom binary that they want to test against.

So I still don't see who you're thinking would want to omit the path to the juju binary.

[Nicholas Skaggs (nskaggs)]

> > However, in the case of
> > someone wanting to run the tests locally, they might not particularly care
> > beyond "use the juju I have".
>
> Who is that person? Are they a juju developer?
Anyone who wishes to run tests locally; aka the maas guys, charm guys, openstack team, docs guys, juju developers. Granted ATM, few if any outside of our team run these tests locally.

> If so, I think they are probably
> 1. writing a test for a feature they have written, or
> 2. running a test to make sure their changes don't break it
>
> In both cases, they most likely have a custom binary that they want to test
> against.
I will grant you that indeed that a developer will have a custom binary in either case. Hence I was trying to phrase it as 'use the juju I have'. I won't pretend to know how many binaries developers tend to keep on there system; thus it depends on the developer if we can pick a sane default for them or not. It's likely they'll specify things.

If a developer has the juju binary they are hacking on set in a single place, it would be nice to just use that to enable that workflow, if indeed it exists. That would be an argument for $GOPATH. For the others listed above, I think /usr/bin/juju would be there expectation.

However, even if we assume developers and ourselves will always specify (and I agree that is likely), I do see value in having things just work when executed beyond enabling others. Removing all required arguments does that, so I would argue for adding the default on that basis alone. If we are left with only one argument, and we can reasonably remove it, let's do so. Lowering the barrier to running tests is the goal; albeit without breaking ourselves.

[Aaron Bentley (abentley)]

Additional temp_env_name comment.

[Nicholas Skaggs (nskaggs)]

Addressed uniqueness issue with temp_env_name

[Martin Packman (gz)]

Most changes seem fine, but we still have the test isolation issue from creating the logdir I pointed out before:

ubuntu@go:~/juju-ci-tools$ bzr st
modified:
  tests/test_utility.py
  utility.py
pending merge tips: (use -v to see all merge revisions)
  Nicholas Skaggs 2016-06-07 make temp_env_name truly unique

ubuntu@go:~/juju-ci-tools$ make test
...
FAILED (failures=2, errors=39)

ubuntu@go:~/juju-ci-tools$ bzr st
modified:
  tests/test_utility.py
  utility.py
unknown:
  baz/
  d/
  log/
  log_dir/
  test_utility_20160608144450_logs/
pending merge tips: (use -v to see all merge revisions)
  Nicholas Skaggs 2016-06-07 make temp_env_name truly unique

review: Needs Fixing

[Nicholas Skaggs (nskaggs)]

@mgz, I don't get any errors running this. I think you are pointing out I shouldn't be creating a folder during test runs; absolutely correct, so I've patched that out. I also will add some more tests to ensure __clean_dir() works as it should given I've modified it as well. Is there something else you are referring to?

[Martin Packman (gz)]

See my first comment for instructions. Tests should not fail based on the contents on /tmp.

[Nicholas Skaggs (nskaggs)]

I fixed everything in test_utility to use a temp dir instead of a hardcoded path. In a seperate MP I'll fix the rest of trunk with the same fix.

[Nicholas Skaggs (nskaggs)]

Ok, I did this in a simpler way -- only throw an exception if the directory doesn't exist, and we fail to create it. We continue on with warnings otherwise. Tests pass, new tests confirm my expectations.

[Nicholas Skaggs (nskaggs)]

This fixes the /tmp/logs problem by not raising the issue. The only remaining annoyance is -h or --help will create a directory and leave the warning (since the default functions still run). Also, running make test will spam blank folders in /tmp. Comments welcome.

[Christopher Lee (veebers)]

Some comments inline.

review: Needs Fixing

[Nicholas Skaggs (nskaggs)]

Replies inline.

[Christopher Lee (veebers)]

Comment re: readability

review: Needs Fixing

[Christopher Lee (veebers)]

Reply to comments

[Christopher Lee (veebers)]

LGTM

review: Approve

[Christopher Lee (veebers)]

> LGTM
Actually I take that back. I have concerns on calling --help creating a directory.

review: Needs Fixing

[lp:~nskaggs/juju-ci-tools/add-sane-args-defaults]

1469. By Nicholas Skaggs on 2016-07-11

fix for bug #1601987

1470. By Nicholas Skaggs on 2016-07-11

fix help test

1471. By Nicholas Skaggs on 2016-07-11

re-merge trunk

1472. By Nicholas Skaggs on 2016-07-12

remove directory creation as part of arg parsing. By default if nothing is specified, we will us the current directory

1473. By Nicholas Skaggs on 2016-07-21

move directory creation to bootstrap manager

1474. By Nicholas Skaggs on 2016-07-21

flake8

1475. By Nicholas Skaggs on 2016-07-21

patch out directory creation during no_args test

1476. By Nicholas Skaggs on 2016-07-21

restore _generate_default_clean_dir

Unmerged revisions

=== modified file 'tests/test_utility.py'
--- tests/test_utility.py	2016-05-26 16:28:21 +0000
+++ tests/test_utility.py	2016-07-07 21:48:09 +0000
@@ -11,6 +11,7 @@
 import logging
 import os
 import socket
+from tempfile import mkdtemp
 from time import time
 import warnings
 
@@ -361,6 +362,38 @@
 
 class TestAddBasicTestingArguments(TestCase):
 
+    def test_no_args(self):
+        cmd_line = []
+        parser = add_basic_testing_arguments(ArgumentParser())
+        with patch('utility.os.makedirs'):
+            with patch('utility.os.getenv', return_value=''):
+                args = parser.parse_args(cmd_line)
+        self.assertEqual(args.env, 'lxd')
+        self.assertEqual(args.juju_bin, '/usr/bin/juju')
+
+        logs_arg = args.logs.split("/")
+        logs_ts = logs_arg[4]
+        self.assertEqual(logs_arg[1:4], ['tmp', 'test_utility', 'logs'])
+        self.assertTrue(logs_ts, datetime.strptime(logs_ts, "%Y%m%d%H%M%S"))
+
+        temp_env_name_arg = args.temp_env_name.split("_")
+        temp_env_name_ts = temp_env_name_arg[2]
+        self.assertEqual(temp_env_name_arg[0:2], ['test', 'utility'])
+        self.assertTrue(temp_env_name_ts,
+                        datetime.strptime(temp_env_name_ts, "%Y%m%d%H%M%S"))
+        self.assertEqual(temp_env_name_arg[3:5], ['temp', 'env'])
+
+        self.assertEqual(logs_ts, temp_env_name_ts)
+
+    def test_default_binary(self):
+        cmd_line = []
+        with patch('utility.os.makedirs'):
+            with patch('utility.os.getenv', return_value='/tmp'):
+                with patch('utility.os.path.isfile', return_value=True):
+                    parser = add_basic_testing_arguments(ArgumentParser())
+                    args = parser.parse_args(cmd_line)
+        self.assertEqual(args.juju_bin, '/tmp/bin/juju')
+
     def test_positional_args(self):
         cmd_line = ['local', '/foo/juju', '/tmp/logs', 'testtest']
         parser = add_basic_testing_arguments(ArgumentParser())
@@ -408,6 +441,38 @@
             self.assertEqual(warned, [])
         self.assertEqual("", self.log_stream.getvalue())
 
+    def test_warn_on_nonexistent_directory_creation(self):
+        with warnings.catch_warnings(record=True) as warned:
+            log_dir = mkdtemp()
+            os.rmdir(log_dir)
+            cmd_line = ['local', '/foo/juju', log_dir, 'testtest']
+            parser = add_basic_testing_arguments(ArgumentParser())
+            parser.parse_args(cmd_line)
+            self.assertEqual(len(warned), 1)
+            self.assertRegexpMatches(
+                str(warned[0].message),
+                r"Not a directory " + log_dir)
+            self.assertEqual("", self.log_stream.getvalue())
+
+    def test_warn_on_directory_creation_failure(self):
+        with warnings.catch_warnings(record=True) as warned:
+            with patch('utility.os.makedirs', side_effect=OSError):
+                cmd_line = []
+                try:
+                    parser = add_basic_testing_arguments(ArgumentParser())
+                    parser.parse_args(cmd_line)
+                except OSError:
+                    # we catch our thrown OSError
+                    pass
+                else:
+                    self.fail('No exception thrown after' +
+                              ' directory creation failure')
+                self.assertEqual(len(warned), 1)
+                self.assertRegexpMatches(
+                    str(warned[0].message),
+                    r"Failed to create logging directory: /tmp/test_utility/" +
+                    "logs/.*. Please specify empty folder or try again")
+
     def test_debug(self):
         cmd_line = ['local', '/foo/juju', '/tmp/logs', 'testtest', '--debug']
         parser = add_basic_testing_arguments(ArgumentParser())
 
=== modified file 'utility.py'
--- utility.py	2016-06-27 11:41:04 +0000
+++ utility.py	2016-07-07 21:48:09 +0000
@@ -104,18 +104,20 @@
     try:
         contents = os.listdir(maybe_dir)
     except OSError as e:
-        if e.errno != errno.ENOENT:
-            raise
-        # GZ 2016-03-01: We may want to raise or just create the dir here, but
-        # that confuses expectations of all existing parse_args tests.
+        if e.errno == errno.ENOENT:
+            # we don't raise this error due to tests abusing /tmp/logs
+            warnings.warn('Not a directory {}'.format(maybe_dir))
+        if e.errno == errno.EEXIST:
+            warnings.warn('Directory {} already exists'.format(maybe_dir))
     else:
         if contents and contents != ["empty"]:
-            warnings.warn("Directory %r has existing contents." % (maybe_dir,))
+            warnings.warn(
+                'Directory {!r} has existing contents.'.format(maybe_dir))
     return maybe_dir
 
 
 def pause(seconds):
-    print_now('Sleeping for %d seconds.' % seconds)
+    print_now('Sleeping for {:d} seconds.'.format(seconds))
     sleep(seconds)
 
 
@@ -194,7 +196,7 @@
         except socket.gaierror as e:
             print_now(str(e))
         except Exception as e:
-            print_now('Unexpected %r: %s' % (type(e), e))
+            print_now('Unexpected {!r}: {}'.format((type(e), e)))
             raise
         else:
             conn.close()
@@ -290,14 +292,59 @@
         return subprocess.check_output(command)
 
 
+def _get_test_name_from_filename():
+    try:
+        calling_file = sys._getframe(2).f_back.f_globals['__file__']
+        return os.path.splitext(os.path.basename(calling_file))[0]
+    except:
+        return 'unknown_test'
+
+
+def _generate_default_clean_dir(timestamp):
+    """Creates a new unique directory for logging and returns the name"""
+    log_dir = os.path.join('/tmp', _get_test_name_from_filename(),
+                           'logs', timestamp)
+    try:
+        os.makedirs(log_dir)
+        warnings.warn('"Created logging directory {}'.format(log_dir))
+    except OSError as e:
+        if e.errno == errno.EEXIST:
+            pass
+        else:
+            warnings.warn('Failed to create logging directory: ' +
+                          log_dir +
+                          ". Please specify empty folder or try again")
+            raise
+    return log_dir
+
+
+def _generate_default_temp_env_name(timestamp):
+    """Creates a new unique name for environment and returns the name"""
+    return '{}_{}_temp_env'.format(_get_test_name_from_filename(), timestamp)
+
+
+def _generate_default_binary():
+    """Returns GOPATH juju binary if it exists, otherwise /usr/bin/juju"""
+    if os.getenv('GOPATH'):
+        go_bin = os.getenv('GOPATH') + '/bin/juju'
+        if os.path.isfile(go_bin):
+            return go_bin
+
+    return '/usr/bin/juju'
+
+
 def add_basic_testing_arguments(parser, using_jes=False):
     """Returns the parser loaded with basic testing arguments.
 
     The basic testing arguments, used in conjuction with boot_context ensures
     a test can be run in any supported substrate in parallel.
 
-    This helper adds 4 positional arguments that define the minimum needed
-    to run a test script: env, juju_bin, logs, and temp_env_name.
+    This helper adds 4 positional arguments that defines the minimum needed
+    to run a test script.
+
+    These arguments (env, juju_bin, logs, temp_env_name) allow you to specify
+    specifics for which env, juju binary, which folder for logging and an
+    environment name for your test respectively.
 
     There are many optional args that either update the env's config or
     manipulate the juju command line options to test in controlled situations
@@ -308,16 +355,32 @@
     :param parser: an ArgumentParser.
     :param using_jes: whether args should be tailored for JES testing.
     """
-    # Required positional arguments.
-    parser.add_argument(
-        'env',
-        help='The juju environment to base the temp test environment on.')
-    parser.add_argument(
-        'juju_bin', help='Full path to the Juju binary.')
-    parser.add_argument(
-        'logs', type=_clean_dir, help='A directory in which to store logs.')
-    parser.add_argument(
-        'temp_env_name', help='A temporary test environment name.')
+
+    # Optional postional arguments
+    # generate timestamp for use with default args
+    timestamp = datetime.now().strftime("%Y%m%d%H%M%S")
+    parser.add_argument(
+        'env', nargs='?',
+        help='The juju environment to base the temp test environment on.',
+        default='lxd')
+    parser.add_argument('juju_bin', nargs='?',
+                        help='Full path to the Juju binary. By default, this'
+                        ' will use $GOPATH/bin/juju or /usr/bin/juju in that'
+                        ' order.',
+                        default=_generate_default_binary())
+    parser.add_argument('logs',  nargs='?',  type=_clean_dir,
+                        help='A directory in which to store logs. By default,'
+                        ' this will generate a directory named after the test'
+                        ' and under /tmp, with a timestamp of the run. '
+                        ' /tmp/test_name/logs/timestamp',
+                        default=_generate_default_clean_dir(timestamp))
+    parser.add_argument('temp_env_name', nargs='?',
+                        help='A temporary test environment name. By default, '
+                        ' this will generate an enviroment name using the '
+                        ' timestamp and testname. '
+                        ' test_name_timestamp_temp_env',
+                        default=_generate_default_temp_env_name(timestamp))
+
     # Optional keyword arguments.
     parser.add_argument('--debug', action='store_true',
                         help='Pass --debug to Juju.')