Merge lp:~vila/bzr/321320-isolate-doc-tests into lp:bzr

[Vincent Ladeuil]

Isolate doctests from os.environ

This isolates doctests from os.environ, fixing bug #321320.

We have a set of variables that we want to be isolated from the external os.environ.

I moved them out of 'Test_case._cleanEnvironment' into a dedicated 'isolated_environ' to be able to reuse them.

I then created a override_os_environ/restore_environ pair of *functions* so I can use them as parameter to the DocTestSuite *function* (yes, it's not a class). These functions are then added to the doctest (not to the suite) as a setUp/tearDown pair.

I've thought about being even more radical here by modifying the selftest *command* to save/restore os.environ so that tests can just start with an empty os.environ and install whatever they need, but there are several problems with this approach:
- other test runners won't support it,
- this may trigger weird issues with lazy imports (what if the import is sometimes done before the os.environ is clean and sometimes not, etc),
- this would make it harder to write tests for this code (note that the tests added here rely on being properly isolated. I've commented heavily on that as it's easy to overlook).

[Martin Packman (gz)]

Some quick notes without getting into a proper review:

Main concern is that I think the doctests need more of bzrlib.tests.TestCase.setUp to be properly isolated, not just the _cleanEnvironment call, or perhaps even TestCaseWithMemoryTransport.setUp equivalent. This would mean pulling a lot more code off the TestCase class so it could be shared. I'm particularly thinking of TestCase._clear_hooks and similar things. Not having cleanUp usable from doctest setUp without clever hacks is particularly annoying.

+ 'http_proxy': None,
+ 'HTTP_PROXY': None,

Scope creepitis: I'd much prefer this was handled by the osutils.set_or_unset_env or similar, platforms without case sensitive environments shouldn't have to do extra work.

[Vincent Ladeuil (vila)]

> Some quick notes without getting into a proper review:
>
> Main concern is that I think the doctests need more of
> bzrlib.tests.TestCase.setUp to be properly isolated, not just the
> _cleanEnvironment call, or perhaps even TestCaseWithMemoryTransport.setUp
> equivalent. This would mean pulling a lot more code off the TestCase class so
> it could be shared. I'm particularly thinking of TestCase._clear_hooks and
> similar things. Not having cleanUp usable from doctest setUp without clever
> hacks is particularly annoying.

You wouldn't be able to call it from a doctest isn't it ?

But even then, DocTestSuite is a *function* which uses DocTestCase (the class) internally, I'm not against the principle of rewriting python's doctest, but I don't think it's in bzr scope :)

>
>
> + 'http_proxy': None,
> + 'HTTP_PROXY': None,
>
> Scope creepitis: I'd much prefer this was handled by the
> osutils.set_or_unset_env or similar, platforms without case sensitive
> environments shouldn't have to do extra work.

But it's handled this way because on OSes with case sensitive envs, both can be (and are) used !

[Andrew Bennetts (spiv)]

17 + 'HOME': os.getcwd(),

There's a small behaviour difference here, although I think it's probably ok: this calculates the isolated $HOME once, at module import time, rather that during each test's setUp. Given that tests that care about this probably use TestCaseWithMemoryTransport, which explicitly overrides HOME appropriately, it's probably ok... I wonder a little about the effect on doctests that might be influenced by $HOME that can't use TestCaseWithMemoryTransport. In fact, isn't that what bug 312320 is about? Does this really allow bzrlib.branchbuilder's doctests to run in isolation from whatever the user has in their ~/.bazaar? I can't see how it does.

194 + doc_suite = DocTestSuite(
195 + mod, optionflags=doctest.REPORT_ONLY_FIRST_FAILURE)

That's a bit odd: shouldn't both DocTestSuite and REPORT_ONLY_FIRST_FAILURE be prefixed with “doctest.”, or neither of them?

240 + Since we use tests that are already isolated from os.environ abit of care

Typo: “a bit” is two words.

review: Needs Information

[Andrew Bennetts (spiv)]

My suspicion was right: this branch does not appear to fix 321320, except sometimes by accident.

