Rev 5583: Isolate doctests from os.environ in http://bazaar.launchpad.net/~vila/bzr/integration/
Vincent Ladeuil
v.ladeuil+lp at free.fr
Fri Jan 7 14:27:43 UTC 2011
At http://bazaar.launchpad.net/~vila/bzr/integration/
------------------------------------------------------------
revno: 5583 [merge]
revision-id: v.ladeuil+lp at free.fr-20110107142743-7k7duztdo654zcv3
parent: pqm at pqm.ubuntu.com-20101227125132-x1rjkygr6ufpflyn
parent: v.ladeuil+lp at free.fr-20110107111347-3ovv39zf4nf3ep0d
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: trunk
timestamp: Fri 2011-01-07 15:27:43 +0100
message:
Isolate doctests from os.environ
modified:
bzrlib/tests/__init__.py selftest.py-20050531073622-8d0e3c8845c97a64
bzrlib/tests/test_selftest.py test_selftest.py-20051202044319-c110a115d8c0456a
doc/developers/testing.txt testing.txt-20080812140359-i70zzh6v2z7grqex-1
doc/en/release-notes/bzr-2.3.txt NEWS-20050323055033-4e00b5db738777ff
-------------- next part --------------
=== modified file 'bzrlib/tests/__init__.py'
--- a/bzrlib/tests/__init__.py 2010-12-26 16:23:16 +0000
+++ b/bzrlib/tests/__init__.py 2011-01-07 11:13:47 +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': 'jrandom at example.com', # 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': 'jrandom at example.com', # 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'
--- a/bzrlib/tests/test_selftest.py 2010-12-26 16:23:16 +0000
+++ b/bzrlib/tests/test_selftest.py 2011-01-07 11:13:47 +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'
--- a/doc/developers/testing.txt 2010-12-16 17:45:14 +0000
+++ b/doc/developers/testing.txt 2011-01-03 13:13:50 +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'
--- a/doc/en/release-notes/bzr-2.3.txt 2010-12-24 16:30:36 +0000
+++ b/doc/en/release-notes/bzr-2.3.txt 2011-01-07 11:13:47 +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).
More information about the bazaar-commits
mailing list