Merge lp:~vila/bzr/690563-test-isolation into lp:bzr

[Vincent Ladeuil]

Fix test isolation leaks destroying env variables.

   BZR_PLUGIN_PATH=-site bzr selftest -s bt.test_import_tariff

suceeding while

   BZR_PLUGIN_PATH=-site bzr selftest --parallel=fork

failing in bzrlib.tests.test_import_tariff.TestImportTariffs.test_simple_local smells like a test isolation issue right ?

The journey was interesting...

After a painful bisection with an instrumented version revealing
that BZR_PLUGIN_PATH was deleted from os.environ somehow, I ended
up with the following suspects:

bzrlib.tests.test_http.TestProxyHttpServer.test_all_proxy(urllib,HTTP/1.0)
bzrlib.tests.test_http.TestProxyHttpServer.test_all_proxy(urllib,HTTP/1.1)
bzrlib.tests.test_http.TestProxyHttpServer.test_all_proxy(pycurl,HTTP/1.0)
bzrlib.tests.test_http.TestProxyHttpServer.test_all_proxy(pycurl,HTTP/1.1)
bzrlib.tests.test_http.TestProxyHttpServer.test_all_proxy_with_no_proxy(urllib,HTTP/1.0)
bzrlib.tests.test_http.TestProxyHttpServer.test_all_proxy_with_no_proxy(urllib,HTTP/1.1)
bzrlib.tests.test_http.TestProxyHttpServer.test_all_proxy_with_no_proxy(pycurl,HTTP/1.0)
bzrlib.tests.test_http.TestProxyHttpServer.test_all_proxy_with_no_proxy(pycurl,HTTP/1.1)
bzrlib.tests.test_import_tariff.TestImportTariffs.test_help_commands
bzrlib.tests.test_import_tariff.TestImportTariffs.test_import_tariffs_working
bzrlib.tests.test_import_tariff.TestImportTariffs.test_simple_local

Deleting test_all_proxy group or test_all_proxy_with_no_proxy
group makes the bug vanish...

The nasty bug is in:

class TestProxyHttpServer(http_utils.TestCaseWithTwoWebservers):

...

    def setUp(self):
        super(TestProxyHttpServer, self).setUp()
        self.transport_secondary_server = http_utils.ProxyServer
        self.build_tree_contents([('foo', 'contents of foo\n'),
                                  ('foo-proxied', 'proxied contents of foo\n')])
        # Let's setup some attributes for tests
        server = self.get_readonly_server()
        self.server_host_port = '%s:%d' % (server.host, server.port)
        if self._testing_pycurl():
            # Oh my ! pycurl does not check for the port as part of
            # no_proxy :-( So we just test the host part
            self.no_proxy_host = server.host
        else:
            self.no_proxy_host = self.server_host_port
        # The secondary server is the proxy
        self.proxy_url = self.get_secondary_url()
        self._old_env = {}

The offending line is the last one, because we have:

class TestCase(testtools.TestCase):

...

    def _cleanEnvironment(self):
        new_env = {
...
        self._old_env = {}

Eerk !

So as soon as one of the proxy tests is executed all values saved by TestCase are lost. For bug #690563, BZR_PLUGIN_PATH was the whistle blower. I'm quite amazed that we didn't get more failures due to this bug so far...

So the obvious fix is to use a different variable to hold the values for the proxy tests.

While this patch is enough for the case at end, I'll propose a more robust one (but more invasive and may be even controversial) later.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/15/2010 8:45 AM, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/690563-test-isolation into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #690563 bzrlib.tests.test_import_tariff.TestImportTariffs.test_simple_local failure
> https://bugs.launchpad.net/bugs/690563
>
>
> BZR_PLUGIN_PATH=-site bzr selftest -s bt.test_import_tariff
>
> suceeding while
>
> BZR_PLUGIN_PATH=-site bzr selftest --parallel=fork
>
> failing in bzrlib.tests.test_import_tariff.TestImportTariffs.test_simple_local smells like a test isolation issue right ?
>
> The journey was interesting...
>

...

I have to wonder why test_http needs its own save-and-restore env
variables. Rather than just doing it via the regular interface. Given
that they are calling "_install_env" and "_restore_env", versus the
TestCase versions.

Anyway, I''m ok with a fix like this, but maybe calling the variable:

self._http_saved_env = {}

Would make it clearer. This is also one of the very few cases where
using a double underscore for self.__old_env would have actually been
useful. (Since they actually end up as different real names, though that
can also confuse you "why isn't it getting set")

 review: needs_fixing
 merge: approve

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk0I688ACgkQJdeBCYSNAAPE3QCfZXe3Eg1ZikWKJIUvX+UW5cmf
LS0An1pLJpSNDh8RPhIkB6pDkqgwfD5d
=efjP
-----END PGP SIGNATURE-----

review: Needs Fixing

[Vincent Ladeuil (vila)]

>>>>> John A Meinel <email address hidden> writes:

<snip/>

    > I have to wonder why test_http needs its own save-and-restore env
    > variables.

Indeed. Hysterical raisins I suspect. I don't remember exactly why I did
it this way, probably TestCase wasn't exactly in the same shape as it is
today or I couldn't figure it.

    > Rather than just doing it via the regular interface.

The regular one has also some traps which I why I want to make another patch.

    > Given that they are calling "_install_env" and "_restore_env",
    > versus the TestCase versions.

Yeah, this clearly pre-dates the overrideAttr era, though os.environ
can't use it easily (it's not a dict and it needs special care).

    > Anyway, I''m ok with a fix like this, but maybe calling the variable:

    > self._http_saved_env = {}

    > Would make it clearer.

