On 02/25/2013 05:39 PM, Jan Cholasta wrote:
On 20.2.2013 13:03, Petr Viktorin wrote:
On 02/19/2013 03:10 PM, Jan Cholasta wrote:
On 1.2.2013 15:38, Petr Viktorin wrote:
Alright, I renamed get_single to single_value().
I also rebased to current master.
Patch 152:
+ def single_value(self, name, default=_missing):
+ values = self.get(name, [default])
+ if len(values) != 1:
+ raise ValueError(
+ '%s has %s values, one expected' % (name, len(values)))
+ if values[0] is _missing:
+ raise KeyError(name)
+ return values[0]
I would prefer if you used __getitem__() instead of get() and re-raised
KeyError if default is _missing. Also, until we disallow non-lists, we
need to check if values actually is list. I.e., do something like this:
+ def single_value(self, name, default=_missing):
+ try:
+ values = super(LDAPEntry,
self).__getitem__(self._get_attr_name(name))
+ except KeyError:
+ if default is _missing:
+ raise
+ return default
+ if not isinstance(values, list):
+ return values
+ if len(values) != 1:
+ raise ValueError(
+ '%s has %s values, one expected' % (name, len(values)))
+ return values[0]
Updated, thanks.
_get_attr_name is only added in your patch 99, so I used _attr_name here
and modified your patch.
I wrote some tests for single_value, and while I was at it I added tests
for a few other LDAPEntry methods as well. I've also split things up
into more testcases. Including as patch 0181.
Thanks. I think you should also add a tearDown method to test_LDAPEntry
which disconnects self.conn if it is connected (the same thing test_ldap
does).
Thanks for the catch, added.
Patch 159:
Like I said in my review of your patch 143, I think we should use DNs
instead of entries in delete_entry, so I think it would make sense to do
delete_entry(entry.dn) in this patch.
I think that for symmetry with add_entry and update_entry, delete_entry
should take entries too. If you already have the entry, having to type
the extra ".dn" is not very intuitive.
The proper thing to do would be a separate delete_by_dn(), but
delete_entry() taking either entries or DNs works fine IMO.
OK, makes sense.
Patch 160:
I think you should do this (replace % with ,):
+ root_logger.debug(
+ "failed to find mapping tree entry for %s",
self.suffix)
Fixed, thanks.
I've also rebased 174 and your patch 104 to current master.
Honza
--
Petr³
From bab85230a06aa780905c91614de8ca904d0c9a4c Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Wed, 20 Feb 2013 04:49:03 -0500
Subject: [PATCH] Improve LDAPEntry tests
---
tests/test_ipaserver/test_ldap.py | 110 ++++++++++++++++++++++++++++++-------
1 files changed, 89 insertions(+), 21 deletions(-)
diff --git a/tests/test_ipaserver/test_ldap.py b/tests/test_ipaserver/test_ldap.py
index a35f7830df63cfa7699b565126283f5886407058..6ac0c2243ee90accb59929241820d2c8c878a134 100644
--- a/tests/test_ipaserver/test_ldap.py
+++ b/tests/test_ipaserver/test_ldap.py
@@ -25,13 +25,15 @@
# The DM password needs to be set in ~/.ipa/.dmpw
-import nose
import os
+
+import nose
+from nose.tools import assert_raises # pylint: disable=E0611
+import nss.nss as nss
+
from ipaserver.plugins.ldap2 import ldap2
-from ipaserver.ipaldap import LDAPEntry
from ipalib.plugins.service import service, service_show
from ipalib.plugins.host import host
-import nss.nss as nss
from ipalib import api, x509, create_api, errors
from ipapython import ipautil
from ipapython.dn import DN
@@ -147,46 +149,112 @@ class test_ldap(object):
serial = unicode(x509.get_serial_number(cert, x509.DER))
assert serial is not None
+
+class test_LDAPEntry(object):
+ """
+ Test the LDAPEntry class
+ """
+ cn1 = [u'test1']
+ cn2 = [u'test2']
+ dn1 = DN(('cn', cn1[0]))
+ dn2 = DN(('cn', cn2[0]))
+
+ def setUp(self):
+ self.ldapuri = 'ldap://%s' % ipautil.format_netloc(api.env.host)
+ self.conn = ldap2(shared_instance=False, ldap_uri=self.ldapuri)
+ self.conn.connect()
+
+ self.entry = self.conn.make_entry(self.dn1, cn=self.cn1)
+
+ def tearDown(self):
+ if self.conn and self.conn.isconnected():
+ self.conn.disconnect()
+
def test_entry(self):
- """
- Test the LDAPEntry class
- """
- cn1 = [u'test1']
- cn2 = [u'test2']
- dn1 = DN(('cn', cn1[0]))
- dn2 = DN(('cn', cn2[0]))
-
- self.conn = ldap2(shared_instance=False, ldap_uri=self.ldapuri)
- self.conn.connect()
-
- e = LDAPEntry(self.conn.conn, dn1, cn=cn1)
- assert e.dn is dn1
+ e = self.entry
+ assert e.dn is self.dn1
assert u'cn' in e
assert u'cn' in e.keys()
assert 'CN' in e
assert 'CN' not in e.keys()
assert 'commonName' in e
assert 'commonName' not in e.keys()
- assert e['CN'] is cn1
+ assert e['CN'] is self.cn1
assert e['CN'] is e[u'cn']
- e.dn = dn2
- assert e.dn is dn2
+ e.dn = self.dn2
+ assert e.dn is self.dn2
- e['commonName'] = cn2
+ def test_set_attr(self):
+ e = self.entry
+ e['commonName'] = self.cn2
assert u'cn' in e
assert u'cn' not in e.keys()
assert 'CN' in e
assert 'CN' not in e.keys()
assert 'commonName' in e
assert 'commonName' in e.keys()
- assert e['CN'] is cn2
+ assert e['CN'] is self.cn2
assert e['CN'] is e[u'cn']
+ def test_del_attr(self):
+ e = self.entry
del e['CN']
assert 'CN' not in e
assert 'CN' not in e.keys()
assert u'cn' not in e
assert u'cn' not in e.keys()
assert 'commonName' not in e
assert 'commonName' not in e.keys()
+
+ def test_popitem(self):
+ e = self.entry
+ assert e.popitem() == ('cn', self.cn1)
+ e.keys() == []
+
+ def test_setdefault(self):
+ e = self.entry
+ assert e.setdefault('cn', self.cn2) == self.cn1
+ assert e['cn'] == self.cn1
+ assert e.setdefault('xyz', self.cn2) == self.cn2
+ assert e['xyz'] == self.cn2
+
+ def test_update(self):
+ e = self.entry
+ e.update({'cn': self.cn2}, xyz=self.cn2)
+ assert e['cn'] == self.cn2
+ assert e['xyz'] == self.cn2
+
+ def test_pop(self):
+ e = self.entry
+ assert e.pop('cn') == self.cn1
+ assert 'cn' not in e
+ assert e.pop('cn', 'default') is 'default'
+ with assert_raises(KeyError):
+ e.pop('cn')
+
+ def test_clear(self):
+ e = self.entry
+ e.clear()
+ assert not e
+ assert 'cn' not in e
+
+ def test_has_key(self):
+ e = self.entry
+ assert not e.has_key('xyz')
+ assert e.has_key('cn')
+ assert e.has_key('COMMONNAME')
+
+ def test_get(self):
+ e = self.entry
+ assert e.get('cn') == self.cn1
+ assert e.get('commonname') == self.cn1
+ assert e.get('COMMONNAME', 'default') == self.cn1
+ assert e.get('bad key', 'default') == 'default'
+
+ def test_single_value(self):
+ e = self.entry
+ assert e.single_value('cn') == self.cn1[0]
+ assert e.single_value('commonname') == self.cn1[0]
+ assert e.single_value('COMMONNAME', 'default') == self.cn1[0]
+ assert e.single_value('bad key', 'default') == 'default'
--
1.7.7.6
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
