Merge lp:~vila/bzr/690563-better-env-isolation into lp:bzr

[Vincent Ladeuil]

This just shows why _captureVar is not how we should override environment variables for tests.

I'd like feedback on the name before deploying it in the test code base.

I chose overrideEnv as it carries the same semantic that overrideAttr and alternatives didn't work for that (setenv, set_or_unset_env_var, etc).

[Vincent Ladeuil (vila)]

I haven't fully investigated all usages but at least caught my eye:
  _captureVar('TERM', None)

Since TERM is one of the variables reset by TestCase, doing the above means that TERM is lost (as shown in the test in this mp).

[Martin Pool (mbp)]

spelled 'isolation'

I agree this name is better.

Rather than poking it into an instance attribute, perhaps we should register a cleanup? But I suppose that can't be done while there is other code depending on it.

It would be a bit more satisfying if this also cleaned up the existing callers but I'm fine with that being done separately.

review: Approve

[Martin Packman (gz)]

Agreed that _captureVar is dodgy and we want a public interface that does the right thing instead. Will need to fiddle with these environment things further to fix bug 321320, as doctests can't use TestCase methods to provide isolation.

+ if name in self._old_env:
+ # We already protect this variable so we should not record its
+ # initial value
+ osutils.set_or_unset_env(name, value)
+ else:
+ self._old_env[name] = osutils.set_or_unset_env(name, value)

I'd probably spell this in a positive way instead:

    old_value = osutils.set_or_unset_env(name, value)
    # Save the original value provided it's not already been recorded
    if name not in self._old_env:
        self._old_env[name] = old_value

review: Approve

[Vincent Ladeuil (vila)]

>>>>> Martin [gz] <email address hidden> writes:

    > Review: Approve
    > Agreed that _captureVar is dodgy and we want a public interface that does the right thing instead. Will need to fiddle with these environment things further to fix bug 321320, as doctests can't use TestCase methods to provide isolation.

    > + if name in self._old_env:
    > + # We already protect this variable so we should not record its
    > + # initial value
    > + osutils.set_or_unset_env(name, value)
    > + else:
    > + self._old_env[name] = osutils.set_or_unset_env(name, value)

    > I'd probably spell this in a positive way instead:

    > old_value = osutils.set_or_unset_env(name, value)
    > # Save the original value provided it's not already been recorded
    > if name not in self._old_env:
    > self._old_env[name] = old_value

Right, and even:

        old_value = osutils.set_or_unset_env(name, value)
        self.addCleanup(osutils.set_or_unset_env, name, old_value)

which is cleaner and clearer. Too bad os.environ is not a regular dict
or we could have used overrideAttr instead.

--367D41813B0.1292493572/axe--

=== modified file 'bzrlib/plugins/launchpad/test_lp_directory.py'
--- bzrlib/plugins/launchpad/test_lp_directory.py	2010-12-03 11:16:25 +0000
+++ bzrlib/plugins/launchpad/test_lp_directory.py	2010-12-16 17:48:29 +0000
@@ -298,7 +298,7 @@
         self.server = self.server_class()
         self.server.start_server()
         # Ensure we don't clobber env
-        self._captureVar('BZR_LP_XMLRPC_URL', None)
+        self.overrideEnv('BZR_LP_XMLRPC_URL', None)
 
     def tearDown(self):
         self.server.stop_server()

=== modified file 'bzrlib/plugins/launchpad/test_lp_service.py'
--- bzrlib/plugins/launchpad/test_lp_service.py	2010-11-26 17:33:55 +0000
+++ bzrlib/plugins/launchpad/test_lp_service.py	2010-12-16 17:48:29 +0000
@@ -32,7 +32,7 @@
     def setUp(self):
         super(LaunchpadServiceTests, self).setUp()
         # make sure we have a reproducible standard environment
-        self._captureVar('BZR_LP_XMLRPC_URL', None)
+        self.overrideEnv('BZR_LP_XMLRPC_URL', None)
 
     def test_default_service(self):
         service = LaunchpadService()

=== modified file 'bzrlib/plugins/launchpad/test_register.py'
--- bzrlib/plugins/launchpad/test_register.py	2010-12-07 09:06:39 +0000
+++ bzrlib/plugins/launchpad/test_register.py	2010-12-16 17:48:29 +0000
@@ -137,7 +137,7 @@
     def setUp(self):
         super(TestBranchRegistration, self).setUp()
         # make sure we have a reproducible standard environment
-        self._captureVar('BZR_LP_XMLRPC_URL', None)
+        self.overrideEnv('BZR_LP_XMLRPC_URL', None)
 
     def test_register_help(self):
         """register-branch accepts --help"""
@@ -296,7 +296,7 @@
     def setUp(self):
         super(TestGatherUserCredentials, self).setUp()
         # make sure we have a reproducible standard environment
-        self._captureVar('BZR_LP_XMLRPC_URL', None)
+        self.overrideEnv('BZR_LP_XMLRPC_URL', None)
 
     def test_gather_user_credentials_has_password(self):
         service = LaunchpadService()

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-12-16 14:39:57 +0000
+++ bzrlib/tests/__init__.py	2010-12-16 17:48:29 +0000
@@ -851,6 +851,7 @@
         self.addDetail("log", content.Content(content.ContentType("text",
             "plain", {"charset": "utf8"}),
             lambda:[self._get_log(keep_log_file=True)]))
+        self._old_env = {}
         self._cleanEnvironment()
         self._silenceUI()
         self._startLogFile()
@@ -1539,6 +1540,20 @@
             setattr(obj, attr_name, new)
         return value
 
+    def overrideEnv(self, name, new):
+        """Set an environment variable, and reset it fater the test.
+
+        :param name: The environment variable name.
+
+        :param new: The value to set the variable to. If None, the 
+            variable is deleted from the environment.
+
+        :returns: The actual variable value.
+        """
+        value = osutils.set_or_unset_env(name, new)
+        self.addCleanup(osutils.set_or_unset_env, name, value)
+        return value
+
     def _cleanEnvironment(self):
         new_env = {
             'BZR_HOME': None, # Don't inherit BZR_HOME to all the tests.
@@ -1588,19 +1603,13 @@
             # use an env var so it propagates to subprocesses.
             'APPORT_DISABLE': '1',
         }
-        self._old_env = {}
-        self.addCleanup(self._restoreEnvironment)
         for name, value in new_env.iteritems():
-            self._captureVar(name, value)
+            self.overrideEnv(name, value)
 
     def _captureVar(self, name, newvalue):
         """Set an environment variable, and reset it when finished."""
         self._old_env[name] = osutils.set_or_unset_env(name, newvalue)
 
-    def _restoreEnvironment(self):
-        for name, value in self._old_env.iteritems():
-            osutils.set_or_unset_env(name, value)
-
     def _restoreHooks(self):
         for klass, (name, hooks) in self._preserved_hooks.items():
             setattr(klass, name, hooks)
@@ -2442,8 +2451,8 @@
         test_home_dir = self.test_home_dir
         if isinstance(test_home_dir, unicode):
             test_home_dir = test_home_dir.encode(sys.getfilesystemencoding())
-        os.environ['HOME'] = test_home_dir
-        os.environ['BZR_HOME'] = test_home_dir
+        self.overrideEnv('HOME', test_home_dir)
+        self.overrideEnv('BZR_HOME', test_home_dir)
 
     def setUp(self):
         super(TestCaseWithMemoryTransport, self).setUp()

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2010-12-02 09:23:10 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2010-12-16 17:48:29 +0000
@@ -719,13 +719,13 @@
             f = file('fed.bat', 'w')
             f.write('@rem dummy fed')
             f.close()
-            osutils.set_or_unset_env('BZR_EDITOR', "fed.bat")
+            self.overrideEnv('BZR_EDITOR', "fed.bat")
         else:
             f = file('fed.sh', 'wb')
             f.write('#!/bin/sh\n')
             f.close()
             os.chmod('fed.sh', 0755)
-            osutils.set_or_unset_env('BZR_EDITOR', "./fed.sh")
+            self.overrideEnv('BZR_EDITOR', "./fed.sh")
 
     def setup_commit_with_template(self):
         self.setup_editor()
@@ -756,8 +756,8 @@
         os.chdir('foo')
         open('foo.txt', 'w').write('hello')
         self.run_bzr(['add'])
-        osutils.set_or_unset_env('EMAIL', None)
-        osutils.set_or_unset_env('BZR_EMAIL', None)
+        self.overrideEnv('EMAIL', None)
+        self.overrideEnv('BZR_EMAIL', None)
         out, err = self.run_bzr(['commit', '-m', 'initial'], 3)
         self.assertContainsRe(err, 'Unable to determine your name')
 

=== modified file 'bzrlib/tests/blackbox/test_diff.py'
--- bzrlib/tests/blackbox/test_diff.py	2010-07-22 13:19:48 +0000
+++ bzrlib/tests/blackbox/test_diff.py	2010-12-16 17:48:29 +0000
@@ -386,8 +386,7 @@
         # subprocess.py that we had to workaround).
         # However, if 'diff' may not be available
         self.make_example_branch()