Ok. Hopefully I will get rid of them soon anyway.

    > This is also one of the very few cases where using a double
    > underscore for self.__old_env would have actually been
    > useful. (Since they actually end up as different real names,
    > though that can also confuse you "why isn't it getting set")

Yup, I almost went this way but that would defeat the trick used by
test_import_tariff (which want to restore the initial values saved
here).

More on that in my next submission on this topic.

--F0E75180F92.1292431208/axe--

[Vincent Ladeuil (vila)]

sent to pqm by email

[Martin Pool (mbp)]

sent to pqm by email

[Martin Pool (mbp)]

failed: http://sourcefrog.net/tmp/1y/690563-failure.subunit.gz

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-12-02 10:41:05 +0000
+++ bzrlib/tests/__init__.py	2010-12-16 14:41:46 +0000
@@ -3842,7 +3842,13 @@
         return []
     return [
         'bzrlib',
-        'bzrlib.branchbuilder',
+        # FIXME: Fixing bug #690563 revealed an isolation problem in the single
+        # doctest for branchbuilder. Uncomment this when bug #321320 is fixed
+        # to ensure the issue is addressed (note that to reproduce the bug in
+        # the doctest below, one should comment the 'email' config var in
+        # bazaar.conf (or anywhere else). This means an setup where *no* user
+        # is being set at all in the environment.
+#       'bzrlib.branchbuilder',
         'bzrlib.decorators',
         'bzrlib.export',
         'bzrlib.inventory',

=== modified file 'bzrlib/tests/test_http.py'
--- bzrlib/tests/test_http.py	2010-12-14 16:27:12 +0000
+++ bzrlib/tests/test_http.py	2010-12-16 14:41:46 +0000
@@ -1075,15 +1075,15 @@
 
     def setUp(self):
         tests.TestCase.setUp(self)
-        self._old_env = {}
+        self._http_saved_env = {}
         self.addCleanup(self._restore_env)
 
     def _install_env(self, env):
         for name, value in env.iteritems():
-            self._old_env[name] = osutils.set_or_unset_env(name, value)
+            self._http_saved_env[name] = osutils.set_or_unset_env(name, value)
 
     def _restore_env(self):
-        for name, value in self._old_env.iteritems():
+        for name, value in self._http_saved_env.iteritems():
             osutils.set_or_unset_env(name, value)
 
     def _proxied_request(self):
@@ -1136,7 +1136,7 @@
             self.no_proxy_host = self.server_host_port
         # The secondary server is the proxy
         self.proxy_url = self.get_secondary_url()
-        self._old_env = {}
+        self._http_saved_env = {}
 
     def _testing_pycurl(self):
         # TODO: This is duplicated for lots of the classes in this file
@@ -1145,10 +1145,10 @@
 
     def _install_env(self, env):
         for name, value in env.iteritems():
-            self._old_env[name] = osutils.set_or_unset_env(name, value)
+            self._http_saved_env[name] = osutils.set_or_unset_env(name, value)
 
     def _restore_env(self):
-        for name, value in self._old_env.iteritems():
+        for name, value in self._http_saved_env.iteritems():
             osutils.set_or_unset_env(name, value)
 
     def proxied_in_env(self, env):
@@ -1707,7 +1707,7 @@
 
     def setUp(self):
         super(TestProxyAuth, self).setUp()
-        self._old_env = {}
+        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'),
@@ -1722,10 +1722,10 @@
 
     def _install_env(self, env):
         for name, value in env.iteritems():
-            self._old_env[name] = osutils.set_or_unset_env(name, value)
+            self._http_saved_env[name] = osutils.set_or_unset_env(name, value)
 
     def _restore_env(self):
-        for name, value in self._old_env.iteritems():
+        for name, value in self._http_saved_env.iteritems():
             osutils.set_or_unset_env(name, value)
 
     def test_empty_pass(self):

=== modified file 'bzrlib/tests/test_https_ca_bundle.py'
--- bzrlib/tests/test_https_ca_bundle.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_https_ca_bundle.py	2010-12-16 14:41:46 +0000
@@ -36,13 +36,13 @@
             'CURL_CA_BUNDLE': None,
             'PATH': None,
             }
-        self._old_env = {}
+        self._http_saved_env = {}
         self.addCleanup(self._restore)
         for name, value in new_env.iteritems():
-            self._old_env[name] = osutils.set_or_unset_env(name, None)
+            self._http_saved_env[name] = osutils.set_or_unset_env(name, None)
 
     def _restore(self):
-        for name, value in self._old_env.iteritems():
+        for name, value in self._http_saved_env.iteritems():
             osutils.set_or_unset_env(name, value)
 
     def _make_file(self, in_dir='.'):

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-12-16 06:24:14 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-12-16 14:41:46 +0000
@@ -40,6 +40,10 @@
   designated by the action. This will *ignore* all differences that would
   have been merge cleanly otherwise. (Vincent Ladeuil, #638451)
 
+* ``bt.test_http`` was breaking ``os.environ`` by erasing the values saved by
+  ``TestCase`` leading to ``bt.test_import_tariff`` failures.
+  (Vincent Ladeuil, #690563)
+
 Bug Fixes
 *********