If you e.g. run 'bzr selftest' from your home dir, HOME is set to your real home, and config values in your normal ~/.bazaar/bazaar.conf like create_signatures take effect. In my case that cases gnome-gpg to be run for every commit the test suite does. At best that will slow down the tests considerably, at worst cause test hangs or test failures. This case definitely ought to work, and if bug 321320 is properly fixed, it would.

review: Needs Fixing

[Vincent Ladeuil (vila)]

Nice catch !

Thinking more about it, I think the issue is that giving a value to a variable via isolated_environ is just wrong.

If set HOME to None there (but still set it to os.getcwd() for TestCase), I get no failures. I didn't dare before but this makes sense. A test that *requires* HOME to be set for whatever reason, should make sure it's set or fail.

Re-running the test suite without setting HOME *at all* ....<drum roll> pass !

The other variables set here are:

- EMAIL: root cause for the branchbuilder doctest failure, this one is under discussion about whether the *feature* of making whoami mandatory is a good one. Setting it here masked the issue and *users* faced the consequences, I'll leave it for now, but I'm convinced we should set it to None and let the related tests themselves care about it (including the currently hidden failures),
- TERM, LINES, COLUMNS, BZR_COLUMNS: again, the tests needing these settings should set these variables explicitly, we'll get a better tracking of which tests really care about them.

That being said, I'll investigate further and submit a more robust solution, two reviewers agreeing on the same issue is worth addressing isn't it ? :D

[Vincent Ladeuil (vila)]

So, it turns out that deleting HOME from the environment doesn't
annoy our test suite even slightly, so I did just that. I then
run this branch on babune to see how our various supported OSes
react against homeless tests and the result is: nothing. Not a
single test failed, much to my surprise I should say.