-        # this will be automatically restored by the base bzr test class
-        os.environ['BZR_PROGRESS_BAR'] = 'none'
+        self.overrideEnv('BZR_PROGRESS_BAR', 'none')
         out, err = self.run_bzr_subprocess('diff -r 1 --diff-options -ub',
                                            universal_newlines=True,
                                            retcode=None)

=== modified file 'bzrlib/tests/blackbox/test_init.py'
--- bzrlib/tests/blackbox/test_init.py	2010-10-12 09:46:37 +0000
+++ bzrlib/tests/blackbox/test_init.py	2010-12-16 17:48:29 +0000
@@ -226,9 +226,9 @@
         # and uses whoami only in a lock file. Without whoami the login name
         # is used. This test is to ensure that init passes even when whoami
         # is not available.
-        osutils.set_or_unset_env('EMAIL', None)
-        osutils.set_or_unset_env('BZR_EMAIL', None)
+        self.overrideEnv('EMAIL', None)
+        self.overrideEnv('BZR_EMAIL', None)
         out, err = self.run_bzr(['init', 'foo'])
         self.assertEqual(err, '')
         self.assertTrue(os.path.exists('foo'))
-        
+

=== modified file 'bzrlib/tests/blackbox/test_non_ascii.py'
--- bzrlib/tests/blackbox/test_non_ascii.py	2010-12-09 04:26:53 +0000
+++ bzrlib/tests/blackbox/test_non_ascii.py	2010-12-16 17:48:29 +0000
@@ -42,7 +42,7 @@
 
         self.overrideAttr(osutils, '_cached_user_encoding', self.encoding)
         email = self.info['committer'] + ' <[e-mail]>'
