From 54a286b41744fbb1bc5296611d72a33d889a681b Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Tue, 28 Nov 2017 11:00:23 +0100 Subject: [PATCH] C extension reference leak testing Add documentation how to run refleak tests with pydebug build of CPython and pytest-refleak. Refleak testing runs each unit test multiple times. Some small changes are required to clean up after each test cycle and to avoid extra references. * Only create logging structure for SlapdObject once. The logging framework creates reference cycles that cause false positives. * Clean up and remove atexit handlers in SlapdObject.stop() * Remove new entries at the end of each test, either with an explicit delete_s() or an write area, which is removed in tearDown(). https://github.com/python-ldap/python-ldap/pull/41 Signed-off-by: Christian Heimes --- INSTALL | 34 ++++++++++++++++++++++++ Lib/slapdtest.py | 44 ++++++++++++++++++++++++++----- Tests/t_cext.py | 68 ++++++++++++++++++++++++++++++++++++------------ Tests/t_edit.py | 6 +++++ 4 files changed, 129 insertions(+), 23 deletions(-) diff --git a/INSTALL b/INSTALL index 98dd40c..57b00b7 100644 --- a/INSTALL +++ b/INSTALL @@ -24,3 +24,37 @@ Quick build instructions: edit setup.cfg (see Build/ for platform-specific examples) python setup.py build python setup.py install + +-------------------- +Reference leak tests +-------------------- + +Reference leak tests require a pydebug build of CPython and pytest with +pytest-leaks plugin. A pydebug build has a global reference counter, which +keeps track of all reference increments and decrements. The leak plugin runs +each tests multiple times and checks if the reference count increases. + +Download and compile pydebug build +---------------------------------- + +- curl -O https://www.python.org/ftp/python/3.6.3/Python-3.6.3.tar.xz +- tar xJf Python-3.6.3.tar.xz +- cd Python-3.6.3 +- ./configure --with-pydebug +- make + +Create virtual env +------------------ + +- ./python -m venv /tmp/refleak +- /tmp/refleak/bin/pip install pytest pytest-leaks + +Run refleak tests +----------------- + +- cd path/to/python-ldap +- /tmp/refleak/bin/pip install --upgrade . +- /tmp/refleak/bin/pytest -v -R: Tests/t_*.py + +Run ``/tmp/refleak/bin/pip install --upgrade .`` every time a file outside +of ``Tests/`` is modified. \ No newline at end of file diff --git a/Lib/slapdtest.py b/Lib/slapdtest.py index 4955420..0bef1c1 100644 --- a/Lib/slapdtest.py +++ b/Lib/slapdtest.py @@ -117,6 +117,7 @@ class SlapdObject(object): else: SCHEMADIR = None PATH_LDAPADD = os.path.join(BINDIR, 'ldapadd') + PATH_LDAPDELETE = os.path.join(BINDIR, 'ldapdelete') PATH_LDAPMODIFY = os.path.join(BINDIR, 'ldapmodify') PATH_LDAPWHOAMI = os.path.join(BINDIR, 'ldapwhoami') PATH_SLAPD = os.environ.get('SLAPD', os.path.join(SBINDIR, 'slapd')) @@ -125,8 +126,10 @@ class SlapdObject(object): # time in secs to wait before trying to access slapd via LDAP (again) _start_sleep = 1.5 + # create loggers once, multiple calls mess up refleak tests + _log = combined_logger('python-ldap-test') + def __init__(self): - self._log = combined_logger('python-ldap-test') self._proc = None self._port = self._avail_tcp_port() self.server_id = self._port % 4096 @@ -189,9 +192,11 @@ def _avail_tcp_port(self): find an available port for TCP connection """ sock = socket.socket() - sock.bind((self.local_host, 0)) - port = sock.getsockname()[1] - sock.close() + try: + sock.bind((self.local_host, 0)) + port = sock.getsockname()[1] + finally: + sock.close() self._log.info('Found available port %d', port) return port @@ -316,6 +321,15 @@ def stop(self): self._proc.terminate() self.wait() self._cleanup_rundir() + if hasattr(atexit, 'unregister'): + # Python 3 + atexit.unregister(self.stop) + elif hasattr(atexit, '_exithandlers'): + # Python 2, can be None during process shutdown + try: + atexit._exithandlers.remove(self.stop) + except ValueError: + pass def restart(self): """ @@ -358,7 +372,10 @@ def _cli_popen(self, ldapcommand, extra_args=None, ldap_uri=None, stdin_data=Non '-H', ldap_uri or self.ldapi_uri, ] + self._cli_auth_args() + (extra_args or []) self._log.debug('Run command: %r', ' '.join(args)) - proc = subprocess.Popen(args, stdin=subprocess.PIPE, stdout=subprocess.PIPE) + proc = subprocess.Popen( + args, stdin=subprocess.PIPE, stdout=subprocess.PIPE, + stderr=subprocess.PIPE + ) self._log.debug('stdin_data=%r', stdin_data) stdout_data, stderr_data = proc.communicate(stdin_data) if stdout_data is not None: @@ -366,7 +383,11 @@ def _cli_popen(self, ldapcommand, extra_args=None, ldap_uri=None, stdin_data=Non if stderr_data is not None: self._log.debug('stderr_data=%r', stderr_data) if proc.wait() != 0: - raise RuntimeError('ldapadd process failed') + raise RuntimeError( + '{!r} process failed:\n{!r}\n{!r}'.format( + args, stdout_data, stderr_data + ) + ) return stdout_data, stderr_data def ldapwhoami(self, extra_args=None): @@ -389,6 +410,17 @@ def ldapmodify(self, ldif, extra_args=None): self._cli_popen(self.PATH_LDAPMODIFY, extra_args=extra_args, stdin_data=ldif.encode('utf-8')) + def ldapdelete(self, dn, recursive=False, extra_args=None): + """ + Runs ldapdelete on this slapd instance, deleting 'dn' + """ + if extra_args is None: + extra_args = [] + if recursive: + extra_args.append('-r') + extra_args.append(dn) + self._cli_popen(self.PATH_LDAPDELETE, extra_args=extra_args) + class SlapdTestCase(unittest.TestCase): """ diff --git a/Tests/t_cext.py b/Tests/t_cext.py index 5aeb03d..6998e73 100644 --- a/Tests/t_cext.py +++ b/Tests/t_cext.py @@ -51,6 +51,40 @@ def setUpClass(cls): ]) ) + def setUp(self): + super(TestLdapCExtension, self).setUp() + self._writesuffix = None + + def tearDown(self): + # cleanup test subtree + if self._writesuffix is not None: + self.server.ldapdelete(self._writesuffix, recursive=True) + super(TestLdapCExtension, self).tearDown() + + @property + def writesuffix(self): + """Initialize writesuffix on demand + + Creates a clean subtree for tests that write to slapd. ldapdelete + is not able to delete a Root DSE, therefore we need a temporary + work space. + + :return: DN + """ + if self._writesuffix is not None: + return self._writesuffix + self._writesuffix = 'ou=write tests,%s' % self.server.suffix + # Add writeable subtree + self.server.ldapadd( + "\n".join([ + 'dn: ' + self._writesuffix, + 'objectClass: organizationalUnit', + 'ou:' + self._writesuffix.split(',')[0][3:], + '' + ]) + ) + return self._writesuffix + def _open_conn(self, bind=True): """ Starts a server, and returns a LDAPObject bound to it @@ -285,7 +319,7 @@ def test_add(self): """ l = self._open_conn() m = l.add_ext( - "cn=Foo," + self.server.suffix, + "cn=Foo," + self.writesuffix, [ ('objectClass', b'organizationalRole'), ('cn', b'Foo'), @@ -299,7 +333,7 @@ def test_add(self): self.assertEqual(msgid, m) self.assertEqual(ctrls, []) # search for it back - m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=Foo)') + m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=Foo)') result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout) # Expect to get the objects self.assertEqual(result, _ldap.RES_SEARCH_RESULT) @@ -309,7 +343,7 @@ def test_add(self): self.assertEqual( pmsg[0], ( - 'cn=Foo,'+self.server.suffix, + 'cn=Foo,'+self.writesuffix, { 'objectClass': [b'organizationalRole'], 'cn': [b'Foo'], @@ -324,7 +358,7 @@ def test_compare(self): """ l = self._open_conn() # first, add an object with a field we can compare on - dn = "cn=CompareTest," + self.server.suffix + dn = "cn=CompareTest," + self.writesuffix m = l.add_ext( dn, [ @@ -378,7 +412,7 @@ def test_delete_no_such_object(self): def test_delete(self): l = self._open_conn() # first, add an object we will delete - dn = "cn=Deleteme,"+self.server.suffix + dn = "cn=Deleteme,"+self.writesuffix m = l.add_ext( dn, [ @@ -402,7 +436,7 @@ def test_modify_no_such_object(self): # try deleting an object that doesn't exist m = l.modify_ext( - "cn=DoesNotExist,"+self.server.suffix, + "cn=DoesNotExist,"+self.writesuffix, [ (_ldap.MOD_ADD, 'description', [b'blah']), ] @@ -439,7 +473,7 @@ def test_modify(self): """ l = self._open_conn() # first, add an object we will delete - dn = "cn=AddToMe,"+self.server.suffix + dn = "cn=AddToMe,"+self.writesuffix m = l.add_ext( dn, [ @@ -465,7 +499,7 @@ def test_modify(self): self.assertEqual(msgid, m) self.assertEqual(ctrls, []) # search for it back - m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=AddToMe)') + m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=AddToMe)') result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout) # Expect to get the objects self.assertEqual(result, _ldap.RES_SEARCH_RESULT) @@ -479,7 +513,7 @@ def test_modify(self): def test_rename(self): l = self._open_conn() - dn = "cn=RenameMe,"+self.server.suffix + dn = "cn=RenameMe,"+self.writesuffix m = l.add_ext( dn, [ @@ -500,7 +534,7 @@ def test_rename(self): self.assertEqual(ctrls, []) # make sure the old one is gone - m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=RenameMe)') + m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=RenameMe)') result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout) self.assertEqual(result, _ldap.RES_SEARCH_RESULT) self.assertEqual(len(pmsg), 0) # expect no results @@ -508,8 +542,8 @@ def test_rename(self): self.assertEqual(ctrls, []) # check that the new one looks right - dn2 = "cn=IAmRenamed,"+self.server.suffix - m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamed)') + dn2 = "cn=IAmRenamed,"+self.writesuffix + m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamed)') result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout) self.assertEqual(result, _ldap.RES_SEARCH_RESULT) self.assertEqual(msgid, m) @@ -519,7 +553,7 @@ def test_rename(self): self.assertEqual(pmsg[0][1]['cn'], [b'IAmRenamed']) # create the container - containerDn = "ou=RenameContainer,"+self.server.suffix + containerDn = "ou=RenameContainer,"+self.writesuffix m = l.add_ext( containerDn, [ @@ -542,18 +576,18 @@ def test_rename(self): self.assertEqual(ctrls, []) # make sure dn2 is gone - m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamed)') + m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamed)') result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout) self.assertEqual(result, _ldap.RES_SEARCH_RESULT) self.assertEqual(len(pmsg), 0) # expect no results self.assertEqual(msgid, m) self.assertEqual(ctrls, []) - m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(objectClass=*)') + m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(objectClass=*)') result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout) # make sure dn3 is there - m = l.search_ext(self.server.suffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamedAgain)') + m = l.search_ext(self.writesuffix, _ldap.SCOPE_SUBTREE, '(cn=IAmRenamedAgain)') result, pmsg, msgid, ctrls = l.result4(m, _ldap.MSG_ALL, self.timeout) self.assertEqual(result, _ldap.RES_SEARCH_RESULT) self.assertEqual(msgid, m) @@ -595,7 +629,7 @@ def test_whoami_after_unbind(self): def test_passwd(self): l = self._open_conn() # first, create a user to change password on - dn = "cn=PasswordTest," + self.server.suffix + dn = "cn=PasswordTest," + self.writesuffix m = l.add_ext( dn, [ diff --git a/Tests/t_edit.py b/Tests/t_edit.py index 9aee43e..df4529a 100644 --- a/Tests/t_edit.py +++ b/Tests/t_edit.py @@ -83,6 +83,12 @@ def test_add_object(self): ("cn=Added,ou=Container," + base, {'cn': [b'Added'], 'objectClass': [b'organizationalRole']}), ]) + # Delete object + self.ldap.delete_s(dn) + result = self.ldap.search_s( + base, ldap.SCOPE_SUBTREE, '(cn=Added)', ['*'] + ) + self.assertEqual(result, []) if __name__ == '__main__':