But once you accept that indeed our code and our tests don't need
it, it feels cleaner (and should address spiv's concerns about
GPG, if it doesn't, yell !).

Note that we still have tests that write to $HOME/.bzr.log
(better tracked by running with 'BZR_HOME=. ./bzr selftest'), but
they aren't doctests and as such are out of scope for this fix.

If someone (mgz ?) really feels strongly about adding more
isolation to doctests then feel free to do so, I've prepare
things a bit by adding two functions isolated_doctest_setUp and
isolated_doctest_tearDown that accept a 'test' parameter so it
should be possible to force them to run in a tmpdir.

But since there wasn't any leak I could find that would benefit
from such an isolation, I stopped there.

Now, to come back to spiv's first comment:
> My suspicion was right: this branch does not appear to fix 321320, except sometimes by accident.

> If you e.g. run 'bzr selftest' from your home dir, HOME is set to your real home, and config values in your normal ~/.bazaar/bazaar.conf like create_signatures take effect. In my case that cases gnome-gpg to be run for every commit the test suite does. At best that will slow down the tests considerably, at worst cause test hangs or test failures. This case definitely ought to work, and if bug 321320 is properly fixed, it would.

Would you mind checking this behaviour with bzr trunk ?

And is it fixed now ?

If it isn't, can you point me to the *doctest* you think is exhibiting this behaviour ?

[Robert Collins (lifeless)]

I suggest using a fixture (lp:python-fixtures) or similar rather than
a pair of functions. Its a bit cleaner and will be less code
(particularly if you reuse Fixture rather than rolling your own).

[Martin Packman (gz)]

> If someone (mgz ?) really feels strongly about adding more
> isolation to doctests then feel free to do so, I've prepare
> things a bit by adding two functions isolated_doctest_setUp and
> isolated_doctest_tearDown that accept a 'test' parameter so it
> should be possible to force them to run in a tmpdir.

This may as well land if it fixes the problems with doctests people are actually having, even if it's not a complete solution.

Anyone with for instance, a plugin that adds a hook and doesn't run selftest with --no-plugins will still risk random behaviour though right? I'd like a note in the developer documentation saying doctests are not as robustly isolated as general tests.

Do you also want to uncomment the doctest we disabled the other day in this branch?

[Vincent Ladeuil (vila)]

> This may as well land if it fixes the problems with doctests people are
> actually having, even if it's not a complete solution.

Ok.

>
> Anyone with for instance, a plugin that adds a hook and doesn't run selftest
> with --no-plugins will still risk random behaviour though right?

Right.

> I'd like a
> note in the developer documentation saying doctests are not as robustly
> isolated as general tests.

Ok.

> Do you also want to uncomment the doctest we disabled the other day in this
> branch?

I did and used it to ensure my fix addressed the related problem.

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[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-26 16:23:16 +0000
+++ bzrlib/tests/__init__.py	2011-01-07 11:18:35 +0000
@@ -123,6 +123,93 @@
 TestSuite = TestUtil.TestSuite
 TestLoader = TestUtil.TestLoader
 
+# Tests should run in a clean and clearly defined environment. The goal is to
+# keep them isolated from the running environment as mush as possible. The test
+# framework ensures the variables defined below are set (or deleted if the
+# value is None) before a test is run and reset to their original value after
+# the test is run. Generally if some code depends on an environment variable,
+# the tests should start without this variable in the environment. There are a
+# few exceptions but you shouldn't violate this rule lightly.
+isolated_environ = {
+    'BZR_HOME': None,
+    'HOME': None,
+    # bzr now uses the Win32 API and doesn't rely on APPDATA, but the
+    # tests do check our impls match APPDATA
+    'BZR_EDITOR': None, # test_msgeditor manipulates this variable
+    'VISUAL': None,
+    'EDITOR': None,
+    'BZR_EMAIL': None,
+    'BZREMAIL': None, # may still be present in the environment
+    'EMAIL': '[e-mail]', # set EMAIL as bzr does not guess
+    'BZR_PROGRESS_BAR': None,
+    'BZR_LOG': None,
+    'BZR_PLUGIN_PATH': None,
+    'BZR_DISABLE_PLUGINS': None,
+    'BZR_PLUGINS_AT': None,
+    'BZR_CONCURRENCY': None,
+    # Make sure that any text ui tests are consistent regardless of
+    # the environment the test case is run in; you may want tests that
+    # test other combinations.  'dumb' is a reasonable guess for tests
+    # going to a pipe or a StringIO.
+    'TERM': 'dumb',
+    'LINES': '25',
+    'COLUMNS': '80',
+    'BZR_COLUMNS': '80',
+    # Disable SSH Agent
+    'SSH_AUTH_SOCK': None,
+    # Proxies
+    'http_proxy': None,
+    'HTTP_PROXY': None,
+    'https_proxy': None,
+    'HTTPS_PROXY': None,
+    'no_proxy': None,
+    'NO_PROXY': None,
+    'all_proxy': None,
+    'ALL_PROXY': None,
+    # Nobody cares about ftp_proxy, FTP_PROXY AFAIK. So far at
+    # least. If you do (care), please update this comment
+    # -- vila 20080401
+    'ftp_proxy': None,
+    'FTP_PROXY': None,
+    'BZR_REMOTE_PATH': None,
+    # Generally speaking, we don't want apport reporting on crashes in
+    # the test envirnoment unless we're specifically testing apport,
+    # so that it doesn't leak into the real system environment.  We
+    # use an env var so it propagates to subprocesses.
+    'APPORT_DISABLE': '1',
+    }
+
+
+def override_os_environ(test, env=None):
+    """Modify os.environ keeping a copy.
+    
+    :param test: A test instance
+
+    :param env: A dict containing variable definitions to be installed
+    """
+    if env is None:
+        env = isolated_environ
+    test._original_os_environ = dict([(var, value)
+                                      for var, value in os.environ.iteritems()])
+    for var, value in env.iteritems():
+        osutils.set_or_unset_env(var, value)
+        if var not in test._original_os_environ:
+            # The var is new, add it with a value of None, so
+            # restore_os_environ will delete it
+            test._original_os_environ[var] = None
+
+
+def restore_os_environ(test):
+    """Restore os.environ to its original state.
+
+    :param test: A test instance previously passed to override_os_environ.
+    """
+    for var, value in test._original_os_environ.iteritems():
+        # Restore the original value (or delete it if the value has been set to
+        # None in override_os_environ).
+        osutils.set_or_unset_env(var, value)
+
+
 class ExtendedTestResult(testtools.TextTestResult):
     """Accepts, reports and accumulates the results of running tests.
 
@@ -794,6 +881,25 @@
         return NullProgressView()
 
 
+def isolated_doctest_setUp(test):
+    override_os_environ(test)
+
+
+def isolated_doctest_tearDown(test):
+    restore_os_environ(test)
+
+
+def IsolatedDocTestSuite(*args, **kwargs):
+    """Overrides doctest.DocTestSuite to handle isolation.
+
+    The method is really a factory and users are expected to use it as such.
+    """
+    
+    kwargs['setUp'] = isolated_doctest_setUp
+    kwargs['tearDown'] = isolated_doctest_tearDown
+    return doctest.DocTestSuite(*args, **kwargs)
+
+
 class TestCase(testtools.TestCase):
     """Base class for bzr unit tests.
 
@@ -1538,55 +1644,7 @@
         return value
 
     def _cleanEnvironment(self):
-        new_env = {
-            'BZR_HOME': None, # Don't inherit BZR_HOME to all the tests.
-            'HOME': os.getcwd(),
-            # bzr now uses the Win32 API and doesn't rely on APPDATA, but the
-            # tests do check our impls match APPDATA
-            'BZR_EDITOR': None, # test_msgeditor manipulates this variable
-            'VISUAL': None,
-            'EDITOR': None,
-            'BZR_EMAIL': None,
-            'BZREMAIL': None, # may still be present in the environment
-            'EMAIL': '[e-mail]', # set EMAIL as bzr does not guess
-            'BZR_PROGRESS_BAR': None,
-            'BZR_LOG': None,
-            'BZR_PLUGIN_PATH': None,
-            'BZR_DISABLE_PLUGINS': None,
-            'BZR_PLUGINS_AT': None,
-            'BZR_CONCURRENCY': None,
-            # Make sure that any text ui tests are consistent regardless of
-            # the environment the test case is run in; you may want tests that
-            # test other combinations.  'dumb' is a reasonable guess for tests
-            # going to a pipe or a StringIO.
-            'TERM': 'dumb',
-            'LINES': '25',
-            'COLUMNS': '80',
-            'BZR_COLUMNS': '80',
-            # SSH Agent
-            'SSH_AUTH_SOCK': None,
-            # Proxies
-            'http_proxy': None,
-            'HTTP_PROXY': None,
-            'https_proxy': None,
-            'HTTPS_PROXY': None,
-            'no_proxy': None,
-            'NO_PROXY': None,
-            'all_proxy': None,
-            'ALL_PROXY': None,
-            # Nobody cares about ftp_proxy, FTP_PROXY AFAIK. So far at
-            # least. If you do (care), please update this comment
-            # -- vila 20080401
-            'ftp_proxy': None,
-            'FTP_PROXY': None,
-            'BZR_REMOTE_PATH': None,
-            # Generally speaking, we don't want apport reporting on crashes in
-            # the test envirnoment unless we're specifically testing apport,
-            # so that it doesn't leak into the real system environment.  We
-            # use an env var so it propagates to subprocesses.
-            'APPORT_DISABLE': '1',
-        }
-        for name, value in new_env.iteritems():
+        for name, value in isolated_environ.iteritems():
             self.overrideEnv(name, value)
 
     def _captureVar(self, name, newvalue):
@@ -3836,13 +3894,7 @@
         return []
     return [
         'bzrlib',
-        # 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.branchbuilder',
         'bzrlib.decorators',
         'bzrlib.export',
         'bzrlib.inventory',
@@ -3914,8 +3966,8 @@
         try:
             # note that this really does mean "report only" -- doctest
             # still runs the rest of the examples
-            doc_suite = doctest.DocTestSuite(mod,
-                optionflags=doctest.REPORT_ONLY_FIRST_FAILURE)
+            doc_suite = IsolatedDocTestSuite(
+                mod, optionflags=doctest.REPORT_ONLY_FIRST_FAILURE)
         except ValueError, e:
             print '**failed to get doctest for: %s\n%s' % (mod, e)
             raise
 
=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2010-12-26 16:23:16 +0000
+++ bzrlib/tests/test_selftest.py	2011-01-07 11:18:35 +0000
@@ -17,7 +17,7 @@
 """Tests for the test framework."""
 
 from cStringIO import StringIO
-from doctest import ELLIPSIS
+import doctest
 import os
 import signal
 import sys
@@ -92,7 +92,7 @@
             "text", "plain", {"charset": "utf8"})))
         self.assertThat(u"".join(log.iter_text()), Equals(self.get_log()))
         self.assertThat(self.get_log(),
-            DocTestMatches(u"...a test message\n", ELLIPSIS))
+            DocTestMatches(u"...a test message\n", doctest.ELLIPSIS))
 
 
 class TestUnicodeFilename(tests.TestCase):
