Merge lp:~dpb/charm-tools/fix-test-env-1271745 into lp:charm-tools/1.2

[David Britton]

Simple fix to import current environment into the test conductor. Without doing this, at least the following things break for me:

- I lose connection to the ssh agent
- I don't get a working path so things like "juju ssh service/0" stop working

I included a test case that breaks if my code change is not in place.

Sorry for the bug number in the branch name, it's a different bug with a similar title, ignore it. This is to fix bug #1271803

[Marco Ceppi (marcoceppi)]

I don't like that this sets /all/ the environment variables. I'd rather there be a whitelist of environment variables to pass to the tests as the testing environment will have very limited scope of env variables.

review: Needs Fixing

[lp:~dpb/charm-tools/fix-test-env-1271745]

310. By David Britton on 2014-01-24

limit to whitelist of env variables.

[David Britton (dpb)]

On Fri, Jan 24, 2014 at 3:47 PM, Marco Ceppi <email address hidden> wrote:

> Review: Needs Fixing
>
> I don't like that this sets /all/ the environment variables. I'd rather
> there be a whitelist of environment variables to pass to the tests as the
> testing environment will have very limited scope of env variables.
>

Fixed with corresponding test cases.

--
David Britton <email address hidden>

[Marco Ceppi (marcoceppi)]

LGTM +1 thank you

review: Approve

[lp:~dpb/charm-tools/fix-test-env-1271745]

311. By David Britton on 2014-01-28

Merging up

=== modified file 'charmtools/test.py'
--- charmtools/test.py	2014-01-22 23:06:10 +0000
+++ charmtools/test.py	2014-01-28 23:58:52 +0000
@@ -25,6 +25,7 @@
 
 LOG_LEVELS = [logging.INFO, logging.DEBUG]
 TEST_RESULT_LEVELV_NUM = 51
+ENV_WHITELIST = ('PATH', 'SSH_AUTH_SOCK', 'SSH_AGENT_PID')
 
 
 class NoTests(Exception):
@@ -64,6 +65,9 @@
     def __init__(self, arguments=None):
         self.args = arguments
         self.env = {'JUJU_HOME': os.path.expanduser('~/.juju')}
+        for var in ENV_WHITELIST:
+            if var in os.environ:
+                self.env[var] = os.environ[var]
         self.log = logging.getLogger('juju-test.conductor')
         self.tests = self.find_tests()
         self.tests_requested = self.args.tests

=== modified file 'tests/test_juju_test.py'
--- tests/test_juju_test.py	2014-01-21 06:01:15 +0000
+++ tests/test_juju_test.py	2014-01-28 23:58:52 +0000
@@ -115,6 +115,24 @@
             args = Arguments(tests="dummy")
             self.assertRaises(juju_test.NoTests, juju_test.Conductor, args)
 
+    def test_conductor_ignores_arbitrary_env(self):
+        """Ensure that the conductor ignores other env vars."""
+        os.environ['CHARMTOOLS_TEST'] = 'FOOBAR';
+        args = Arguments(tests="dummy")
+        c = juju_test.Conductor(args)
+        self.assertNotIn('CHARMTOOLS_TEST', c.env)
+
+    def test_conductor_keeps_whitelist_env(self):
+        """Ensure that the conductor copies the environment whitelist."""
+        os.environ['PATH'] = 'FOOBAR';
+        os.environ['SSH_AGENT_PID'] = 'FOOBAR';
+        os.environ['SSH_AUTH_SOCK'] = 'FOOBAR';
+        args = Arguments(tests="dummy")
+        c = juju_test.Conductor(args)
+        self.assertEqual('FOOBAR', c.env['PATH'])
+        self.assertEqual('FOOBAR', c.env['SSH_AGENT_PID'])
+        self.assertEqual('FOOBAR', c.env['SSH_AUTH_SOCK'])
+
     @patch.object(juju_test.Conductor, 'find_tests')
     def test_conductor_find_tests_exception(self, mfind_tests):
         mfind_tests.return_value = None