RA layer independence for the Python test suite

On Thu, 25 Oct 2007, Eric Gillespie wrote:
...
> This is the third or fourth time I've hacked ra-independence into
> the Python binding test suite. This time, I tried to do it in a
> bit less of a hacky way. I'm going to clean it up again and post
> a proposal in a new thread, probably not today.
>
> Index: subversion/bindings/swig/python/tests/trac/versioncontrol/tests/svn_fs.py
> ===================================================================
> --- subversion/bindings/swig/python/tests/trac/versioncontrol/tests/svn_fs.py (revision 27391)
> +++ subversion/bindings/swig/python/tests/trac/versioncontrol/tests/svn_fs.py (working copy)
...
> @@ -30,21 +31,24 @@
> from trac.versioncontrol import Changeset, Node
> from trac.versioncontrol.svn_fs import SubversionRepository
>
> -REPOS_PATH = tempfile.mktemp("-trac-svnrepos")
> -REPOS_URL = pathname2url(REPOS_PATH)
> -if REPOS_URL.startswith("///"):
> - # Don't add extra slashes if they're already present.
> - # (This is important for Windows compatibility).
> - REPOS_URL = "file:" + REPOS_URL
> -else:
> - # If the URL simply starts with '/', we need to add two
> - # extra slashes to make it a valid 'file://' URL
> - REPOS_URL = "file://" + REPOS_URL
> +this_dir = os.path.dirname(__file__)
> +REPOS_BASE_DIR = os.path.normpath(
> + os.path.join(this_dir, '..', '..', '..', '..', '..', '..', '..', 'tests',
> + 'cmdline', 'svn-test-work', 'repositories'))

How well does this hold up for vpath builds?

> +_REPOS_PATH = None
> +_REPOS_URL = None
>
> +def REPOS_PATH():
> + return _REPOS_PATH
> +def REPOS_URL():
> + return _REPOS_URL
  
Function names uppercase to denote macro-ish-ness, or just a
carry-over...?

> +
> class SubversionRepositoryTestSetup(TestSetup):
>
> def setUp(self):
> + global _REPOS_PATH, _REPOS_URL
> +

Any particular reason not to use "global" down near where the
variables are actually used (e.g. an early hint to readers that this
method modifies global data)?

> dumpfile = open(os.path.join(os.path.split(__file__)[0],
> 'svnrepos.dump'))
>
> @@ -52,20 +56,42 @@
> # ensure a fresh start.
> self.tearDown()
>
> - r = repos.svn_repos_create(REPOS_PATH, '', '', None, None)
> + try:
> + os.makedirs(REPOS_BASE_DIR)
> + except OSError, e:
> + if e.errno != EEXIST:
> + raise
> + _REPOS_PATH = tempfile.mkdtemp(dir=REPOS_BASE_DIR, prefix="trac-tests-")
> + r = repos.svn_repos_create(_REPOS_PATH, '', '', None, None)
> repos.svn_repos_load_fs2(r, dumpfile, StringIO(),
> repos.svn_repos_load_uuid_default, '',
> 0, 0, None)
>
> + url = os.environ.get('TRAC_TESTS_URL')
> + if url == None:
> + _REPOS_URL = pathname2url(_REPOS_PATH)
> + if _REPOS_URL.startswith("///"):
> + # Don't add extra slashes if they're already present.
> + # (This is important for Windows compatibility).
> + _REPOS_URL = "file:" + _REPOS_URL
> + else:
> + # If the URL simply starts with '/', we need to add two
> + # extra slashes to make it a valid 'file://' URL
> + _REPOS_URL = "file://" + _REPOS_URL
> + else:
> + _REPOS_URL = '/'.join([url, 'svn-test-work/repositories',
> + os.path.basename(_REPOS_PATH)])
> +
> +
> def tearDown(self):
> - if os.path.exists(REPOS_PATH):
> - repos.delete(REPOS_PATH)
> + if _REPOS_PATH != None and os.path.exists(_REPOS_PATH):

You could use "is not None" instead of "!= None", since None is always
the same "object".

...
> Index: subversion/bindings/swig/python/tests/wc.py
> ===================================================================
> --- subversion/bindings/swig/python/tests/wc.py (revision 27391)
> +++ subversion/bindings/swig/python/tests/wc.py (working copy)
> @@ -1,11 +1,17 @@
> -import unittest, os, tempfile, shutil, types, setup_path, binascii
> +import unittest, os, tempfile, shutil, types, setup_path, binascii, re

Any reason not to use the "sre" module instead of "re", since "re" has
been deprecated? (I also tend to use "re" myself, out of habit.)