-        os.environ['BZR_EMAIL'] = email.encode(osutils.get_user_encoding())
+        self.overrideEnv('BZR_EMAIL', email.encode(osutils.get_user_encoding()))
         self.create_base()
 
     def run_bzr_decode(self, args, encoding=None, fail=False, retcode=None,

=== modified file 'bzrlib/tests/blackbox/test_serve.py'
--- bzrlib/tests/blackbox/test_serve.py	2010-06-20 11:18:38 +0000
+++ bzrlib/tests/blackbox/test_serve.py	2010-12-16 17:48:29 +0000
@@ -225,7 +225,7 @@
         # -Dhpss, and does drop some hpss logging to the file.
         self.make_branch('.')
         log_fname = os.getcwd() + '/server.log'
-        self._captureVar('BZR_LOG', log_fname)
+        self.overrideEnv('BZR_LOG', log_fname)
         process, transport = self.start_server_inet(['-Dhpss'])
         branch = BzrDir.open_from_transport(transport).open_branch()
         self.make_read_requests(branch)

=== modified file 'bzrlib/tests/blackbox/test_shared_repository.py'
--- bzrlib/tests/blackbox/test_shared_repository.py	2010-05-05 14:02:53 +0000
+++ bzrlib/tests/blackbox/test_shared_repository.py	2010-12-16 17:48:29 +0000
@@ -155,8 +155,8 @@
         # and uses whoami only in a lock file. Without whoami the login name
         # is used. This test is to ensure that init-repo passes even when whoami
         # is not available.
-        osutils.set_or_unset_env('EMAIL', None)
-        osutils.set_or_unset_env('BZR_EMAIL', None)
+        self.overrideEnv('EMAIL', None)
+        self.overrideEnv('BZR_EMAIL', None)
         out, err = self.run_bzr(['init-repo', 'foo'])
         self.assertEqual(err, '')
         self.assertTrue(os.path.exists('foo'))

=== modified file 'bzrlib/tests/blackbox/test_too_much.py'
--- bzrlib/tests/blackbox/test_too_much.py	2010-06-11 07:32:12 +0000
+++ bzrlib/tests/blackbox/test_too_much.py	2010-12-16 17:48:29 +0000
@@ -315,37 +315,28 @@
         cmd_name = 'test-command'
         if sys.platform == 'win32':
             cmd_name += '.bat'
-        oldpath = os.environ.get('BZRPATH', None)
-        try:
-            if 'BZRPATH' in os.environ:
-                del os.environ['BZRPATH']
-
-            f = file(cmd_name, 'wb')
-            if sys.platform == 'win32':
-                f.write('@echo off\n')
-            else:
-                f.write('#!/bin/sh\n')
-            # f.write('echo Hello from test-command')
-            f.close()
-            os.chmod(cmd_name, 0755)
-
-            # It should not find the command in the local
-            # directory by default, since it is not in my path
-            self.run_bzr(cmd_name, retcode=3)
-
-            # Now put it into my path
-            os.environ['BZRPATH'] = '.'
-
-            self.run_bzr(cmd_name)
-
-            # Make sure empty path elements are ignored
-            os.environ['BZRPATH'] = os.pathsep
-
-            self.run_bzr(cmd_name, retcode=3)
-
-        finally:
-            if oldpath:
-                os.environ['BZRPATH'] = oldpath
+        self.overrideEnv('BZRPATH', None)
+
+        f = file(cmd_name, 'wb')
+        if sys.platform == 'win32':
+            f.write('@echo off\n')
+        else:
+            f.write('#!/bin/sh\n')
+        # f.write('echo Hello from test-command')
+        f.close()
+        os.chmod(cmd_name, 0755)
+
+        # It should not find the command in the local
+        # directory by default, since it is not in my path
+        self.run_bzr(cmd_name, retcode=3)
+
+        # Now put it into my path
+        self.overrideEnv('BZRPATH', '.')
+        self.run_bzr(cmd_name)
+
+        # Make sure empty path elements are ignored
+        self.overrideEnv('BZRPATH', os.pathsep)
+        self.run_bzr(cmd_name, retcode=3)
 
 
 def listdir_sorted(dir):

=== modified file 'bzrlib/tests/blackbox/test_version.py'
--- bzrlib/tests/blackbox/test_version.py	2009-12-02 16:43:09 +0000
+++ bzrlib/tests/blackbox/test_version.py	2010-12-16 17:48:29 +0000
@@ -95,7 +95,7 @@
             raise TestSkipped('Cannot find a unicode character that works in'
                               ' encoding %s' % (osutils.get_user_encoding(),))
 
-        osutils.set_or_unset_env('BZR_HOME', str_val)
+        self.overrideEnv('BZR_HOME', str_val)
         self.permit_source_tree_branch_repo()
         out = self.run_bzr("version")[0]
         self.assertTrue(len(out) > 0)
@@ -106,7 +106,7 @@
 
     def test_simple(self):
         bzr_log = 'my.bzr.log'
-        osutils.set_or_unset_env('BZR_LOG', bzr_log)
+        self.overrideEnv('BZR_LOG', bzr_log)
         default_log = os.path.join(os.environ['BZR_HOME'], '.bzr.log')
         self.failIfExists([default_log, bzr_log])
         out = self.run_bzr_subprocess('version')[0]
@@ -122,7 +122,7 @@
             bzr_log = 'NUL'
         else:
             bzr_log = '/dev/null'
-        osutils.set_or_unset_env('BZR_LOG', bzr_log)
+        self.overrideEnv('BZR_LOG', bzr_log)
         default_log = os.path.join(os.environ['BZR_HOME'], '.bzr.log')
         self.failIfExists(default_log)
         out = self.run_bzr_subprocess('version')[0]

=== modified file 'bzrlib/tests/blackbox/test_whoami.py'
--- bzrlib/tests/blackbox/test_whoami.py	2010-06-23 08:19:28 +0000
+++ bzrlib/tests/blackbox/test_whoami.py	2010-12-16 17:48:29 +0000
@@ -52,7 +52,7 @@
 
         # Verify that the environment variable overrides the value
         # in the file
-        os.environ['BZR_EMAIL'] = 'Different ID <[e-mail]>'
+        self.overrideEnv('BZR_EMAIL', 'Different ID <[e-mail]>')
         whoami = self.run_bzr("whoami")[0]
         self.assertEquals('Different ID <[e-mail]>\n', whoami)
         whoami_email = self.run_bzr("whoami --email")[0]
@@ -94,8 +94,8 @@
     def test_whoami_not_set(self):
         """Ensure whoami error if username is not set.
         """
-        osutils.set_or_unset_env('EMAIL', None)
-        osutils.set_or_unset_env('BZR_EMAIL', None)
+        self.overrideEnv('EMAIL', None)
+        self.overrideEnv('BZR_EMAIL', None)
         out, err = self.run_bzr(['whoami'], 3)
         self.assertContainsRe(err, 'Unable to determine your name')
 

=== modified file 'bzrlib/tests/per_controldir/test_controldir.py'
--- bzrlib/tests/per_controldir/test_controldir.py	2010-11-26 17:59:08 +0000
+++ bzrlib/tests/per_controldir/test_controldir.py	2010-12-16 17:48:29 +0000
@@ -257,7 +257,7 @@
 
     def test_clone_bzrdir_branch_and_repo_fixed_user_id(self):
         # Bug #430868 is about an email containing '.sig'
-        os.environ['BZR_EMAIL'] = '[e-mail]'
+        self.overrideEnv('BZR_EMAIL', '[e-mail]')
         tree = self.make_branch_and_tree('commit_tree')
         self.build_tree(['commit_tree/foo'])
         tree.add('foo')

=== modified file 'bzrlib/tests/per_repository/test_check.py'
--- bzrlib/tests/per_repository/test_check.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/per_repository/test_check.py	2010-12-16 17:48:29 +0000
@@ -135,7 +135,7 @@
         repo.lock_write()
         self.addCleanup(repo.unlock)
         config = _mod_config.Config()
-        os.environ['BZR_EMAIL'] = '[e-mail]'
+        self.overrideEnv('BZR_EMAIL', '[e-mail]')
         builder = repo.get_commit_builder(None, [], config)
         list(builder.record_iter_changes(None, _mod_revision.NULL_REVISION, [
             ('TREE_ROOT', (None, ''), True, (False, True), (None, None),

=== modified file 'bzrlib/tests/per_repository/test_commit_builder.py'
--- bzrlib/tests/per_repository/test_commit_builder.py	2010-10-15 16:43:03 +0000
+++ bzrlib/tests/per_repository/test_commit_builder.py	2010-12-16 17:48:29 +0000
@@ -1317,7 +1317,7 @@
     def test_committer_no_username(self):
         # Ensure that when no username is available but a committer is
         # supplied, commit works.
-        del os.environ['EMAIL']
+        self.overrideEnv('EMAIL', None)
         tree = self.make_branch_and_tree(".")
         tree.lock_write()
         try:

=== modified file 'bzrlib/tests/per_transport.py'
--- bzrlib/tests/per_transport.py	2010-09-21 03:20:09 +0000
+++ bzrlib/tests/per_transport.py	2010-12-16 17:48:29 +0000
@@ -98,7 +98,7 @@
 
     def setUp(self):
         super(TransportTests, self).setUp()
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
 
     def check_transport_contents(self, content, transport, relpath):
         """Check that transport.get(relpath).read() == content."""

=== modified file 'bzrlib/tests/per_uifactory/__init__.py'
--- bzrlib/tests/per_uifactory/__init__.py	2010-09-09 07:49:00 +0000
+++ bzrlib/tests/per_uifactory/__init__.py	2010-12-16 17:48:29 +0000
@@ -190,7 +190,7 @@
 
         # Remove 'TERM' == 'dumb' which causes us to *not* treat output as a
         # real terminal, even though isatty returns True
-        self._captureVar('TERM', None)
+        self.overrideEnv('TERM', None)
         self.stderr = TTYStringIO()
         self.stdout = TTYStringIO()
         self.factory = ui.text.TextUIFactory(self.stdin, self.stdout,

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2010-11-29 01:23:53 +0000
+++ bzrlib/tests/test_config.py	2010-12-16 17:48:29 +0000
@@ -402,11 +402,11 @@
 
     def setUp(self):
         super(TestConfigPath, self).setUp()
-        os.environ['HOME'] = '/home/bogus'
-        os.environ['XDG_CACHE_DIR'] = ''
+        self.overrideEnv('HOME', '/home/bogus')
+        self.overrideEnv('XDG_CACHE_DIR', '')
         if sys.platform == 'win32':
-            os.environ['BZR_HOME'] = \
-                r'C:\Documents and Settings\bogus\Application Data'
+            self.overrideEnv(
+                'BZR_HOME', r'C:\Documents and Settings\bogus\Application Data')
             self.bzr_home = \
                 'C:/Documents and Settings/bogus/Application Data/bazaar/2.0'
         else:
@@ -441,9 +441,9 @@
             raise tests.TestNotApplicable(
                 'XDG config dir not used on this platform')
         super(TestXDGConfigDir, self).setUp()
-        os.environ['HOME'] = self.test_home_dir
+        self.overrideEnv('HOME', self.test_home_dir)
         # BZR_HOME overrides everything we want to test so unset it.
-        del os.environ['BZR_HOME']
+        self.overrideEnv('BZR_HOME', None)
 
     def test_xdg_config_dir_exists(self):
         """When ~/.config/bazaar exists, use it as the config dir."""
@@ -454,7 +454,7 @@
     def test_xdg_config_home(self):
         """When XDG_CONFIG_HOME is set, use it."""
         xdgconfigdir = osutils.pathjoin(self.test_home_dir, 'xdgconfig')
-        os.environ['XDG_CONFIG_HOME'] = xdgconfigdir
+        self.overrideEnv('XDG_CONFIG_HOME', xdgconfigdir)
         newdir = osutils.pathjoin(xdgconfigdir, 'bazaar')
         os.makedirs(newdir)
         self.assertEqual(config.config_dir(), newdir)
@@ -1308,7 +1308,7 @@
         self.assertEqual('bzr', my_config.get_bzr_remote_path())
         my_config.set_user_option('bzr_remote_path', '/path-bzr')
         self.assertEqual('/path-bzr', my_config.get_bzr_remote_path())
-        os.environ['BZR_REMOTE_PATH'] = '/environ-bzr'
+        self.overrideEnv('BZR_REMOTE_PATH', '/environ-bzr')
         self.assertEqual('/environ-bzr', my_config.get_bzr_remote_path())
 
 
@@ -1360,7 +1360,7 @@
         self.assertEqual("John", my_config._get_user_id())
 
     def test_BZR_EMAIL_OVERRIDES(self):
-        os.environ['BZR_EMAIL'] = "Robert Collins <[e-mail]>"
+        self.overrideEnv('BZR_EMAIL', "Robert Collins <[e-mail]>")
         branch = FakeBranch()
         my_config = config.BranchConfig(branch)
         self.assertEqual("Robert Collins <[e-mail]>",

=== modified file 'bzrlib/tests/test_crash.py'
--- bzrlib/tests/test_crash.py	2010-04-08 04:34:03 +0000
+++ bzrlib/tests/test_crash.py	2010-12-16 17:48:29 +0000
@@ -40,7 +40,7 @@
     def test_apport_report(self):
         crash_dir = osutils.joinpath((self.test_base_dir, 'crash'))
         os.mkdir(crash_dir)
-        os.environ['APPORT_CRASH_DIR'] = crash_dir
+        self.overrideEnv('APPORT_CRASH_DIR', crash_dir)
         self.assertEquals(crash_dir, config.crash_dir())
 
         stderr = StringIO()

=== modified file 'bzrlib/tests/test_diff.py'
--- bzrlib/tests/test_diff.py	2010-05-26 15:58:08 +0000
+++ bzrlib/tests/test_diff.py	2010-12-16 17:48:29 +0000
@@ -144,31 +144,22 @@
         self.check_patch(lines)
 
     def test_external_diff_binary_lang_c(self):
-        old_env = {}
         for lang in ('LANG', 'LC_ALL', 'LANGUAGE'):
-            old_env[lang] = osutils.set_or_unset_env(lang, 'C')
-        try:
-            lines = external_udiff_lines(['\x00foobar\n'], ['foo\x00bar\n'])
-            # Older versions of diffutils say "Binary files", newer
-            # versions just say "Files".
-            self.assertContainsRe(lines[0],
-                                  '(Binary f|F)iles old and new differ\n')
-            self.assertEquals(lines[1:], ['\n'])
-        finally:
-            for lang, old_val in old_env.iteritems():
-                osutils.set_or_unset_env(lang, old_val)
+            self.overrideEnv(lang, 'C')
+        lines = external_udiff_lines(['\x00foobar\n'], ['foo\x00bar\n'])
+        # Older versions of diffutils say "Binary files", newer
+        # versions just say "Files".
+        self.assertContainsRe(lines[0], '(Binary f|F)iles old and new differ\n')
+        self.assertEquals(lines[1:], ['\n'])
 
     def test_no_external_diff(self):
         """Check that NoDiff is raised when diff is not available"""
-        # Use os.environ['PATH'] to make sure no 'diff' command is available
-        orig_path = os.environ['PATH']
-        try:
-            os.environ['PATH'] = ''
-            self.assertRaises(errors.NoDiff, diff.external_diff,
-                              'old', ['boo\n'], 'new', ['goo\n'],
-                              StringIO(), diff_opts=['-u'])
-        finally:
-            os.environ['PATH'] = orig_path
+        # Make sure no 'diff' command is available
+        # XXX: Weird, using None instead of '' breaks the test -- vila 20101216
+        self.overrideEnv('PATH', '')
+        self.assertRaises(errors.NoDiff, diff.external_diff,
+                          'old', ['boo\n'], 'new', ['goo\n'],
+                          StringIO(), diff_opts=['-u'])
 
     def test_internal_diff_default(self):
         # Default internal diff encoding is utf8

=== modified file 'bzrlib/tests/test_http.py'
--- bzrlib/tests/test_http.py	2010-12-15 16:39:25 +0000
+++ bzrlib/tests/test_http.py	2010-12-16 17:48:29 +0000
@@ -1073,19 +1073,6 @@
     Only the urllib implementation is tested here.
     """
 
-    def setUp(self):
-        tests.TestCase.setUp(self)
-        self._http_saved_env = {}
-        self.addCleanup(self._restore_env)
-
-    def _install_env(self, env):
-        for name, value in env.iteritems():
-            self._http_saved_env[name] = osutils.set_or_unset_env(name, value)
-
-    def _restore_env(self):
-        for name, value in self._http_saved_env.iteritems():
-            osutils.set_or_unset_env(name, value)
-
     def _proxied_request(self):
         handler = _urllib2_wrappers.ProxyHandler()
         request = _urllib2_wrappers.Request('GET','http://baz/buzzle')
@@ -1093,13 +1080,13 @@
         return request
 
     def test_empty_user(self):
-        self._install_env({'http_proxy': 'http://bar.com'})
+        self.overrideEnv('http_proxy', 'http://bar.com')
         request = self._proxied_request()
         self.assertFalse(request.headers.has_key('Proxy-authorization'))
 
     def test_invalid_proxy(self):
         """A proxy env variable without scheme"""
-        self._install_env({'http_proxy': 'host:1234'})
+        self.overrideEnv('http_proxy', 'host:1234')
         self.assertRaises(errors.InvalidURL, self._proxied_request)
 
 
@@ -1136,39 +1123,23 @@
             self.no_proxy_host = self.server_host_port
         # The secondary server is the proxy
         self.proxy_url = self.get_secondary_url()
-        self._http_saved_env = {}
 
     def _testing_pycurl(self):
         # TODO: This is duplicated for lots of the classes in this file
         return (features.pycurl.available()
                 and self._transport == PyCurlTransport)
 
-    def _install_env(self, env):
-        for name, value in env.iteritems():
-            self._http_saved_env[name] = osutils.set_or_unset_env(name, value)
-
-    def _restore_env(self):
-        for name, value in self._http_saved_env.iteritems():
-            osutils.set_or_unset_env(name, value)
-
-    def proxied_in_env(self, env):
-        self._install_env(env)
-        t = self.get_readonly_transport()
-        try:
-            self.assertEqual('proxied contents of foo\n', t.get('foo').read())
-        finally:
-            self._restore_env()
-
-    def not_proxied_in_env(self, env):
-        self._install_env(env)
-        t = self.get_readonly_transport()
-        try:
-            self.assertEqual('contents of foo\n', t.get('foo').read())
-        finally:
-            self._restore_env()
+    def assertProxied(self):
+        t = self.get_readonly_transport()
+        self.assertEqual('proxied contents of foo\n', t.get('foo').read())
+
+    def assertNotProxied(self):
+        t = self.get_readonly_transport()
+        self.assertEqual('contents of foo\n', t.get('foo').read())
 
     def test_http_proxy(self):
-        self.proxied_in_env({'http_proxy': self.proxy_url})
+        self.overrideEnv('http_proxy', self.proxy_url)
+        self.assertProxied()
 
     def test_HTTP_PROXY(self):
         if self._testing_pycurl():
@@ -1177,43 +1148,49 @@
             # about. Should we ?)
             raise tests.TestNotApplicable(
                 'pycurl does not check HTTP_PROXY for security reasons')
-        self.proxied_in_env({'HTTP_PROXY': self.proxy_url})
+        self.overrideEnv('HTTP_PROXY', self.proxy_url)
+        self.assertProxied()
 
     def test_all_proxy(self):
-        self.proxied_in_env({'all_proxy': self.proxy_url})
+        self.overrideEnv('all_proxy', self.proxy_url)
+        self.assertProxied()
 
     def test_ALL_PROXY(self):
-        self.proxied_in_env({'ALL_PROXY': self.proxy_url})
+        self.overrideEnv('ALL_PROXY', self.proxy_url)
+        self.assertProxied()
 
     def test_http_proxy_with_no_proxy(self):
-        self.not_proxied_in_env({'http_proxy': self.proxy_url,
-                                 'no_proxy': self.no_proxy_host})
+        self.overrideEnv('no_proxy', self.no_proxy_host)
+        self.overrideEnv('http_proxy', self.proxy_url)
+        self.assertNotProxied()
 
     def test_HTTP_PROXY_with_NO_PROXY(self):
         if self._testing_pycurl():
             raise tests.TestNotApplicable(
                 'pycurl does not check HTTP_PROXY for security reasons')
-        self.not_proxied_in_env({'HTTP_PROXY': self.proxy_url,
-                                 'NO_PROXY': self.no_proxy_host})
+        self.overrideEnv('NO_PROXY', self.no_proxy_host)
+        self.overrideEnv('HTTP_PROXY', self.proxy_url)
+        self.assertNotProxied()
 
     def test_all_proxy_with_no_proxy(self):
-        self.not_proxied_in_env({'all_proxy': self.proxy_url,
-                                 'no_proxy': self.no_proxy_host})
+        self.overrideEnv('no_proxy', self.no_proxy_host)
+        self.overrideEnv('all_proxy', self.proxy_url)
+        self.assertNotProxied()
 
     def test_ALL_PROXY_with_NO_PROXY(self):
-        self.not_proxied_in_env({'ALL_PROXY': self.proxy_url,
-                                 'NO_PROXY': self.no_proxy_host})
+        self.overrideEnv('NO_PROXY', self.no_proxy_host)
+        self.overrideEnv('ALL_PROXY', self.proxy_url)
+        self.assertNotProxied()
 
     def test_http_proxy_without_scheme(self):
+        self.overrideEnv('http_proxy', self.server_host_port)
         if self._testing_pycurl():
             # pycurl *ignores* invalid proxy env variables. If that ever change
             # in the future, this test will fail indicating that pycurl do not
             # ignore anymore such variables.
-            self.not_proxied_in_env({'http_proxy': self.server_host_port})
+            self.assertNotProxied()
         else:
-            self.assertRaises(errors.InvalidURL,
-                              self.proxied_in_env,
-                              {'http_proxy': self.server_host_port})
+            self.assertRaises(errors.InvalidURL, self.assertProxied)
 
 
 class TestRanges(http_utils.TestCaseWithWebserver):
@@ -1707,8 +1684,6 @@
 
     def setUp(self):
         super(TestProxyAuth, self).setUp()
-        self._http_saved_env = {}
-        self.addCleanup(self._restore_env)
         # Override the contents to avoid false positives
         self.build_tree_contents([('a', 'not proxied contents of a\n'),
                                   ('b', 'not proxied contents of b\n'),
@@ -1717,17 +1692,9 @@
                                   ])
 
     def get_user_transport(self, user, password):
-        self._install_env({'all_proxy': self.get_user_url(user, password)})
+        self.overrideEnv('all_proxy', self.get_user_url(user, password))
         return TestAuth.get_user_transport(self, user, password)
 
-    def _install_env(self, env):
-        for name, value in env.iteritems():
-            self._http_saved_env[name] = osutils.set_or_unset_env(name, value)
-
-    def _restore_env(self):
-        for name, value in self._http_saved_env.iteritems():
-            osutils.set_or_unset_env(name, value)
-
     def test_empty_pass(self):
         if self._testing_pycurl():
             import pycurl
@@ -1770,7 +1737,7 @@
     def setUp(self):
         super(SmartHTTPTunnellingTest, self).setUp()
         # We use the VFS layer as part of HTTP tunnelling tests.
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
         self.transport_readonly_server = http_utils.HTTPServerWithSmarts
         self.http_server = self.get_readonly_server()
 

=== modified file 'bzrlib/tests/test_https_ca_bundle.py'
--- bzrlib/tests/test_https_ca_bundle.py	2010-12-15 16:39:25 +0000
+++ bzrlib/tests/test_https_ca_bundle.py	2010-12-16 17:48:29 +0000
@@ -31,19 +31,9 @@
 class TestGetCAPath(TestCaseInTempDir):
 
     def setUp(self):
-        TestCaseInTempDir.setUp(self)
-        new_env = {
-            'CURL_CA_BUNDLE': None,
-            'PATH': None,
-            }
-        self._http_saved_env = {}
-        self.addCleanup(self._restore)
-        for name, value in new_env.iteritems():
-            self._http_saved_env[name] = osutils.set_or_unset_env(name, None)
-
-    def _restore(self):
-        for name, value in self._http_saved_env.iteritems():
-            osutils.set_or_unset_env(name, value)
+        super(TestGetCAPath, self).setUp()
+        self.overrideEnv('CURL_CA_BUNDLE', None)
+        self.overrideEnv('PATH', None)
 
     def _make_file(self, in_dir='.'):
         fname = os.path.join(in_dir, 'curl-ca-bundle.crt')
@@ -55,7 +45,7 @@
         self.assertEqual('', ca_bundle.get_ca_path(use_cache=False))
 
     def test_env_var(self):
-        osutils.set_or_unset_env('CURL_CA_BUNDLE', 'foo.bar')
+        self.overrideEnv('CURL_CA_BUNDLE', 'foo.bar')
         self._make_file()
         self.assertEqual('foo.bar', ca_bundle.get_ca_path(use_cache=False))
 
@@ -65,6 +55,6 @@
         os.mkdir('foo')
         in_dir = os.path.join(os.getcwd(), 'foo')
         self._make_file(in_dir=in_dir)
-        osutils.set_or_unset_env('PATH', in_dir)
+        self.overrideEnv('PATH', in_dir)
         shouldbe = os.path.join(in_dir, 'curl-ca-bundle.crt')
         self.assertEqual(shouldbe, ca_bundle.get_ca_path(use_cache=False))

=== modified file 'bzrlib/tests/test_import_tariff.py'
--- bzrlib/tests/test_import_tariff.py	2010-10-20 11:16:16 +0000
+++ bzrlib/tests/test_import_tariff.py	2010-12-16 17:48:29 +0000
@@ -17,6 +17,7 @@
 
 """Tests for how many modules are loaded in executing various commands."""
 
+import os
 from testtools import content
 
 from bzrlib.plugin import (
@@ -29,12 +30,19 @@
 
 
 class TestImportTariffs(TestCaseWithTransport):
-
     """Check how many modules are loaded for some representative scenarios.
 
     See the Testing Guide in the developer documentation for more explanation.
     """
 
+    def setUp(self):
+        # Preserve some env vars as we want to escape the isolation for them
+        self.preserved_env_vars = {}
+        for name in ('BZR_HOME', 'BZR_PLUGIN_PATH', 'BZR_DISABLE_PLUGINS',
+                     'BZR_PLUGINS_AT', 'HOME'):
+            self.preserved_env_vars[name] = os.environ.get(name)
+        super(TestImportTariffs, self).setUp()
+
     def run_command_check_imports(self, args, forbidden_imports):
         """Run bzr ARGS in a subprocess and check its imports.
 
@@ -50,15 +58,11 @@
         # more likely to always show everything.  And we use the environment
         # variable rather than 'python -v' in the hope it will work even if
         # bzr is frozen and python is not explicitly specified. -- mbp 20100208
-        #
+
         # Normally we want test isolation from the real $HOME but here we
         # explicitly do want to test against things installed there, therefore
         # we pass it through.
-        env_changes = dict(PYTHONVERBOSE='1')
-        for name in ['BZR_HOME', 'BZR_PLUGIN_PATH',
-                     'BZR_DISABLE_PLUGINS', 'BZR_PLUGINS_AT',
-                     'HOME',]:
-            env_changes[name] = self._old_env.get(name)
+        env_changes = dict(PYTHONVERBOSE='1', **self.preserved_env_vars)
         out, err = self.run_bzr_subprocess(args,
             allow_plugins=(not are_plugins_disabled()),
             env_changes=env_changes)

=== modified file 'bzrlib/tests/test_msgeditor.py'
--- bzrlib/tests/test_msgeditor.py	2010-11-11 13:45:02 +0000
+++ bzrlib/tests/test_msgeditor.py	2010-12-16 17:48:29 +0000
@@ -157,7 +157,7 @@
             return './fed.sh'
 
     def test_run_editor(self):
-        os.environ['BZR_EDITOR'] = self.make_do_nothing_editor()
+        self.overrideEnv('BZR_EDITOR', self.make_do_nothing_editor())
         self.assertEqual(True, msgeditor._run_editor(''),
                          'Unable to run dummy fake editor')
 
@@ -191,11 +191,11 @@
 "%s" fed.py %%1
 """ % sys.executable)
             f.close()
-            os.environ['BZR_EDITOR'] = 'fed.bat'
+            self.overrideEnv('BZR_EDITOR', 'fed.bat')
         else:
             # [non-win32] make python script executable and set BZR_EDITOR
             os.chmod('fed.py', 0755)
-            os.environ['BZR_EDITOR'] = './fed.py'
+            self.overrideEnv('BZR_EDITOR', './fed.py')
 
     def test_edit_commit_message(self):
         working_tree = self.make_uncommitted_tree()
@@ -230,16 +230,17 @@
         working_tree = self.make_uncommitted_tree()
 
         if sys.platform == 'win32':
-            os.environ['BZR_EDITOR'] = 'cmd.exe /c del'
+            editor = 'cmd.exe /c del'
         else:
-            os.environ['BZR_EDITOR'] = 'rm'
+            editor = 'rm'
+        self.overrideEnv('BZR_EDITOR', editor)
 
         self.assertRaises((IOError, OSError), msgeditor.edit_commit_message, '')
 
     def test__get_editor(self):
-        os.environ['BZR_EDITOR'] = 'bzr_editor'
-        os.environ['VISUAL'] = 'visual'
-        os.environ['EDITOR'] = 'editor'
+        self.overrideEnv('BZR_EDITOR', 'bzr_editor')
+        self.overrideEnv('VISUAL', 'visual')
+        self.overrideEnv('EDITOR', 'editor')
 
         conf = config.GlobalConfig.from_string('editor = config_editor\n',
                                                save=True)
@@ -259,7 +260,7 @@
 
     def test__run_editor_EACCES(self):
         """If running a configured editor raises EACESS, the user is warned."""
-        os.environ['BZR_EDITOR'] = 'eacces.py'
+        self.overrideEnv('BZR_EDITOR', 'eacces.py')
         f = file('eacces.py', 'wb')
         f.write('# Not a real editor')
         f.close()
@@ -267,7 +268,7 @@
         os.chmod('eacces.py', 0)
         # Set $EDITOR so that _run_editor will terminate before trying real
         # editors.
-        os.environ['EDITOR'] = self.make_do_nothing_editor()
+        self.overrideEnv('EDITOR', self.make_do_nothing_editor())
         # Call _run_editor, capturing mutter.warning calls.
         warnings = []
         def warning(*args):
@@ -319,23 +320,20 @@
         self.assertFileEqual('', msgfilename)
 
     def test_unsupported_encoding_commit_message(self):
-        old_env = osutils.set_or_unset_env('LANG', 'C')
-        try:
-            # LANG env variable has no effect on Windows
-            # but some characters anyway cannot be represented
-            # in default user encoding
-            char = probe_bad_non_ascii(osutils.get_user_encoding())
-            if char is None:
-                raise TestSkipped('Cannot find suitable non-ascii character '
-                    'for user_encoding (%s)' % osutils.get_user_encoding())
-
-            self.make_fake_editor(message=char)
-
-            working_tree = self.make_uncommitted_tree()
-            self.assertRaises(errors.BadCommitMessageEncoding,
-                              msgeditor.edit_commit_message, '')
-        finally:
-            osutils.set_or_unset_env('LANG', old_env)
+        self.overrideEnv('LANG', 'C')
+        # LANG env variable has no effect on Windows
+        # but some characters anyway cannot be represented
+        # in default user encoding
+        char = probe_bad_non_ascii(osutils.get_user_encoding())
+        if char is None:
+            raise TestSkipped('Cannot find suitable non-ascii character '
+                'for user_encoding (%s)' % osutils.get_user_encoding())
+
+        self.make_fake_editor(message=char)
+
+        working_tree = self.make_uncommitted_tree()
+        self.assertRaises(errors.BadCommitMessageEncoding,
+                          msgeditor.edit_commit_message, '')
 
     def test_generate_commit_message_template_no_hooks(self):
         commit_obj = commit.Commit()

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2010-12-10 00:12:42 +0000
+++ bzrlib/tests/test_osutils.py	2010-12-16 17:48:29 +0000
@@ -1899,17 +1899,17 @@
         self.assertIsInstance(concurrency, int)
 
     def test_local_concurrency_environment_variable(self):
-        os.environ['BZR_CONCURRENCY'] = '2'
+        self.overrideEnv('BZR_CONCURRENCY', '2')
         self.assertEqual(2, osutils.local_concurrency(use_cache=False))
-        os.environ['BZR_CONCURRENCY'] = '3'
+        self.overrideEnv('BZR_CONCURRENCY', '3')
         self.assertEqual(3, osutils.local_concurrency(use_cache=False))
-        os.environ['BZR_CONCURRENCY'] = 'foo'
+        self.overrideEnv('BZR_CONCURRENCY', 'foo')
         self.assertEqual(1, osutils.local_concurrency(use_cache=False))
 
     def test_option_concurrency(self):
-        os.environ['BZR_CONCURRENCY'] = '1'
+        self.overrideEnv('BZR_CONCURRENCY', '1')
         self.run_bzr('rocks --concurrency 42')
-        # Command line overrides envrionment variable
+        # Command line overrides environment variable
         self.assertEquals('42', os.environ['BZR_CONCURRENCY'])
         self.assertEquals(42, osutils.local_concurrency(use_cache=False))
 
@@ -1985,19 +1985,19 @@
     def test_defaults_to_BZR_COLUMNS(self):
         # BZR_COLUMNS is set by the test framework
         self.assertNotEqual('12', os.environ['BZR_COLUMNS'])
-        os.environ['BZR_COLUMNS'] = '12'
+        self.overrideEnv('BZR_COLUMNS', '12')
         self.assertEqual(12, osutils.terminal_width())
 
     def test_falls_back_to_COLUMNS(self):
-        del os.environ['BZR_COLUMNS']
+        self.overrideEnv('BZR_COLUMNS', None)
         self.assertNotEqual('42', os.environ['COLUMNS'])
         self.set_fake_tty()
-        os.environ['COLUMNS'] = '42'
+        self.overrideEnv('COLUMNS', '42')
         self.assertEqual(42, osutils.terminal_width())
 
     def test_tty_default_without_columns(self):
-        del os.environ['BZR_COLUMNS']
-        del os.environ['COLUMNS']
+        self.overrideEnv('BZR_COLUMNS', None)
+        self.overrideEnv('COLUMNS', None)
 
         def terminal_size(w, h):
             return 42, 42
@@ -2010,8 +2010,8 @@
         self.assertEqual(42, osutils.terminal_width())
 
     def test_non_tty_default_without_columns(self):
-        del os.environ['BZR_COLUMNS']
-        del os.environ['COLUMNS']
+        self.overrideEnv('BZR_COLUMNS', None)
+        self.overrideEnv('COLUMNS', None)
         self.replace_stdout(None)
         self.assertEqual(None, osutils.terminal_width())
 
@@ -2027,8 +2027,8 @@
         else:
             self.overrideAttr(termios, 'TIOCGWINSZ')
             del termios.TIOCGWINSZ
-        del os.environ['BZR_COLUMNS']
-        del os.environ['COLUMNS']
+        self.overrideEnv('BZR_COLUMNS', None)
+        self.overrideEnv('COLUMNS', None)
         # Whatever the result is, if we don't raise an exception, it's ok.
         osutils.terminal_width()
 
@@ -2070,7 +2070,7 @@
 class TestGetuserUnicode(tests.TestCase):
 
     def test_ascii_user(self):
-        osutils.set_or_unset_env('LOGNAME', 'jrandom')
+        self.overrideEnv('LOGNAME', 'jrandom')
         self.assertEqual(u'jrandom', osutils.getuser_unicode())
 
     def test_unicode_user(self):
@@ -2082,9 +2082,9 @@
                 % (osutils.get_user_encoding(),))
         uni_username = u'jrandom' + uni_val
         encoded_username = uni_username.encode(ue)
-        osutils.set_or_unset_env('LOGNAME', encoded_username)
+        self.overrideEnv('LOGNAME', encoded_username)
         self.assertEqual(uni_username, osutils.getuser_unicode())
-        osutils.set_or_unset_env('LOGNAME', u'jrandom\xb6'.encode(ue))
+        self.overrideEnv('LOGNAME', u'jrandom\xb6'.encode(ue))
         self.assertEqual(u'jrandom\xb6', osutils.getuser_unicode())
 
 class TestBackupNames(tests.TestCase):

=== modified file 'bzrlib/tests/test_osutils_encodings.py'
--- bzrlib/tests/test_osutils_encodings.py	2010-06-29 16:21:13 +0000
+++ bzrlib/tests/test_osutils_encodings.py	2010-12-16 17:48:29 +0000
@@ -171,8 +171,6 @@
     def setUp(self):
         TestCase.setUp(self)
         self.overrideAttr(locale, 'getpreferredencoding')
-        self.addCleanup(osutils.set_or_unset_env,
-                        'LANG', os.environ.get('LANG'))
         self.overrideAttr(sys, 'stderr', StringIOWrapper())
 
     def test_get_user_encoding(self):
@@ -181,7 +179,8 @@
 
         locale.getpreferredencoding = f
         fake_codec.add('user_encoding')
-        self.assertEquals('user_encoding', osutils.get_user_encoding(use_cache=False))
+        self.assertEquals('user_encoding',
+                          osutils.get_user_encoding(use_cache=False))
         self.assertEquals('', sys.stderr.getvalue())
 
     def test_user_cp0(self):
@@ -216,7 +215,7 @@
             raise locale.Error, 'unsupported locale'
 
         locale.getpreferredencoding = f
-        os.environ['LANG'] = 'BOGUS'
+        self.overrideEnv('LANG', 'BOGUS')
         self.assertEquals('ascii', osutils.get_user_encoding(use_cache=False))
         self.assertEquals('bzr: warning: unsupported locale\n'
                           '  Could not determine what text encoding to use.\n'

=== modified file 'bzrlib/tests/test_plugins.py'
--- bzrlib/tests/test_plugins.py	2010-06-11 08:02:42 +0000
+++ bzrlib/tests/test_plugins.py	2010-12-16 17:48:29 +0000
@@ -656,7 +656,7 @@
                     self.fail('No path to global plugins')
 
     def test_get_standard_plugins_path_env(self):
-        os.environ['BZR_PLUGIN_PATH'] = 'foo/'
+        self.overrideEnv('BZR_PLUGIN_PATH', 'foo/')
         path = plugin.get_standard_plugins_path()
         for directory in path:
             self.assertNotContainsRe(directory, r'\\/$')
@@ -692,7 +692,7 @@
 
     def _set_path(self, *args):
         path = os.pathsep.join(self._list2paths(*args))
-        osutils.set_or_unset_env('BZR_PLUGIN_PATH', path)
+        self.overrideEnv('BZR_PLUGIN_PATH', path)
 
     def check_path(self, expected_dirs, setting_dirs):
         if setting_dirs:
@@ -779,7 +779,7 @@
         self.addCleanup(self._unregister_plugin, 'test_foo')
 
     def test_cannot_import(self):
-        osutils.set_or_unset_env('BZR_DISABLE_PLUGINS', 'test_foo')
+        self.overrideEnv('BZR_DISABLE_PLUGINS', 'test_foo')
         plugin.set_plugins_path(['.'])
         try:
             import bzrlib.plugins.test_foo
@@ -801,7 +801,7 @@
         self.overrideAttr(trace, 'warning', captured_warning)
         # Reset the flag that protect against double loading
         self.overrideAttr(plugin, '_loaded', False)
-        osutils.set_or_unset_env('BZR_DISABLE_PLUGINS', 'test_foo')
+        self.overrideEnv('BZR_DISABLE_PLUGINS', 'test_foo')
         plugin.load_plugins(['.'])
         self.assertPluginUnknown('test_foo')
         # Make sure we don't warn about the plugin ImportError since this has
@@ -859,7 +859,7 @@
         self.assertTestFooLoadedFrom('standard/test_foo')
 
     def test_import(self):
-        osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
+        self.overrideEnv('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
         plugin.set_plugins_path(['standard'])
         try:
             import bzrlib.plugins.test_foo
@@ -868,12 +868,12 @@
         self.assertTestFooLoadedFrom('non-standard-dir')
 
     def test_loading(self):
-        osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
+        self.overrideEnv('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
         plugin.load_plugins(['standard'])
         self.assertTestFooLoadedFrom('non-standard-dir')
 
     def test_compiled_loaded(self):
-        osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
+        self.overrideEnv('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
         plugin.load_plugins(['standard'])
         self.assertTestFooLoadedFrom('non-standard-dir')
         self.assertIsSameRealPath('non-standard-dir/__init__.py',
@@ -896,7 +896,7 @@
         self.create_plugin_package('test_bar', dir='non-standard-dir/test_bar')
         self.addCleanup(self._unregister_plugin_submodule,
                         'test_foo', 'test_bar')
-        osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
+        self.overrideEnv('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
         plugin.set_plugins_path(['standard'])
         import bzrlib.plugins.test_foo
         self.assertEqual('bzrlib.plugins.test_foo',
@@ -913,7 +913,7 @@
         self.create_plugin_package('test_bar', dir='another-dir/test_bar')
         self.addCleanup(self._unregister_plugin_submodule,
                         'test_foo', 'test_bar')
-        osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@another-dir')
+        self.overrideEnv('BZR_PLUGINS_AT', 'test_foo@another-dir')
         plugin.set_plugins_path(['standard'])
         import bzrlib.plugins.test_foo
         self.assertEqual('bzrlib.plugins.test_foo',
@@ -928,7 +928,7 @@
         random = 'non-standard-dir/setup.py'
         os.rename(init, random)
         self.addCleanup(os.rename, random, init)
-        osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
+        self.overrideEnv('BZR_PLUGINS_AT', 'test_foo@non-standard-dir')
         plugin.load_plugins(['standard'])
         self.assertPluginUnknown('test_foo')
 
@@ -942,6 +942,6 @@
 ''' % ('test_foo', plugin_path)
         self.create_plugin('test_foo', source=source,
                            dir=plugin_dir, file_name=plugin_file_name)
-        osutils.set_or_unset_env('BZR_PLUGINS_AT', 'test_foo@%s' % plugin_path)
+        self.overrideEnv('BZR_PLUGINS_AT', 'test_foo@%s' % plugin_path)
         plugin.load_plugins(['standard'])
         self.assertTestFooLoadedFrom(plugin_path)

=== modified file 'bzrlib/tests/test_read_bundle.py'
--- bzrlib/tests/test_read_bundle.py	2010-12-09 04:26:53 +0000
+++ bzrlib/tests/test_read_bundle.py	2010-12-16 17:48:29 +0000
@@ -69,7 +69,7 @@
         # from the one we want to test, so we must inject a correct transport
         # into possible_transports first).
         self.possible_transports = [self.get_transport(self.bundle_name)]
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
         wt = self.create_test_bundle()
 
     def read_mergeable_from_url(self, url):

=== modified file 'bzrlib/tests/test_rules.py'
--- bzrlib/tests/test_rules.py	2010-01-14 15:24:04 +0000
+++ bzrlib/tests/test_rules.py	2010-12-16 17:48:29 +0000
@@ -118,10 +118,10 @@
 
     def setUp(self):
         super(TestRulesPath, self).setUp()
-        os.environ['HOME'] = '/home/bogus'
+        self.overrideEnv('HOME', '/home/bogus')
         if sys.platform == 'win32':
-            os.environ['BZR_HOME'] = \
-                r'C:\Documents and Settings\bogus\Application Data'
+            self.overrideEnv(
+                'BZR_HOME', r'C:\Documents and Settings\bogus\Application Data')
             self.bzr_home = \
                 'C:/Documents and Settings/bogus/Application Data/bazaar/2.0'
         else:

=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2010-11-19 23:03:19 +0000
+++ bzrlib/tests/test_selftest.py	2010-12-16 17:48:29 +0000
@@ -3387,10 +3387,9 @@
         result = tests.ExtendedTestResult(StringIO(), 0, 1)
         post_mortem_calls = []
         self.overrideAttr(pdb, "post_mortem", post_mortem_calls.append)
-        self.addCleanup(osutils.set_or_unset_env, "BZR_TEST_PDB",
-            osutils.set_or_unset_env("BZR_TEST_PDB", None))
+        self.overrideEnv('BZR_TEST_PDB', None)
         result._post_mortem(1)
-        os.environ["BZR_TEST_PDB"] = "on"
+        self.overrideEnv('BZR_TEST_PDB', 'on')
         result._post_mortem(2)
         self.assertEqual([2], post_mortem_calls)
 
@@ -3411,3 +3410,48 @@
                                                 self.verbosity)
         tests.run_suite(suite, runner_class=MyRunner, stream=StringIO())
         self.assertLength(1, calls)
+
+
+class TestEnvironHandling(tests.TestCase):
+
+    def test__captureVar_None_called_twice_leaks(self):
+        self.failIf('MYVAR' in os.environ)
+        self._captureVar('MYVAR', '42')
+        # We need an embedded test to observe the bug
+        class Test(tests.TestCase):
+            def test_me(self):
+                # The first call save the 42 value
+                self._captureVar('MYVAR', None)
+                self.assertEquals(None, os.environ.get('MYVAR'))
+                self.assertEquals('42', self._old_env.get('MYVAR'))
+                # But the second one erases it !
+                self._captureVar('MYVAR', None)
+                self.assertEquals(None, self._old_env.get('MYVAR'))
+        output = StringIO()
+        result = tests.TextTestResult(output, 0, 1)
+        Test('test_me').run(result)
+        if not result.wasStrictlySuccessful():
+            self.fail(output.getvalue())
+        # And we have lost all trace of the original value
+        self.assertEquals(None, os.environ.get('MYVAR'))
+        self.assertEquals(None, self._old_env.get('MYVAR'))
+
+    def test_overrideEnv_None_called_twice_doesnt_leak(self):
+        self.failIf('MYVAR' in os.environ)
+        self.overrideEnv('MYVAR', '42')
+        # We use an embedded test to make sure we fix the _captureVar bug
+        class Test(tests.TestCase):
+            def test_me(self):
+                # The first call save the 42 value
+                self.overrideEnv('MYVAR', None)
+                self.assertEquals(None, os.environ.get('MYVAR'))
+                # Make sure we can call it twice
+                self.overrideEnv('MYVAR', None)
+                self.assertEquals(None, os.environ.get('MYVAR'))
+        output = StringIO()
+        result = tests.TextTestResult(output, 0, 1)
+        Test('test_me').run(result)
+        if not result.wasStrictlySuccessful():
+            self.fail(output.getvalue())
+        # We get our value back
+        self.assertEquals('42', os.environ.get('MYVAR'))

=== modified file 'bzrlib/tests/test_sftp_transport.py'
--- bzrlib/tests/test_sftp_transport.py	2010-06-23 08:17:20 +0000
+++ bzrlib/tests/test_sftp_transport.py	2010-12-16 17:48:29 +0000
@@ -32,7 +32,6 @@
 from bzrlib.osutils import (
     pathjoin,
     lexists,
-    set_or_unset_env,
     )
 from bzrlib.tests import (
     features,
@@ -174,27 +173,21 @@
         """Test that if no 'ssh' is available we get builtin paramiko"""
         from bzrlib.transport import ssh
         # set '.' as the only location in the path, forcing no 'ssh' to exist
-        orig_vendor = ssh._ssh_vendor_manager._cached_ssh_vendor
-        orig_path = set_or_unset_env('PATH', '.')
-        try:
-            # No vendor defined yet, query for one
-            ssh._ssh_vendor_manager.clear_cache()
-            vendor = ssh._get_ssh_vendor()
-            self.assertIsInstance(vendor, ssh.ParamikoVendor)
-        finally:
-            set_or_unset_env('PATH', orig_path)
-            ssh._ssh_vendor_manager._cached_ssh_vendor = orig_vendor
+        self.overrideAttr(ssh, '_ssh_vendor_manager')
+        self.overrideEnv('PATH', '.')
+        ssh._ssh_vendor_manager.clear_cache()
+        vendor = ssh._get_ssh_vendor()
+        self.assertIsInstance(vendor, ssh.ParamikoVendor)
 
     def test_abspath_root_sibling_server(self):
         server = stub_sftp.SFTPSiblingAbsoluteServer()
         server.start_server()
-        try:
-            transport = _mod_transport.get_transport(server.get_url())
-            self.assertFalse(transport.abspath('/').endswith('/~/'))
-            self.assertTrue(transport.abspath('/').endswith('/'))
-            del transport
-        finally:
-            server.stop_server()
+        self.addCleanup(server.stop_server)
+
+        transport = _mod_transport.get_transport(server.get_url())
+        self.assertFalse(transport.abspath('/').endswith('/~/'))
+        self.assertTrue(transport.abspath('/').endswith('/'))
+        del transport
 
 
 class SFTPBranchTest(TestCaseWithSFTPServer):

=== modified file 'bzrlib/tests/test_smart_transport.py'
--- bzrlib/tests/test_smart_transport.py	2010-07-01 15:25:41 +0000
+++ bzrlib/tests/test_smart_transport.py	2010-12-16 17:48:29 +0000
@@ -615,7 +615,7 @@
 
     def setUp(self):
         super(TestSmartServerStreamMedium, self).setUp()
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
 
     def portable_socket_pair(self):
         """Return a pair of TCP sockets connected to each other.
@@ -970,7 +970,7 @@
 
     def test_get_error_unexpected(self):
         """Error reported by server with no specific representation"""
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
         class FlakyTransport(object):
             base = 'a_url'
             def external_url(self):
@@ -1077,14 +1077,14 @@
 
     def test_smart_transport_has(self):
         """Checking for file existence over smart."""
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
         self.backing_transport.put_bytes("foo", "contents of foo\n")
         self.assertTrue(self.transport.has("foo"))
         self.assertFalse(self.transport.has("non-foo"))
 
     def test_smart_transport_get(self):
         """Read back a file over smart."""
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
         self.backing_transport.put_bytes("foo", "contents\nof\nfoo\n")
         fp = self.transport.get("foo")
         self.assertEqual('contents\nof\nfoo\n', fp.read())
@@ -1094,7 +1094,7 @@
         # The path in a raised NoSuchFile exception should be the precise path
         # asked for by the client. This gives meaningful and unsurprising errors
         # for users.
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
         err = self.assertRaises(
             errors.NoSuchFile, self.transport.get, 'not%20a%20file')
         self.assertSubset([err.path], ['not%20a%20file', './not%20a%20file'])
@@ -1119,7 +1119,7 @@
 
     def test_open_dir(self):
         """Test changing directory"""
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
         transport = self.transport
         self.backing_transport.mkdir('toffee')
         self.backing_transport.mkdir('toffee/apple')
@@ -1147,7 +1147,7 @@
 
     def test_mkdir_error_readonly(self):
         """TransportNotPossible should be preserved from the backing transport."""
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
         self.start_server(readonly=True)
         self.assertRaises(errors.TransportNotPossible, self.transport.mkdir,
             'foo')
@@ -1261,7 +1261,7 @@
 
     def setUp(self):
         super(SmartServerRequestHandlerTests, self).setUp()
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
 
     def build_handler(self, transport):
         """Returns a handler for the commands in protocol version one."""
@@ -1286,10 +1286,7 @@
         handler = vfs.HasRequest(None, '/')
         # set environment variable after construction to make sure it's
         # examined.
-        # Note that we can safely clobber BZR_NO_SMART_VFS here, because setUp
-        # has called _captureVar, so it will be restored to the right state
-        # afterwards.
-        os.environ['BZR_NO_SMART_VFS'] = ''
+        self.overrideEnv('BZR_NO_SMART_VFS', '')
         self.assertRaises(errors.DisabledMethod, handler.execute)
 
     def test_readonly_exception_becomes_transport_not_possible(self):
@@ -1602,7 +1599,7 @@
         self.assertTrue(self.end_received)
 
     def test_accept_request_and_body_all_at_once(self):
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
         mem_transport = memory.MemoryTransport()
         mem_transport.put_bytes('foo', 'abcdefghij')
         out_stream = StringIO()
@@ -1868,7 +1865,7 @@
         self.assertTrue(self.end_received)
 
     def test_accept_request_and_body_all_at_once(self):
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
         mem_transport = memory.MemoryTransport()
         mem_transport.put_bytes('foo', 'abcdefghij')
         out_stream = StringIO()
@@ -3517,7 +3514,7 @@
     def setUp(self):
         super(HTTPTunnellingSmokeTest, self).setUp()
         # We use the VFS layer as part of HTTP tunnelling tests.
-        self._captureVar('BZR_NO_SMART_VFS', None)
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
 
     def test_smart_http_medium_request_accept_bytes(self):
         medium = FakeHTTPMedium()

=== modified file 'bzrlib/tests/test_trace.py'
--- bzrlib/tests/test_trace.py	2010-10-11 09:53:16 +0000
+++ bzrlib/tests/test_trace.py	2010-12-16 17:48:29 +0000
@@ -301,9 +301,7 @@
         # set up.
         self.overrideAttr(sys, 'stderr', StringIO())
         # Set the log file to something that cannot exist
-        # FIXME: A bit dangerous: we are not in an isolated dir here -- vilajam
-        # 20100125
-        os.environ['BZR_LOG'] = os.getcwd() + '/no-dir/bzr.log'
+        self.overrideEnv('BZR_LOG', os.getcwd() + '/no-dir/bzr.log')
         self.overrideAttr(trace, '_bzr_log_filename')
         logf = trace._open_bzr_log()
         self.assertIs(None, logf)

=== modified file 'bzrlib/tests/test_transport.py'
--- bzrlib/tests/test_transport.py	2010-10-08 07:17:16 +0000
+++ bzrlib/tests/test_transport.py	2010-12-16 17:48:29 +0000
@@ -967,7 +967,7 @@
             bzr_remote_path = sys.executable + ' ' + self.get_bzr_path()
         else:
             bzr_remote_path = self.get_bzr_path()
-        os.environ['BZR_REMOTE_PATH'] = bzr_remote_path
+        self.overrideEnv('BZR_REMOTE_PATH', bzr_remote_path)
 
         # Access the branch via a bzr+ssh URL.  The BZR_REMOTE_PATH environment
         # variable is used to tell bzr what command to run on the remote end.

=== modified file 'bzrlib/tests/test_ui.py'
--- bzrlib/tests/test_ui.py	2010-12-02 10:41:05 +0000
+++ bzrlib/tests/test_ui.py	2010-12-16 17:48:29 +0000
@@ -158,7 +158,7 @@
     def test_text_factory_prompts_and_clears(self):
         # a get_boolean call should clear the pb before prompting
         out = test_progress._TTYStringIO()
-        os.environ['TERM'] = 'xterm'
+        self.overrideEnv('TERM', 'xterm')
         factory = _mod_ui_text.TextUIFactory(
             stdin=tests.StringIOWrapper("yada\ny\n"),
             stdout=out, stderr=out)
@@ -226,7 +226,7 @@
             pb.finished()
 
     def test_quietness(self):
-        os.environ['BZR_PROGRESS_BAR'] = 'text'
+        self.overrideEnv('BZR_PROGRESS_BAR', 'text')
         ui_factory = _mod_ui_text.TextUIFactory(None,
             test_progress._TTYStringIO(),
             test_progress._TTYStringIO())
@@ -313,12 +313,8 @@
             # however, it can still be forced on
             (FileStringIO, 'dumb', 'text', _mod_ui_text.TextProgressView),
             ):
-            os.environ['TERM'] = term
-            if pb is None:
-                if 'BZR_PROGRESS_BAR' in os.environ:
-                    del os.environ['BZR_PROGRESS_BAR']
-            else:
-                os.environ['BZR_PROGRESS_BAR'] = pb
+            self.overrideEnv('TERM', term)
+            self.overrideEnv('BZR_PROGRESS_BAR', pb)
             stdin = file_class('')
             stderr = file_class()
             stdout = file_class()
@@ -335,10 +331,7 @@
         stderr = test_progress._NonTTYStringIO()
         stdout = test_progress._NonTTYStringIO()
         for term_type in ['dumb', None, 'xterm']:
-            if term_type is None:
-                del os.environ['TERM']
-            else:
-                os.environ['TERM'] = term_type
+            self.overrideEnv('TERM', term_type)
             uif = _mod_ui.make_ui_for_terminal(stdin, stdout, stderr)
             self.assertIsInstance(uif, _mod_ui_text.TextUIFactory,
                 'TERM=%r' % (term_type,))

=== modified file 'bzrlib/tests/test_win32utils.py'
--- bzrlib/tests/test_win32utils.py	2010-12-07 17:28:31 +0000
+++ bzrlib/tests/test_win32utils.py	2010-12-16 17:48:29 +0000
@@ -217,7 +217,7 @@
     def test_appdata_not_using_environment(self):
         # Test that we aren't falling back to the environment
         first = win32utils.get_appdata_location()
-        self._captureVar("APPDATA", None)
+        self.overrideEnv("APPDATA", None)
         self.assertPathsEqual(first, win32utils.get_appdata_location())
 
     def test_appdata_matches_environment(self):
@@ -234,7 +234,7 @@
     def test_local_appdata_not_using_environment(self):
         # Test that we aren't falling back to the environment
         first = win32utils.get_local_appdata_location()
-        self._captureVar("LOCALAPPDATA", None)
+        self.overrideEnv("LOCALAPPDATA", None)
         self.assertPathsEqual(first, win32utils.get_local_appdata_location())
 
     def test_local_appdata_matches_environment(self):

=== modified file 'doc/developers/testing.txt'
--- doc/developers/testing.txt	2010-12-09 00:18:26 +0000
+++ doc/developers/testing.txt	2010-12-16 17:48:29 +0000
@@ -977,6 +977,25 @@
 
     self.overrideAttr(osutils, '_cached_user_encoding', 'latin-1')
 
+Temporarily changing environment variables
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If yout test needs to temporarily change some environment variable value
+(which generally means you want it restored at the end), you can use::
+
+    self.overrideEnv('BZR_ENV_VAR', 'new_value')
+
+If you want to remove a variable from the environment, you should use the
+special ``None`` value::
+
+    self.overrideEnv('PATH', None)
+
+If you add a new feature which depends on a new environment variable, make
+sure it behaves properly when this variable is not defined (if applicable) and
+if you need to enforce a specific default value, check the
+``TestCase._cleanEnvironment`` in ``bzrlib.tests.__init__.py`` which defines a
+proper set of values for all tests.
+
 Cleaning up
 ~~~~~~~~~~~
 

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-12-16 15:15:17 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-12-16 17:48:29 +0000
@@ -189,6 +189,11 @@
   to 0.9.5 which will allow tests that need the fixed unicode handling to be
   written. (Martin [gz])
 
+* Introduce an ``overrideEnv()`` helper for tests that needs to change the
+  environment variables while respecting the isolation rules. Get rid of
+  TestCase._restoreEnvironment which is now useless.
+  (Vincent Ladeuil, #690563)
+
 * Printing selftest results to a non-UTF-8 console will now escape characters
   that can't be encoded rather than aborting the test run with an exception.
   (Martin [gz], #633216)