Noorul Islam K M <noo...@collab.net> writes:
> Noorul Islam K M <noo...@collab.net> writes:
>
>> Philip Martin <philip.mar...@wandisco.com> writes:
>>
>>> Noorul Islam K M <noo...@collab.net> writes:
>>>
>>>> Test cases are written using python unittest framework and it has two
>>>> methods, setUp() and tearDown() which gets executed for every case. In
>>>> tearDown(), repository which is created in setUp() is deleted using
>>>> svn_repos_delete(). During first iteration there are no issues but in
>>>> the second iteration (test case), the system throws the above mentioned
>>>> error. Using lsof command I could see something like this
>>>>
>>>> python 18111 noorul 4u REG 8,1 5120 279333
>>>> /tmp/svn_test_repos/db/revp
>>>> rops/revprops.db (deleted)
>>>>
>>>> Does this mean that the sqlite file pointers are not completely
>>>> destroyed?
>>>
>>> Yes. The repository handle from the previous svn_repos_create function
>>> is still around when svn_repos_delete is called.
>>>
>>> This is a new problem caused by the revprop packing. Also, there
>>> doesn't appear to be an API for explicitly closing the repository
>>> handle. Solutions include:
>>>
>>> - adding an svn_repos_close API
>>> - clearing or destroying the pool passed to svn_repos_create
>>> - having the test create repositories at different locations
>>
>> I have implemented the last solution and attached is the patch for the
>> same.
>>
>> Log
>>
>> [[[
>>
>> Modify test cases to overcome limitations in closing repository handle.
>>
>> * subversion/bindings/ctypes-python/test/wc.py:
>> * subversion/bindings/ctypes-python/test/localrepos.py:
>> * subversion/bindings/ctypes-python/test/remoterepos.py: Use random
>> repository location during each iteration.
>>
>> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
>>
>> ]]]
>>
>> Thanks and Regards
>> Noorul
>>
>> Index: build/run_ctypesgen.sh
>> ===================================================================
>> --- build/run_ctypesgen.sh (revision 1028789)
>> +++ build/run_ctypesgen.sh (working copy)
>> @@ -86,4 +86,4 @@
>> (cat $abs_srcdir/$cp_relpath/csvn/core/functions.py.in; \
>> sed -e '/^FILE =/d' $output | \
>> perl -pe 's{(\s+\w+)\.restype = POINTER\(svn_error_t\)}{\1.restype =
>> POINTER(svn_error_t)\n\1.errcheck = _svn_errcheck}' \
>> - ) > $abs_builddir/$cp_relpath/csvn/core/functions.py
>> + ) > $abs_srcdir/$cp_relpath/csvn/core/functions.py
>
> This patch should not include build/run_ctypesgen.sh file. Sorry for the
> mess. Attached is the revised one.
>
> Thanks and Regards
> Noorul
>
> Index: subversion/bindings/ctypes-python/test/wc.py
> ===================================================================
> --- subversion/bindings/ctypes-python/test/wc.py (revision 1028789)
> +++ subversion/bindings/ctypes-python/test/wc.py (working copy)
> @@ -22,6 +22,7 @@
> import os
> import shutil
> import tempfile
> +import random
> from sys import version_info # For Python version check
> if version_info[0] >= 3:
> # Python >=3.0
> @@ -36,22 +37,6 @@
>
> locale.setlocale(locale.LC_ALL, "C")
>
> -repos_location = os.path.join(tempfile.gettempdir(), "svn_test_repos")
> -wc_location = os.path.join(tempfile.gettempdir(), "svn_test_wc")
> -repo_url = pathname2url(repos_location)
> -if repo_url.startswith("///"):
> - # Don't add extra slashes if they're already present.
> - # (This is important for Windows compatibility).
> - repo_url = "file:" + repo_url
> -else:
> - # If the URL simply starts with '/', we need to add two
> - # extra slashes to make it a valid 'file://' URL
> - repo_url = "file://" + repo_url
> -
> -if os.sep != "/":
> - repos_location = repos_location.replace(os.sep, "/")
> - wc_location = wc_location.replace(os.sep, "/")
> -
> class WCTestCase(unittest.TestCase):
> """Test case for Subversion WC layer."""
>
> @@ -59,20 +44,24 @@
> dumpfile = open(os.path.join(os.path.split(__file__)[0],
> 'test.dumpfile'))
>
> + self.repos_location = self.get_repos_location()
> + self.repos_url = self.get_repos_url(self.repos_location)
> + self.wc_location = self.get_wc_location()
> +
> # Just in case a preivous test instance was not properly cleaned up
> self.tearDown()
> - self.repos = LocalRepository(repos_location, create=True)
> + self.repos = LocalRepository(self.repos_location, create=True)
> self.repos.load(dumpfile)
>
> - self.wc = WC(wc_location)
> - self.wc.checkout(repo_url)
> + self.wc = WC(self.wc_location)
> + self.wc.checkout(self.repos_url)
>
> def tearDown(self):
> pool = Pool()
> - if os.path.exists(wc_location):
> - svn_io_remove_dir(wc_location, pool)
> - if os.path.exists(repos_location):
> - svn_repos_delete(repos_location, pool)
> + if os.path.exists(self.wc_location):
> + svn_io_remove_dir(self.wc_location, pool)
> + if os.path.exists(self.repos_location):
> + svn_repos_delete(self.repos_location, pool)
> self.wc = None
>
> def _info_receiver(self, path, info):
> @@ -82,7 +71,7 @@
> self.wc.info(path="trunk/README.txt",info_func=self._info_receiver)
> self.assertEqual(9, self.last_info.rev)
> self.assertEqual(svn_node_file, self.last_info.kind)
> - self.assertEqual(repo_url, self.last_info.repos_root_URL)
> + self.assertEqual(self.repos_url, self.last_info.repos_root_URL)
> self.assertEqual("890f2569-e600-4cfc-842a-f574dec58d87",
> self.last_info.repos_UUID)
> self.assertEqual(9, self.last_info.last_changed_rev)
> @@ -118,7 +107,7 @@
> self.assertEqual(svn_wc_schedule_add, self.last_info.schedule)
>
> def test_add(self):
> - f = open("%s/trunk/ADDED.txt" % wc_location, "w")
> + f = open("%s/trunk/ADDED.txt" % self.wc_location, "w")
> f.write("Something")
> f.close()
>
> @@ -134,7 +123,7 @@
> self.assertEqual(svn_wc_schedule_normal, self.last_info.schedule)
>
> def test_diff(self):
> - path = "%s/trunk/README.txt" % wc_location
> + path = "%s/trunk/README.txt" % self.wc_location
>
> diffstring="""Index: """+path+"""
> ===================================================================
> @@ -158,7 +147,7 @@
> diffresult = difffile.read().replace("\r","")
> self.assertEqual(diffstring, diffresult)
>
> - path = "%s/branches/0.x/README.txt" % wc_location
> + path = "%s/branches/0.x/README.txt" % self.wc_location
> diffstring="""Index: """+path+"""
> ===================================================================
> --- """+path+"""\t(revision 0)
> @@ -191,14 +180,14 @@
>
> def test_propget(self):
> props = self.wc.propget("Awesome")
> - path = "%s/trunk/README.txt" % wc_location
> + path = "%s/trunk/README.txt" % self.wc_location
> if not path in props.keys():
> self.fail("File missing in propget")
>
> def test_propset(self):
> self.wc.propset("testprop", "testval", "branches/0.x/README.txt")
> props = self.wc.propget("testprop", "branches/0.x/README.txt")
> - if not "%s/branches/0.x/README.txt" % wc_location in \
> + if not "%s/branches/0.x/README.txt" % self.wc_location in \
> props.keys():
>
> self.fail("Property not set")
> @@ -208,7 +197,7 @@
> results = self.wc.update([path], revnum=7)
> self.assertEqual(results[0], 7)
> props = self.wc.propget("Awesome")
> - if "%s/%s" % (wc_location, path) in \
> + if "%s/%s" % (self.wc_location, path) in \
> props.keys():
> self.fail("File not updated to old revision")
> results = self.wc.update([path])
> @@ -216,12 +205,12 @@
> self.test_propget()
>
> def test_switch(self):
> - self.wc.switch("trunk", "%s/tags" % repo_url)
> - if os.path.exists("%s/trunk/README.txt" % wc_location):
> + self.wc.switch("trunk", "%s/tags" % self.repos_url)
> + if os.path.exists("%s/trunk/README.txt" % self.wc_location):
> self.fail("Switch did not happen")
>
> def test_lock(self):
> - self.wc.lock(["%s/trunk/README.txt" % wc_location],
> + self.wc.lock(["%s/trunk/README.txt" % self.wc_location],
> "Test lock")
> self.wc.info(path="trunk/README.txt",
> info_func=self._info_receiver)
> @@ -229,7 +218,7 @@
> self.fail("Lock not aquired")
>
> def test_unlock(self):
> - path = "%s/trunk/README.txt" % wc_location
> + path = "%s/trunk/README.txt" % self.wc_location
> self.wc.lock([path], "Test lock")
> self.wc.info(path=path,
> info_func=self._info_receiver)
> @@ -242,7 +231,29 @@
> if self.last_info.lock:
> self.fail("Lock not released")
>
> + @staticmethod
> + def get_repos_location():
> + return os.path.join(tempfile.gettempdir(),
> + "svn_test_repos_%f" % random.random())
>
> + @staticmethod
> + def get_repos_url(repos_location):
> + repos_url = pathname2url(repos_location)
> + 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
> + return repos_url
> +
> + @staticmethod
> + def get_wc_location():
> + return os.path.join(tempfile.gettempdir(),
> + "svn_test_wc_%f" % random.random())
> +
> def suite():
> return unittest.makeSuite(WCTestCase, 'test')
>
> Index: subversion/bindings/ctypes-python/test/localrepos.py
> ===================================================================
> --- subversion/bindings/ctypes-python/test/localrepos.py (revision
> 1028789)
> +++ subversion/bindings/ctypes-python/test/localrepos.py (working copy)
> @@ -21,26 +21,26 @@
> import os
> import shutil
> import tempfile
> +import random
> from csvn.core import *
> from urllib import pathname2url
> from csvn.repos import LocalRepository, RemoteRepository
>
> -repos_location = os.path.join(tempfile.gettempdir(), "svn_test_repos")
> -
> class LocalRepositoryTestCase(unittest.TestCase):
>
> def setUp(self):
> + self.repos_location = self.get_repos_location()
> dumpfile = open(os.path.join(os.path.split(__file__)[0],
> 'test.dumpfile'))
>
> # Just in case a preivous test instance was not properly cleaned up
> self.tearDown()
> - self.repos = LocalRepository(repos_location, create=True)
> + self.repos = LocalRepository(self.repos_location, create=True)
> self.repos.load(dumpfile)
>
> def tearDown(self):
> - if os.path.exists(repos_location):
> - svn_repos_delete(repos_location, Pool())
> + if os.path.exists(self.repos_location):
> + svn_repos_delete(self.repos_location, Pool())
> self.repos = None
>
> def test_local_latest_revnum(self):
> @@ -62,6 +62,11 @@
> self.assertEqual(svn_node_none,
> self.repos.check_path("does_not_compute"))
>
> + @staticmethod
> + def get_repos_location():
> + return os.path.join(tempfile.gettempdir(),
> + "svn_test_repos_%f" % random.random())
> +
> def suite():
> return unittest.makeSuite(LocalRepositoryTestCase, 'test')
>
> Index: subversion/bindings/ctypes-python/test/remoterepos.py
> ===================================================================
> --- subversion/bindings/ctypes-python/test/remoterepos.py (revision
> 1028789)
> +++ subversion/bindings/ctypes-python/test/remoterepos.py (working copy)
> @@ -23,39 +23,31 @@
> import shutil
> import tempfile
> import sys
> +import random
> from csvn.core import *
> from urllib import pathname2url
> from csvn.repos import LocalRepository, RemoteRepository
> from stat import *
>
> -repos_location = os.path.join(tempfile.gettempdir(), "svn_test_repos")
> -repos_url = pathname2url(repos_location)
> -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
> -
> -
> class RemoteRepositoryTestCase(unittest.TestCase):
>
> def setUp(self):
> dumpfile = open(os.path.join(os.path.split(__file__)[0],
> 'test.dumpfile'))
>
> + self.repos_location = self.get_repos_location()
> + self.repos_url = self.get_repos_url(self.repos_location)
> +
> # Just in case a preivous test instance was not properly cleaned up
> self.tearDown()
> - self.repos = LocalRepository(repos_location, create=True)
> + self.repos = LocalRepository(self.repos_location, create=True)
> self.repos.load(dumpfile)
>
> - self.repos = RemoteRepository(repos_url)
> + self.repos = RemoteRepository(self.repos_url)
>
> def tearDown(self):
> - if os.path.exists(repos_location):
> - svn_repos_delete(repos_location, Pool())
> + if os.path.exists(self.repos_location):
> + svn_repos_delete(self.repos_location, Pool())
> self.repos = None
>
> def test_remote_latest_revnum(self):
> @@ -98,12 +90,12 @@
> # For revprops to be changeable, we need to have a hook.
> # We'll make a hook that accepts anything
> if sys.platform == "win32":
> - hook = os.path.join(repos_location, "hooks",
> "pre-revprop-change.bat")
> + hook = os.path.join(self.repos_location, "hooks",
> "pre-revprop-change.bat")
> f = open(hook, "w")
> f.write("@exit")
> f.close()
> else:
> - hook = os.path.join(repos_location, "hooks",
> "pre-revprop-change")
> + hook = os.path.join(self.repos_location, "hooks",
> "pre-revprop-change")
> f = open(hook, "w")
> f.write("#!/bin/sh\nexit 0;")
> f.close()
> @@ -133,9 +125,26 @@
> f = open(newfile, "w")
> f.write("Some new stuff\n")
> f.close()
> - commit_info = self.repos.svnimport(newfile, "%s/newfile.txt" %
> repos_url, log_func=self._log_func)
> + commit_info = self.repos.svnimport(newfile, "%s/newfile.txt" %
> self.repos_url, log_func=self._log_func)
> self.assertEqual(commit_info.revision, 10)
>
> + @staticmethod
> + def get_repos_location():
> + return os.path.join(tempfile.gettempdir(),
> + "svn_test_repos_%f" % random.random())
> + @staticmethod
> + def get_repos_url(repos_location):
> + repos_url = pathname2url(repos_location)
> + 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
> + return repos_url
> +
> def suite():
> return unittest.makeSuite(RemoteRepositoryTestCase, 'test')
>
Are we waiting for someone to implement any other solution?
Thanks and Regards
Noorul