@@ -3210,7 +3210,8 @@
         tpr.register('bar', 'bBB.aAA.rRR')
         self.assertEquals('bbb.aaa.rrr', tpr.get('bar'))
         self.assertThat(self.get_log(),
-            DocTestMatches("...bar...bbb.aaa.rrr...BB.aAA.rRR", ELLIPSIS))
+            DocTestMatches("...bar...bbb.aaa.rrr...BB.aAA.rRR",
+                           doctest.ELLIPSIS))
 
     def test_get_unknown_prefix(self):
         tpr = self._get_registry()
@@ -3453,3 +3454,121 @@
             self.fail(output.getvalue())
         # We get our value back
         self.assertEquals('42', os.environ.get('MYVAR'))
+
+
+class TestIsolatedEnv(tests.TestCase):
+    """Test isolating tests from os.environ.
+
+    Since we use tests that are already isolated from os.environ a bit of care
+    should be taken when designing the tests to avoid bootstrap side-effects.
+    The tests start an already clean os.environ which allow doing valid
+    assertions about which variables are present or not and design tests around
+    these assertions.
+    """
+
+    class ScratchMonkey(tests.TestCase):
+
+        def test_me(self):
+            pass
+
+    def test_basics(self):
+        # Make sure we know the definition of BZR_HOME: not part of os.environ
+        # for tests.TestCase.
+        self.assertTrue('BZR_HOME' in tests.isolated_environ)
+        self.assertEquals(None, tests.isolated_environ['BZR_HOME'])
+        # Being part of isolated_environ, BZR_HOME should not appear here
+        self.assertFalse('BZR_HOME' in os.environ)
+        # Make sure we know the definition of LINES: part of os.environ for
+        # tests.TestCase
+        self.assertTrue('LINES' in tests.isolated_environ)
+        self.assertEquals('25', tests.isolated_environ['LINES'])
+        self.assertEquals('25', os.environ['LINES'])
+
+    def test_injecting_unknown_variable(self):
+        # BZR_HOME is known to be absent from os.environ
+        test = self.ScratchMonkey('test_me')
+        tests.override_os_environ(test, {'BZR_HOME': 'foo'})
+        self.assertEquals('foo', os.environ['BZR_HOME'])
+        tests.restore_os_environ(test)
+        self.assertFalse('BZR_HOME' in os.environ)
+
+    def test_injecting_known_variable(self):
+        test = self.ScratchMonkey('test_me')
+        # LINES is known to be present in os.environ
+        tests.override_os_environ(test, {'LINES': '42'})
+        self.assertEquals('42', os.environ['LINES'])
+        tests.restore_os_environ(test)
+        self.assertEquals('25', os.environ['LINES'])
+
+    def test_deleting_variable(self):
+        test = self.ScratchMonkey('test_me')
+        # LINES is known to be present in os.environ
+        tests.override_os_environ(test, {'LINES': None})
+        self.assertTrue('LINES' not in os.environ)
+        tests.restore_os_environ(test)
+        self.assertEquals('25', os.environ['LINES'])
+
+
+class TestDocTestSuiteIsolation(tests.TestCase):
+    """Test that `tests.DocTestSuite` isolates doc tests from os.environ.
+
+    Since tests.TestCase alreay provides an isolation from os.environ, we use
+    the clean environment as a base for testing. To precisely capture the
+    isolation provided by tests.DocTestSuite, we use doctest.DocTestSuite to
+    compare against.
+
+    We want to make sure `tests.DocTestSuite` respect `tests.isolated_environ`,
+    not `os.environ` so each test overrides it to suit its needs.
+
+    """
+
+    def get_doctest_suite_for_string(self, klass, string):
+        class Finder(doctest.DocTestFinder):
+
+            def find(*args, **kwargs):
+                test = doctest.DocTestParser().get_doctest(
+                    string, {}, 'foo', 'foo.py', 0)
+                return [test]
+
+        suite = klass(test_finder=Finder())
+        return suite
+
+    def run_doctest_suite_for_string(self, klass, string):
+        suite = self.get_doctest_suite_for_string(klass, string)
+        output = StringIO()
+        result = tests.TextTestResult(output, 0, 1)
+        suite.run(result)
+        return result, output
+
+    def assertDocTestStringSucceds(self, klass, string):
+        result, output = self.run_doctest_suite_for_string(klass, string)
+        if not result.wasStrictlySuccessful():
+            self.fail(output.getvalue())
+
+    def assertDocTestStringFails(self, klass, string):
+        result, output = self.run_doctest_suite_for_string(klass, string)
+        if result.wasStrictlySuccessful():
+            self.fail(output.getvalue())
+
+    def test_injected_variable(self):
+        self.overrideAttr(tests, 'isolated_environ', {'LINES': '42'})
+        test = """
+            >>> import os
+            >>> os.environ['LINES']
+            '42'
+            """
+        # doctest.DocTestSuite fails as it sees '25'
+        self.assertDocTestStringFails(doctest.DocTestSuite, test)
+        # tests.DocTestSuite sees '42'
+        self.assertDocTestStringSucceds(tests.IsolatedDocTestSuite, test)
+
+    def test_deleted_variable(self):
+        self.overrideAttr(tests, 'isolated_environ', {'LINES': None})
+        test = """
+            >>> import os
+            >>> os.environ.get('LINES')
+            """
+        # doctest.DocTestSuite fails as it sees '25'
+        self.assertDocTestStringFails(doctest.DocTestSuite, test)
+        # tests.DocTestSuite sees None
+        self.assertDocTestStringSucceds(tests.IsolatedDocTestSuite, test)
 