...
> +def run_svn(*args):
> + argv = map(str, args)
> + argv.insert(0, 'subversion/svn/svn -q --username jrandom --password rayjandom')
> + return os.system(' '.join(argv))
> +
...
> @@ -28,8 +34,7 @@
> rev = core.svn_opt_revision_t()
> rev.kind = core.svn_opt_revision_head
>
> - client.checkout2(REPOS_URL, self.path, rev, rev, True, True,
> - client_ctx)
> + run_svn('co', REPOS_URL(), self.path)

Instead, how about extending "client" in a manner which supplies the
appropriate credentials (e.g. via a callback implementation which
hard-codes them), rather than forking the command-line binary?

...
> @@ -256,12 +261,96 @@
> self.assertEquals(entry.cmt_date,
> core.svn_time_from_cstring(commit_info.date))
>
> + def test_commit_out_of_date(self):
> + readme_path = '%s/trunk/README.txt' % self.path
> + readme_rev = 2
> +
> + # Close wc, backdate readme_path, and re-open wc.
> + wc.adm_close(self.wc)
> + run_svn('up', '-r', readme_rev, readme_path)

The above suggestion might also be able to handle this case.

> + self.wc = wc.adm_open3(None, self.path, True, -1, None)
> +
> + # Replace README.txt's contents.
> + fp = open(readme_path, 'w')
> + fp.write('test_commit_out_of_date\n')
> + fp.close()
> +
> + # Setup ra_ctx.
> + ra.initialize()
> + callbacks = ra.Callbacks()
> + def open_tmp_file(pool):
> + (fd, fn) = tempfile.mkstemp(dir=self.path)
> + os.close(fd)
> + return fn
> + callbacks.open_tmp_file = open_tmp_file
> + svn_config_dir = os.path.expanduser('~/.subversion')
> + svn_config = core.svn_config_get_config(svn_config_dir)
> + providers = [
> + client.get_simple_provider(),
> + client.get_username_provider(),
> + ]
> + auth_baton = core.svn_auth_open(providers)
> + core.svn_auth_set_parameter(auth_baton,
> + core.SVN_AUTH_PARAM_CONFIG_DIR,
> + svn_config_dir)
> + core.svn_auth_set_parameter(auth_baton,
> + core.SVN_AUTH_PARAM_DEFAULT_USERNAME,
> + 'jrandom')
> + core.svn_auth_set_parameter(auth_baton,
> + core.SVN_AUTH_PARAM_DEFAULT_PASSWORD,
> + 'rayjandom')
> + core.svn_auth_set_parameter(auth_baton,
> + core.SVN_AUTH_PARAM_NON_INTERACTIVE,
> + '')

We also have the user "jconstant" in our command-line test suite.

The above code block looks like it could be factored into one or two
helper functions.

> + callbacks.auth_baton = auth_baton
> + svn_config_dir = os.path.expanduser('~/.subversion')
> + svn_config = core.svn_config_get_config(svn_config_dir)

We get svn_config_dir and svn_config above. If we need to do it
again, let's document why.

> + ra_ctx = ra.open2(REPOS_URL(), callbacks, svn_config, None)
> +
> + # Get commit editor.
> + def commit_cb(_commit_info, pool):
> + self.fail('Commit succeeded, should have been out of date.')
> + (editor, edit_baton) = ra.get_commit_editor2(ra_ctx, 'log message',
> + commit_cb,
> + None,
> + False)
> + try:
> + # Drive the commit.
> + try:
> + def driver_cb(parent, path, pool):
> + baton = editor.open_file(path, parent, readme_rev, pool)
> + adm_access = wc.adm_probe_retrieve(self.wc, readme_path, pool)
> + wc.transmit_text_deltas2(readme_path, adm_access, False, editor,
> + baton, pool)
> + return baton
> + delta.path_driver(editor, edit_baton, -1, ['trunk/README.txt'],
> + driver_cb)
> + editor.close_edit(edit_baton)
> + except SubversionException, e:
> + if e.apr_err in [
> + # ra-{local,svn} raise this
> + core.SVN_ERR_FS_TXN_OUT_OF_DATE,
> + # ra-neon (and serf?) raise this
> + core.SVN_ERR_FS_CONFLICT,
> + ]:
> + self.assert_('trunk/README.txt' in e.args[0])
> + else:
> + raise
> + except:
> + try:
> + editor.abort_edit(edit_baton)
> + except:
> + # We already have an exception in progress, not much we can do
> + # about this.
> + pass
> + raise
> +
> def tearDown(self):
> wc.adm_close(self.wc)
> core.svn_io_remove_dir(self.path)
>
> def suite():
> - return unittest.makeSuite(SubversionWorkingCopyTestCase, 'test')
> + return unittest.makeSuite(SubversionWorkingCopyTestCase, 'test_commit_o')

This test case name was probably meant to be test_commit_out_of_date.

>
> if __name__ == '__main__':
> runner = unittest.TextTestRunner()