=== modified file 'doc/developers/testing.txt'
--- doc/developers/testing.txt	2010-12-16 17:45:14 +0000
+++ doc/developers/testing.txt	2011-01-07 11:18:35 +0000
@@ -329,8 +329,11 @@
 We make selective use of doctests__.  In general they should provide
 *examples* within the API documentation which can incidentally be tested.  We
 don't try to test every important case using doctests |--| regular Python
-tests are generally a better solution.  That is, we just use doctests to
-make our documentation testable, rather than as a way to make tests.
+tests are generally a better solution.  That is, we just use doctests to make
+our documentation testable, rather than as a way to make tests. Be aware that
+doctests are not as well isolated as the unit tests, if you need more
+isolation, you're likely want to write unit tests anyway if only to get a
+better control of the test environment.
 
 Most of these are in ``bzrlib/doc/api``.  More additions are welcome.
 
 
=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-12-24 16:30:36 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2011-01-07 11:18:35 +0000
@@ -90,6 +90,13 @@
    suite.  This can include new facilities for writing tests, fixes to 
    spurious test failures and changes to the way things should be tested.
 
+* ``bzrlib.tests`` defines ``isolated_environ`` with the definitions of all
+  the environment variables the tests should care about. It also defines
+  ``override_os_environ`` and ``restore_os_environ`` to properly implement
+  isolation from ``os.environ`` for tests. ``bzrlib.tests`` now defines a
+  ``DocTestSuite`` class using this facility for all ``bzrlib``
+  doctests. (Vincent Ladeuil, #321320)
+
 * Catch exceptions related to bug #637821 during test cleanup to avoid
   spurious failures. (Vincent Ladeuil, #686008).