Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Jakub Hrozek]

On Wed, Nov 11, 2015 at 06:05:02PM +0100, Michal Židek wrote:
> On 11/11/2015 06:03 PM, Michal Židek wrote:
> >On 11/09/2015 09:19 AM, Lukas Slebodnik wrote:
> >>On (08/11/15 23:49), Michal Židek wrote:
> >>>I am attaching patch that can be applied and squashed into
> >>>Nick's patch. I am sending both patches to see what
> >>>changes have been made, *please squash them before pushing*.
> >>>
> >>>This patch just removes the negchace (together with the
> >>>asserts that are testing it) and memcache,
> >>>and adds global constant to avoid using magic values.
> >>>
> >>Patches for bug with memory cache are already on the list.
> >>So it would be good to remove workarounds.
> >>Could you prepare new patches on top of mc patches?
> >>
> >>LS
> >>___
> >>sssd-devel mailing list
> >>sssd-devel@lists.fedorahosted.org
> >>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >>
> >
> >I modified the "squashing" patch to remove the
> >workaround for failing memcache.
> >
> >SQUASH THESE 2 ATTACHED PATCHES BEFORE PUSHING
> >and use the commit message from Nick's
> >(first) patch (I altered the message a little
> >so that the removed test are not mentioned).
> >
> >CI link passed:
> >http://sssd-ci.duckdns.org/logs/job/32/83/summary.html
> >
> >Michal
> >
> 
> I forgot to say that the memcache fix must be
> pushed before this.
> 
> Michal

CI passed some time ago:
http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/3304/

and we agreed off-list that the fixup is a good
thing to do.

I'm going to push the patches..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Jakub Hrozek]

On Sat, Nov 14, 2015 at 01:43:45PM +0100, Jakub Hrozek wrote:
> On Wed, Nov 11, 2015 at 06:05:02PM +0100, Michal Židek wrote:
> > On 11/11/2015 06:03 PM, Michal Židek wrote:
> > >On 11/09/2015 09:19 AM, Lukas Slebodnik wrote:
> > >>On (08/11/15 23:49), Michal Židek wrote:
> > >>>I am attaching patch that can be applied and squashed into
> > >>>Nick's patch. I am sending both patches to see what
> > >>>changes have been made, *please squash them before pushing*.
> > >>>
> > >>>This patch just removes the negchace (together with the
> > >>>asserts that are testing it) and memcache,
> > >>>and adds global constant to avoid using magic values.
> > >>>
> > >>Patches for bug with memory cache are already on the list.
> > >>So it would be good to remove workarounds.
> > >>Could you prepare new patches on top of mc patches?
> > >>
> > >>LS
> > >>___
> > >>sssd-devel mailing list
> > >>sssd-devel@lists.fedorahosted.org
> > >>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >>
> > >
> > >I modified the "squashing" patch to remove the
> > >workaround for failing memcache.
> > >
> > >SQUASH THESE 2 ATTACHED PATCHES BEFORE PUSHING
> > >and use the commit message from Nick's
> > >(first) patch (I altered the message a little
> > >so that the removed test are not mentioned).
> > >
> > >CI link passed:
> > >http://sssd-ci.duckdns.org/logs/job/32/83/summary.html
> > >
> > >Michal
> > >
> > 
> > I forgot to say that the memcache fix must be
> > pushed before this.
> > 
> > Michal
> 
> CI passed some time ago:
> http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/3304/
> 
> and we agreed off-list that the fixup is a good
> thing to do.
> 
> I'm going to push the patches..

Squashed into one and pushed to master with Michal's RB:
c20811708e584b49ef12ffe1950d71356604bd3b
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Michal Židek]

On 11/09/2015 09:19 AM, Lukas Slebodnik wrote:

On (08/11/15 23:49), Michal Židek wrote:

I am attaching patch that can be applied and squashed into
Nick's patch. I am sending both patches to see what
changes have been made, *please squash them before pushing*.

This patch just removes the negchace (together with the
asserts that are testing it) and memcache,
and adds global constant to avoid using magic values.


Patches for bug with memory cache are already on the list.
So it would be good to remove workarounds.
Could you prepare new patches on top of mc patches?

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



I modified the "squashing" patch to remove the
workaround for failing memcache.

SQUASH THESE 2 ATTACHED PATCHES BEFORE PUSHING
and use the commit message from Nick's
(first) patch (I altered the message a little
so that the removed test are not mentioned).

CI link passed:
http://sssd-ci.duckdns.org/logs/job/32/83/summary.html

Michal
>From 904a94d406eaaa2a0f8389951b007552bfc04e1d Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov 
Date: Tue, 29 Sep 2015 21:18:18 +0300
Subject: [PATCH 5/6] intg: Add more LDAP tests

Add a bunch of LDAP tests.

* Adding/removing a user/group/membership with rfc2307(bis) schema.
* The effect of override_homedir option.
* The effect of fallback_homedir option.
* The effect of override_shell option.
* The effect of shell_fallback option.
* The effect of default_shell option.
* The effect of vetoed_shells option.
---
 src/tests/intg/ldap_test.py | 539 +++-
 1 file changed, 537 insertions(+), 2 deletions(-)

diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
index 9f1b7e0..9403b63 100644
--- a/src/tests/intg/ldap_test.py
+++ b/src/tests/intg/ldap_test.py
@@ -127,6 +127,22 @@ def format_basic_conf(ldap_conn, schema, enum):
 """).format(**locals())
 
 
+def format_interactive_conf(ldap_conn, schema):
+"""Format an SSSD configuration with all caches refreshing in 4 seconds"""
+return \
+format_basic_conf(ldap_conn, schema, enum=True) + \
+unindent("""
+[nss]
+enum_cache_timeout  = 4
+entry_negative_timeout  = 4
+
+[domain/LDAP]
+ldap_enumeration_refresh_timeout= 4
+ldap_purge_cache_timeout= 1
+entry_cache_timeout = 4
+""")
+
+
 def create_conf_file(contents):
 """Create sssd.conf with specified contents"""
 conf = open(config.CONF_PATH, "w")
@@ -176,8 +192,10 @@ def cleanup_sssd_process():
 pass
 for path in os.listdir(config.DB_PATH):
 os.unlink(config.DB_PATH + "/" + path)
-for path in os.listdir(config.MCACHE_PATH):
-os.unlink(config.MCACHE_PATH + "/" + path)
+# FIXME: Uncomment when removed cache invalidation is fixed:
+# https://fedorahosted.org/sssd/ticket/2726
+# for path in os.listdir(config.MCACHE_PATH):
+#os.unlink(config.MCACHE_PATH + "/" + path)
 
 
 def create_sssd_cleanup(request):
@@ -388,3 +406,520 @@ def test_refresh_after_cleanup_task(ldap_conn, refresh_after_cleanup_task):
 ent.assert_group_by_name(
 "group2",
 dict(mem=ent.contains_only("user1")))
+
+
+@pytest.fixture
+def blank_rfc2307(request, ldap_conn):
+"""Create blank RFC2307 directory fixture with interactive SSSD conf"""
+create_ldap_cleanup(request, ldap_conn)
+create_conf_fixture(request,
+format_interactive_conf(ldap_conn, SCHEMA_RFC2307))
+create_sssd_fixture(request)
+
+
+@pytest.fixture
+def blank_rfc2307_bis(request, ldap_conn):
+"""Create blank RFC2307bis directory fixture with interactive SSSD conf"""
+create_ldap_cleanup(request, ldap_conn)
+create_conf_fixture(request,
+format_interactive_conf(ldap_conn, SCHEMA_RFC2307_BIS))
+create_sssd_fixture(request)
+
+
+@pytest.fixture
+def user_and_group_rfc2307(request, ldap_conn):
+"""
+Create an RFC2307 directory fixture with interactive SSSD conf,
+one user and one group
+"""
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+ent_list.add_user("user", 1001, 2000)
+ent_list.add_group("group", 2001)
+create_ldap_fixture(request, ldap_conn, ent_list)
+create_conf_fixture(request,
+format_interactive_conf(ldap_conn, SCHEMA_RFC2307))
+create_sssd_fixture(request)
+return None
+
+
+@pytest.fixture
+def user_and_groups_rfc2307_bis(request, ldap_conn):
+"""
+Create an RFC2307bis directory fixture with interactive SSSD conf,
+one user and two groups
+"""
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+ent_list.add_user("user", 1001, 2000)
+ent_list.add_group_bis("group1", 

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Michal Židek]

On 11/11/2015 06:03 PM, Michal Židek wrote:

On 11/09/2015 09:19 AM, Lukas Slebodnik wrote:

On (08/11/15 23:49), Michal Židek wrote:

I am attaching patch that can be applied and squashed into
Nick's patch. I am sending both patches to see what
changes have been made, *please squash them before pushing*.

This patch just removes the negchace (together with the
asserts that are testing it) and memcache,
and adds global constant to avoid using magic values.


Patches for bug with memory cache are already on the list.
So it would be good to remove workarounds.
Could you prepare new patches on top of mc patches?

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



I modified the "squashing" patch to remove the
workaround for failing memcache.

SQUASH THESE 2 ATTACHED PATCHES BEFORE PUSHING
and use the commit message from Nick's
(first) patch (I altered the message a little
so that the removed test are not mentioned).

CI link passed:
http://sssd-ci.duckdns.org/logs/job/32/83/summary.html

Michal



I forgot to say that the memcache fix must be
pushed before this.

Michal

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Lukas Slebodnik]

On (08/11/15 23:49), Michal Židek wrote:
>I am attaching patch that can be applied and squashed into
>Nick's patch. I am sending both patches to see what
>changes have been made, *please squash them before pushing*.
>
>This patch just removes the negchace (together with the
>asserts that are testing it) and memcache,
>and adds global constant to avoid using magic values.
>
Patches for bug with memory cache are already on the list.
So it would be good to remove workarounds.
Could you prepare new patches on top of mc patches?

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Michal Židek]

I am attaching patch that can be applied and squashed into
Nick's patch. I am sending both patches to see what
changes have been made, *please squash them before pushing*.

This patch just removes the negchace (together with the
asserts that are testing it) and memcache,
and adds global constant to avoid using magic values.

It is an ACK from me.

CI:
http://sssd-ci.duckdns.org/logs/job/32/19/summary.html

I removed some tests from Nick's patch that need more
refactoring. I will send them as separate patch.

On 11/05/2015 03:27 PM, Nikolai Kondrashov wrote:

On 11/04/2015 05:01 PM, Nikolai Kondrashov wrote:

On 11/04/2015 04:25 PM, Michal Židek wrote:


That is good! If the patches do not work for you without the sleep()s
you can send them with the sleep()s for now and I will take a look.


Just for the record, it did not work for me without the
sleeps either. It probably worked before, because I had some
unsaved changes in code. So Nick was right and the sleeps
are necessary because of the enum_cache_timeout.



Please find attached patches with what I have working right now. The
remaining
work includes:

* Put the 4s timeout into a global variable and use it instead of all the
   literal timeouts throughout the code. Instead of 2s timeouts use that
   variable divided by 2 - that's its meaning.

* Attempt refactoring configuration generation inside loops within tests
into
   separate tests with separate fixtures. That concerns the following
   functions:

 test_filter_users
 test_filter_groups_rfc2307
 test_filter_groups_rfc2307_bis

   Michal's comment was:

 Instead of generating the conf files in nested for loops, it would be
 better to split the test to multiple smaller tests where each has
 different config file fixture (instad of void_conf and void_sssd).
 This may seem like code duplication, but I believe it will result in
 better readable code.

   My response was:

 I would really like to do that, but the problem is we will either
need to
 copy-paste a ton of tests and fixtures, or use py.test's convoluted
means
 of sharing parameters between fixtures and tests which will produce
much
 more complicated code that this. At least this is my impression after
 researching that.

   We can try doing that and seeing how it works.

* Consider avoiding any assertions hitting negative cache, and disabling
   negative cache to possibly increase reliability - Michal's suggestion.
   Michal, please correct me if I cite it wrong or misunderstood it.

* Check the patch with pep8 and resolve any found issues.

* After the tests are merged, rebase the "intg: Add a memcache invalidation
   failure test" patch on top of it and attach it to ticket #2726 as the
   possible regression tracker.

Nick


>From 8d062c4e509fd7c3bd25daf4af9426ebcc426a4b Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov 
Date: Tue, 29 Sep 2015 21:18:18 +0300
Subject: [PATCH 1/2] intg: Add more LDAP tests

Add a bunch of LDAP tests.

* Adding/removing a user/group/membership with rfc2307(bis) schema.
* Filtering users/groups with rfc2307(bis) schema.
* The effect of override_homedir option.
* The effect of fallback_homedir option.
* The effect of override_shell option.
* The effect of shell_fallback option.
* The effect of default_shell option.
* The effect of vetoed_shells option.
---
 src/tests/intg/ldap_test.py | 539 +++-
 1 file changed, 537 insertions(+), 2 deletions(-)

diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
index 6d61726..40f9009 100644
--- a/src/tests/intg/ldap_test.py
+++ b/src/tests/intg/ldap_test.py
@@ -127,6 +127,22 @@ def format_basic_conf(ldap_conn, schema, enum):
 """).format(**locals())
 
 
+def format_interactive_conf(ldap_conn, schema):
+"""Format an SSSD configuration with all caches refreshing in 4 seconds"""
+return \
+format_basic_conf(ldap_conn, schema, enum=True) + \
+unindent("""
+[nss]
+enum_cache_timeout  = 4
+entry_negative_timeout  = 4
+
+[domain/LDAP]
+ldap_enumeration_refresh_timeout= 4
+ldap_purge_cache_timeout= 1
+entry_cache_timeout = 4
+""")
+
+
 def create_conf_file(contents):
 """Create sssd.conf with specified contents"""
 conf = open(config.CONF_PATH, "w")
@@ -177,8 +193,10 @@ def cleanup_sssd_process():
 subprocess.call(["sss_cache", "-E"])
 for path in os.listdir(config.DB_PATH):
 os.unlink(config.DB_PATH + "/" + path)
-for path in os.listdir(config.MCACHE_PATH):
-os.unlink(config.MCACHE_PATH + "/" + path)
+# FIXME: Uncomment when removed cache invalidation is fixed:
+# https://fedorahosted.org/sssd/ticket/2726
+# for path in os.listdir(config.MCACHE_PATH):
+#

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Nikolai Kondrashov]

On 11/04/2015 05:01 PM, Nikolai Kondrashov wrote:

On 11/04/2015 04:25 PM, Michal Židek wrote:

On 11/03/2015 06:30 PM, Nikolai Kondrashov wrote:

Now, I think I got confused here. I'm not actually testing negative
cache, but
only the fact that the changes to LDAP don't appear before the caches
expire.
Since the negative cache timeout equals the timeout of positive caches,
there
are really no negative cache-specific tests.

That said, I can still remove the tests you mention in your patch, if you
think we should remove them. However, just for the record, I didn't see any
issues with them.

Another thing is, I couldn't make the tests work with those sleep(4)'s
removed
as you did in your patch up-thread. In fact all *add_remove* tests failed
except the add_remove_user_memcache_mix (delay results are not checked
there).

The problem being, the assertions following these delays check
enumeration and
enumeration timeout is still 4 seconds, so we need to wait for it. For that
matter removing tests you mention doesn't have an impact on execution time.


I see where the confusion comes from now. But the enumeration should not
play a role here. You can think about enumeration as forced sysdb fill
operation, but it is not necessary in order to grab user from LDAP.
If the user is not found in the sysdb (and also not found in the
negcache) SSSD grabs the user from LDAP even before the next
enumeration.


Sure. However, ent.assert_passwd does pwd.getpwall, not only pwd.getpwnam and
pwd.getpwuid. Pwd.getpwall result check is the one that fails.
Ent.assert_group behaves similarly.

I think I remember Jakub asking to have this test without enumeration as well,
and I can do that, but I think enumeration test is also valuable.


I'm not quite sure how you made it work. Was some other change omitted from
your patch accidentally? I'm talking about changes like this:


I do not think I omitted something. Did you really set
the negative_cache to 0 in sssd.conf?


Yes. I tried the exact patch you attached.



  def create_sssd_cleanup(request):
@@ -462,12 +464,9 @@ def user_and_groups_rfc2307_bis(request, ldap_conn):
  def test_add_remove_user(ldap_conn, blank_rfc2307):
  """Test user addition and removal are reflected by SSSD"""
  e = ldap_ent.user(ldap_conn.ds_inst.base_dn, "user", 1001, 2000)
-time.sleep(2)
  # Add the user
  ent.assert_passwd(ent.contains_only())

 ^^
Here you fill the negative cache.


  ldap_conn.add_s(*e)
-ent.assert_passwd(ent.contains_only())

^^
Here you are testing the negative cache. If there was no negcache
hit, SSSD would grab the user from LDAP.


Ah, yes, it seems this should fetch from the negative cache.


-time.sleep(4)

^
So this sleep should not be necessary if the negcache is disabled.


  ent.assert_passwd(ent.contains_only(dict(name="user", uid=1001)))
  # Remove the user
  ldap_conn.delete_s(e[0])

All in all, setting memcache_timeout to zero and commenting-out memcache
file
removal was enough to make *all* tests work fine.



That is good! If the patches do not work for you without the sleep()s
you can send them with the sleep()s for now and I will take a look.


Please find attached patches with what I have working right now. The remaining
work includes:

* Put the 4s timeout into a global variable and use it instead of all the
  literal timeouts throughout the code. Instead of 2s timeouts use that
  variable divided by 2 - that's its meaning.

* Attempt refactoring configuration generation inside loops within tests into
  separate tests with separate fixtures. That concerns the following
  functions:

test_filter_users
test_filter_groups_rfc2307
test_filter_groups_rfc2307_bis

  Michal's comment was:

Instead of generating the conf files in nested for loops, it would be
better to split the test to multiple smaller tests where each has
different config file fixture (instad of void_conf and void_sssd).
This may seem like code duplication, but I believe it will result in
better readable code.

  My response was:

I would really like to do that, but the problem is we will either need to
copy-paste a ton of tests and fixtures, or use py.test's convoluted means
of sharing parameters between fixtures and tests which will produce much
more complicated code that this. At least this is my impression after
researching that.

  We can try doing that and seeing how it works.

* Consider avoiding any assertions hitting negative cache, and disabling
  negative cache to possibly increase reliability - Michal's suggestion.
  Michal, please correct me if I cite it wrong or misunderstood it.

* Check the patch with pep8 and resolve any found issues.

* After the tests are merged, rebase the "intg: Add a memcache invalidation
  failure test" patch on top of it and attach it to ticket #2726 as the
  

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Nikolai Kondrashov]

On 11/04/2015 04:25 PM, Michal Židek wrote:

On 11/03/2015 06:30 PM, Nikolai Kondrashov wrote:

Now, I think I got confused here. I'm not actually testing negative
cache, but
only the fact that the changes to LDAP don't appear before the caches
expire.
Since the negative cache timeout equals the timeout of positive caches,
there
are really no negative cache-specific tests.

That said, I can still remove the tests you mention in your patch, if you
think we should remove them. However, just for the record, I didn't see any
issues with them.

Another thing is, I couldn't make the tests work with those sleep(4)'s
removed
as you did in your patch up-thread. In fact all *add_remove* tests failed
except the add_remove_user_memcache_mix (delay results are not checked
there).

The problem being, the assertions following these delays check
enumeration and
enumeration timeout is still 4 seconds, so we need to wait for it. For that
matter removing tests you mention doesn't have an impact on execution time.


I see where the confusion comes from now. But the enumeration should not
play a role here. You can think about enumeration as forced sysdb fill
operation, but it is not necessary in order to grab user from LDAP.
If the user is not found in the sysdb (and also not found in the
negcache) SSSD grabs the user from LDAP even before the next
enumeration.


Sure. However, ent.assert_passwd does pwd.getpwall, not only pwd.getpwnam and
pwd.getpwuid. Pwd.getpwall result check is the one that fails.
Ent.assert_group behaves similarly.

I think I remember Jakub asking to have this test without enumeration as well,
and I can do that, but I think enumeration test is also valuable.


I'm not quite sure how you made it work. Was some other change omitted from
your patch accidentally? I'm talking about changes like this:


I do not think I omitted something. Did you really set
the negative_cache to 0 in sssd.conf?


Yes. I tried the exact patch you attached.



  def create_sssd_cleanup(request):
@@ -462,12 +464,9 @@ def user_and_groups_rfc2307_bis(request, ldap_conn):
  def test_add_remove_user(ldap_conn, blank_rfc2307):
  """Test user addition and removal are reflected by SSSD"""
  e = ldap_ent.user(ldap_conn.ds_inst.base_dn, "user", 1001, 2000)
-time.sleep(2)
  # Add the user
  ent.assert_passwd(ent.contains_only())

 ^^
Here you fill the negative cache.


  ldap_conn.add_s(*e)
-ent.assert_passwd(ent.contains_only())

^^
Here you are testing the negative cache. If there was no negcache
hit, SSSD would grab the user from LDAP.


Ah, yes, it seems this should fetch from the negative cache.


-time.sleep(4)

^
So this sleep should not be necessary if the negcache is disabled.


  ent.assert_passwd(ent.contains_only(dict(name="user", uid=1001)))
  # Remove the user
  ldap_conn.delete_s(e[0])

All in all, setting memcache_timeout to zero and commenting-out memcache
file
removal was enough to make *all* tests work fine.



That is good! If the patches do not work for you without the sleep()s
you can send them with the sleep()s for now and I will take a look.


Alright.

Nick
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Michal Židek]

On 11/03/2015 06:30 PM, Nikolai Kondrashov wrote:

On 10/29/2015 04:52 PM, Michal Židek wrote:

On 10/27/2015 04:10 PM, Nikolai Kondrashov wrote:

On 10/23/2015 02:54 PM, Michal Židek wrote:

+def format_interactive_conf(ldap_conn, schema):
+"""Format an SSSD configuration with all caches refreshing in 4
seconds"""
+return \
+format_basic_conf(ldap_conn, schema, enum=True) + \
+unindent("""
+[nss]
+memcache_timeout= 4


It is better to set memcache timeout to zero outside tests
that are not dedicated to memcache. This will also probably
solve the membership tests failure that you saw when using the
workaround for the memcache tests failure.


Hmm, perhaps. However, could you please explain why the membership tests
fail?

I noticed that they also fail if I simply don't run the group
addition/removal
tests before them, in addition to enabling the workaround. Also, they
work if
I put "run_shell" before each "assert" and immediately exit the spawned
shells
even without doing anything in them.


I really do not know why the run_shell helped. Maybe it
forced pytest to initialize new client memory cache context.
I tried to do some tests with run_shell, but they did not
work for me.

Anyway, the failures were connected to memory cache
and memory cache currently is not reliable in these
test, so I did not investigated further, why they
failed.


Alright.




+enum_cache_timeout  = 4
+entry_negative_timeout  = 4


I would set negative cache timeout to zero as well,
see comments below.


Replying below.


+def test_add_remove_user(ldap_conn, blank_rfc2307):
+"""Test user addition and removal are reflected by SSSD"""
+e = ldap_ent.user(ldap_conn.ds_inst.base_dn, "user", 1001, 2000)
+time.sleep(2)


What is the purpose of this timeout? It does not seem to be
necessary. You use it in all the other tests as well, so
I guess it had some meaning (but I tried to remove them and
tests passed for me without problems).


This puts test actions and tests in the middle of the 4 second cache
timeouts,
so they are more reliable and time drift doesn't affect them that much.

E.g.:

 0 - sssd start
 1 -
 2 - add user, check it's not yet present
 3 -
 4 - cache expiry/purging
 5 -
 6 - check user added
 7 -
 8 - cache expiry/purging

etc.

IIRC, I got 400+ perfect runs before the first failure occurred with
this -
better than other schemes.


Hmmm...maybe this was because SSSD was not initialized (fully
started) when the test was run. In which case it is problem
of the fixture that starts SSSD and should be solved inside
the fixture (the sleep() should be added there).


IIRC it wasn't always failing on the first test. That rules out the
initialization issue. Besides, IIRC, about two years ago we worked on
ensuring
SSSD is fully initialized and ready to serve request when service start
completes. Red Hat's downstream tests rely on that.

However, the initial delay might be better moved to the fixture. Will
try to
do that and see how it fits.


+# Add the user
+ent.assert_passwd(ent.contains_only())
+ldap_conn.add_s(*e)
+ent.assert_passwd(ent.contains_only())


I would avoid testing the negative cache outside tests
that are dedicated to negative cache. We had this same
pattern in our C code tests and it timed out when CI
was under heavy load. We can add dedicated tests
for negative cache in CI later with big enough timeout
to pass even under heavy load CI.

So please, remove the negative cache testing from everywhere
in these tests.


I'm OK with minimizing the tests and not exercising some caching
mechanisms. I understand it will make tests more reliable.

However, my intention was to test the full end-user functionality, what
actually matters to users. Users don't really care about caches, they
just
want everything work fast and reliably. They just want their LDAP
changes
propagated. For that matter I only touch cache timeouts to make tests
run in
reasonable time. I don't disable them, because users will probably have
them
enabled as well.

We can test all the cache mechanisms separately, but we will still
have to
test them working together. Can we do that? Can we have these particular
tests
do that? Or is it too hard/impossible?

If we can do that, how would you like to see them?

If not, I'll just disable memory cache and remove negative cache
testing as
you request.

Or do I misunderstand the idea behind this?


I understand your point, but negative cache IMO just slows
down the tests. If we disable it, we still actually use it
under the hood, it just timeouts immediately (so we trigger
the same codepath, that most users will trigger in their
environment). Users will usually not add and delete users
in such short time, so using negative cache does not get
us closer to real cases and it really is just test scenario.

Main reason why I would like to 

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Nikolai Kondrashov]

On 11/03/2015 07:30 PM, Nikolai Kondrashov wrote:

All in all, setting memcache_timeout to zero and commenting-out memcache file
removal was enough to make *all* tests work fine.


FWIW, I left it running repeatedly overnight and it got 250 successful runs
and is still going.

Nick
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Nikolai Kondrashov]

On 10/29/2015 04:52 PM, Michal Židek wrote:

On 10/27/2015 04:10 PM, Nikolai Kondrashov wrote:

On 10/23/2015 02:54 PM, Michal Židek wrote:

+def format_interactive_conf(ldap_conn, schema):
+"""Format an SSSD configuration with all caches refreshing in 4
seconds"""
+return \
+format_basic_conf(ldap_conn, schema, enum=True) + \
+unindent("""
+[nss]
+memcache_timeout= 4


It is better to set memcache timeout to zero outside tests
that are not dedicated to memcache. This will also probably
solve the membership tests failure that you saw when using the
workaround for the memcache tests failure.


Hmm, perhaps. However, could you please explain why the membership tests
fail?

I noticed that they also fail if I simply don't run the group
addition/removal
tests before them, in addition to enabling the workaround. Also, they
work if
I put "run_shell" before each "assert" and immediately exit the spawned
shells
even without doing anything in them.


I really do not know why the run_shell helped. Maybe it
forced pytest to initialize new client memory cache context.
I tried to do some tests with run_shell, but they did not
work for me.

Anyway, the failures were connected to memory cache
and memory cache currently is not reliable in these
test, so I did not investigated further, why they
failed.


Alright.




+enum_cache_timeout  = 4
+entry_negative_timeout  = 4


I would set negative cache timeout to zero as well,
see comments below.


Replying below.


+def test_add_remove_user(ldap_conn, blank_rfc2307):
+"""Test user addition and removal are reflected by SSSD"""
+e = ldap_ent.user(ldap_conn.ds_inst.base_dn, "user", 1001, 2000)
+time.sleep(2)


What is the purpose of this timeout? It does not seem to be
necessary. You use it in all the other tests as well, so
I guess it had some meaning (but I tried to remove them and
tests passed for me without problems).


This puts test actions and tests in the middle of the 4 second cache
timeouts,
so they are more reliable and time drift doesn't affect them that much.

E.g.:

 0 - sssd start
 1 -
 2 - add user, check it's not yet present
 3 -
 4 - cache expiry/purging
 5 -
 6 - check user added
 7 -
 8 - cache expiry/purging

etc.

IIRC, I got 400+ perfect runs before the first failure occurred with this -
better than other schemes.


Hmmm...maybe this was because SSSD was not initialized (fully
started) when the test was run. In which case it is problem
of the fixture that starts SSSD and should be solved inside
the fixture (the sleep() should be added there).


IIRC it wasn't always failing on the first test. That rules out the
initialization issue. Besides, IIRC, about two years ago we worked on ensuring
SSSD is fully initialized and ready to serve request when service start
completes. Red Hat's downstream tests rely on that.

However, the initial delay might be better moved to the fixture. Will try to
do that and see how it fits.


+# Add the user
+ent.assert_passwd(ent.contains_only())
+ldap_conn.add_s(*e)
+ent.assert_passwd(ent.contains_only())


I would avoid testing the negative cache outside tests
that are dedicated to negative cache. We had this same
pattern in our C code tests and it timed out when CI
was under heavy load. We can add dedicated tests
for negative cache in CI later with big enough timeout
to pass even under heavy load CI.

So please, remove the negative cache testing from everywhere
in these tests.


I'm OK with minimizing the tests and not exercising some caching
mechanisms. I understand it will make tests more reliable.

However, my intention was to test the full end-user functionality, what
actually matters to users. Users don't really care about caches, they just
want everything work fast and reliably. They just want their LDAP changes
propagated. For that matter I only touch cache timeouts to make tests
run in
reasonable time. I don't disable them, because users will probably have
them
enabled as well.

We can test all the cache mechanisms separately, but we will still have to
test them working together. Can we do that? Can we have these particular
tests
do that? Or is it too hard/impossible?

If we can do that, how would you like to see them?

If not, I'll just disable memory cache and remove negative cache testing as
you request.

Or do I misunderstand the idea behind this?


I understand your point, but negative cache IMO just slows
down the tests. If we disable it, we still actually use it
under the hood, it just timeouts immediately (so we trigger
the same codepath, that most users will trigger in their
environment). Users will usually not add and delete users
in such short time, so using negative cache does not get
us closer to real cases and it really is just test scenario.

Main reason why I would like to have separate tests for
negative cache (which I do 

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Michal Židek]

On 10/27/2015 04:10 PM, Nikolai Kondrashov wrote:

Hi Michal,

Thanks a lot for the detailed review and testing!
Please see my comments below.

On 10/23/2015 02:54 PM, Michal Židek wrote:

Hi!

There is one new pep8 error in the code:
../src/tests/intg/ldap_test.py:819:37: E126 continuation line
over-indented for hanging indent


Sure, will fix it.


For the memcache workaroud please do this change in the
code:
diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
index 8263d88..eb466ab 100644
--- a/src/tests/intg/ldap_test.py
+++ b/src/tests/intg/ldap_test.py
@@ -194,8 +194,10 @@ def cleanup_sssd_process():
  subprocess.call(["sss_cache", "-E"])
  for path in os.listdir(config.DB_PATH):
  os.unlink(config.DB_PATH + "/" + path)
-for path in os.listdir(config.MCACHE_PATH):
-os.unlink(config.MCACHE_PATH + "/" + path)
+# FIXME: Uncomment this when ticket #2726 is solved
+# https://fedorahosted.org/sssd/ticket/2726
+# for path in os.listdir(config.MCACHE_PATH):
+#os.unlink(config.MCACHE_PATH + "/" + path)




+def format_interactive_conf(ldap_conn, schema):
+"""Format an SSSD configuration with all caches refreshing in 4
seconds"""
+return \
+format_basic_conf(ldap_conn, schema, enum=True) + \
+unindent("""
+[nss]
+memcache_timeout= 4


It is better to set memcache timeout to zero outside tests
that are not dedicated to memcache. This will also probably
solve the membership tests failure that you saw when using the
workaround for the memcache tests failure.


Hmm, perhaps. However, could you please explain why the membership tests
fail?

I noticed that they also fail if I simply don't run the group
addition/removal
tests before them, in addition to enabling the workaround. Also, they
work if
I put "run_shell" before each "assert" and immediately exit the spawned
shells
even without doing anything in them.


I really do not know why the run_shell helped. Maybe it
forced pytest to initialize new client memory cache context.
I tried to do some tests with run_shell, but they did not
work for me.

Anyway, the failures were connected to memory cache
and memory cache currently is not reliable in these
test, so I did not investigated further, why they
failed.




+enum_cache_timeout  = 4
+entry_negative_timeout  = 4


I would set negative cache timeout to zero as well,
see comments below.


Replying below.


+def test_add_remove_user(ldap_conn, blank_rfc2307):
+"""Test user addition and removal are reflected by SSSD"""
+e = ldap_ent.user(ldap_conn.ds_inst.base_dn, "user", 1001, 2000)
+time.sleep(2)


What is the purpose of this timeout? It does not seem to be
necessary. You use it in all the other tests as well, so
I guess it had some meaning (but I tried to remove them and
tests passed for me without problems).


This puts test actions and tests in the middle of the 4 second cache
timeouts,
so they are more reliable and time drift doesn't affect them that much.

E.g.:

 0 - sssd start
 1 -
 2 - add user, check it's not yet present
 3 -
 4 - cache expiry/purging
 5 -
 6 - check user added
 7 -
 8 - cache expiry/purging

etc.

IIRC, I got 400+ perfect runs before the first failure occurred with this -
better than other schemes.


Hmmm...maybe this was because SSSD was not initialized (fully
started) when the test was run. In which case it is problem
of the fixture that starts SSSD and should be solved inside
the fixture (the sleep() should be added there).




+# Add the user
+ent.assert_passwd(ent.contains_only())
+ldap_conn.add_s(*e)
+ent.assert_passwd(ent.contains_only())


I would avoid testing the negative cache outside tests
that are dedicated to negative cache. We had this same
pattern in our C code tests and it timed out when CI
was under heavy load. We can add dedicated tests
for negative cache in CI later with big enough timeout
to pass even under heavy load CI.

So please, remove the negative cache testing from everywhere
in these tests.


I'm OK with minimizing the tests and not exercising some caching
mechanisms. I understand it will make tests more reliable.

However, my intention was to test the full end-user functionality, what
actually matters to users. Users don't really care about caches, they just
want everything work fast and reliably. They just want their LDAP changes
propagated. For that matter I only touch cache timeouts to make tests
run in
reasonable time. I don't disable them, because users will probably have
them
enabled as well.

We can test all the cache mechanisms separately, but we will still have to
test them working together. Can we do that? Can we have these particular
tests
do that? Or is it too hard/impossible?

If we can do that, how would you like to see them?

If not, I'll just disable memory cache and remove negative cache testing 

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Nikolai Kondrashov]

Hi Michal,

Thanks a lot for the detailed review and testing!
Please see my comments below.

On 10/23/2015 02:54 PM, Michal Židek wrote:

Hi!

There is one new pep8 error in the code:
../src/tests/intg/ldap_test.py:819:37: E126 continuation line over-indented for 
hanging indent


Sure, will fix it.


For the memcache workaroud please do this change in the
code:
diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
index 8263d88..eb466ab 100644
--- a/src/tests/intg/ldap_test.py
+++ b/src/tests/intg/ldap_test.py
@@ -194,8 +194,10 @@ def cleanup_sssd_process():
  subprocess.call(["sss_cache", "-E"])
  for path in os.listdir(config.DB_PATH):
  os.unlink(config.DB_PATH + "/" + path)
-for path in os.listdir(config.MCACHE_PATH):
-os.unlink(config.MCACHE_PATH + "/" + path)
+# FIXME: Uncomment this when ticket #2726 is solved
+# https://fedorahosted.org/sssd/ticket/2726
+# for path in os.listdir(config.MCACHE_PATH):
+#os.unlink(config.MCACHE_PATH + "/" + path)




+def format_interactive_conf(ldap_conn, schema):
+"""Format an SSSD configuration with all caches refreshing in 4 seconds"""
+return \
+format_basic_conf(ldap_conn, schema, enum=True) + \
+unindent("""
+[nss]
+memcache_timeout= 4


It is better to set memcache timeout to zero outside tests
that are not dedicated to memcache. This will also probably
solve the membership tests failure that you saw when using the
workaround for the memcache tests failure.


Hmm, perhaps. However, could you please explain why the membership tests fail?

I noticed that they also fail if I simply don't run the group addition/removal
tests before them, in addition to enabling the workaround. Also, they work if
I put "run_shell" before each "assert" and immediately exit the spawned shells
even without doing anything in them.


+enum_cache_timeout  = 4
+entry_negative_timeout  = 4


I would set negative cache timeout to zero as well,
see comments below.


Replying below.


+def test_add_remove_user(ldap_conn, blank_rfc2307):
+"""Test user addition and removal are reflected by SSSD"""
+e = ldap_ent.user(ldap_conn.ds_inst.base_dn, "user", 1001, 2000)
+time.sleep(2)


What is the purpose of this timeout? It does not seem to be
necessary. You use it in all the other tests as well, so
I guess it had some meaning (but I tried to remove them and
tests passed for me without problems).


This puts test actions and tests in the middle of the 4 second cache timeouts,
so they are more reliable and time drift doesn't affect them that much.

E.g.:

0 - sssd start
1 -
2 - add user, check it's not yet present
3 -
4 - cache expiry/purging
5 -
6 - check user added
7 -
8 - cache expiry/purging

etc.

IIRC, I got 400+ perfect runs before the first failure occurred with this -
better than other schemes.


+# Add the user
+ent.assert_passwd(ent.contains_only())
+ldap_conn.add_s(*e)
+ent.assert_passwd(ent.contains_only())


I would avoid testing the negative cache outside tests
that are dedicated to negative cache. We had this same
pattern in our C code tests and it timed out when CI
was under heavy load. We can add dedicated tests
for negative cache in CI later with big enough timeout
to pass even under heavy load CI.

So please, remove the negative cache testing from everywhere
in these tests.


I'm OK with minimizing the tests and not exercising some caching
mechanisms. I understand it will make tests more reliable.

However, my intention was to test the full end-user functionality, what
actually matters to users. Users don't really care about caches, they just
want everything work fast and reliably. They just want their LDAP changes
propagated. For that matter I only touch cache timeouts to make tests run in
reasonable time. I don't disable them, because users will probably have them
enabled as well.

We can test all the cache mechanisms separately, but we will still have to
test them working together. Can we do that? Can we have these particular tests
do that? Or is it too hard/impossible?

If we can do that, how would you like to see them?

If not, I'll just disable memory cache and remove negative cache testing as
you request.

Or do I misunderstand the idea behind this?


+time.sleep(4)


Instead of the number 4, could you use some global constant. You
could use the constant to generate the "interactive" config
file as well. If we later see some timeout issues related to
this, we can change it on one place.


Yes, good idea, thank you.


Also I think that this
particular timeout will not be necessary if we stop testing the
negative cache, which will speed up the tests.


OK, if we decide to disable negative tests,
then I'll see if I can speed it up.


+conf = \
+format_basic_conf(ldap_conn, SCHEMA_RFC2307, enum=True) 

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Michal Židek]

Hi!

There is one new pep8 error in the code:
../src/tests/intg/ldap_test.py:819:37: E126 continuation line 
over-indented for hanging indent



For the memcache workaroud please do this change in the
code:
diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
index 8263d88..eb466ab 100644
--- a/src/tests/intg/ldap_test.py
+++ b/src/tests/intg/ldap_test.py
@@ -194,8 +194,10 @@ def cleanup_sssd_process():
 subprocess.call(["sss_cache", "-E"])
 for path in os.listdir(config.DB_PATH):
 os.unlink(config.DB_PATH + "/" + path)
-for path in os.listdir(config.MCACHE_PATH):
-os.unlink(config.MCACHE_PATH + "/" + path)
+# FIXME: Uncomment this when ticket #2726 is solved
+# https://fedorahosted.org/sssd/ticket/2726
+# for path in os.listdir(config.MCACHE_PATH):
+#os.unlink(config.MCACHE_PATH + "/" + path)


See some comments bellow...

On 10/12/2015 03:31 PM, Nikolai Kondrashov wrote:


0002-intg-Add-more-LDAP-tests.patch


 From a2c75e045f8f341402a5c771489b462f18e27a39 Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov
Date: Tue, 29 Sep 2015 21:18:18 +0300
Subject: [PATCH 2/3] intg: Add more LDAP tests

Add a bunch of LDAP tests.

 * Adding/removing a user/group/membership with rfc2307(bis) schema.
 * Filtering users/groups with rfc2307(bis) schema.
 * The effect of override_homedir option.
 * The effect of fallback_homedir option.
 * The effect of override_shell option.
 * The effect of shell_fallback option.
 * The effect of default_shell option.
 * The effect of vetoed_shells option.
---
  src/tests/intg/ldap_test.py | 534 
  1 file changed, 534 insertions(+)

diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
index 6a09b37..cca9431 100644
--- a/src/tests/intg/ldap_test.py
+++ b/src/tests/intg/ldap_test.py
@@ -125,6 +125,23 @@ def format_basic_conf(ldap_conn, schema, enum):
  """).format(**locals())


+def format_interactive_conf(ldap_conn, schema):
+"""Format an SSSD configuration with all caches refreshing in 4 seconds"""
+return \
+format_basic_conf(ldap_conn, schema, enum=True) + \
+unindent("""
+[nss]
+memcache_timeout= 4


It is better to set memcache timeout to zero outside tests
that are not dedicated to memcache. This will also probably
solve the membership tests failure that you saw when using the
workaround for the memcache tests failure.


+enum_cache_timeout  = 4
+entry_negative_timeout  = 4


I would set negative cache timeout to zero as well,
see comments below.


+
+[domain/LDAP]
+ldap_enumeration_refresh_timeout= 4
+ldap_purge_cache_timeout= 1
+entry_cache_timeout = 4
+""")
+
+
  def create_conf_file(contents):
  """Create sssd.conf with specified contents"""
  conf = open(config.CONF_PATH, "w")
@@ -387,3 +404,520 @@ def test_refresh_after_cleanup_task(ldap_conn, 
refresh_after_cleanup_task):
  ent.assert_group_by_name(
  "group2",
  dict(mem=ent.contains_only("user1")))
+
+
+@pytest.fixture
+def blank_rfc2307(request, ldap_conn):
+"""Create blank RFC2307 directory fixture with interactive SSSD conf"""
+create_ldap_cleanup(request, ldap_conn)
+create_conf_fixture(request,
+format_interactive_conf(ldap_conn, SCHEMA_RFC2307))
+create_sssd_fixture(request)
+
+
+@pytest.fixture
+def blank_rfc2307_bis(request, ldap_conn):
+"""Create blank RFC2307bis directory fixture with interactive SSSD conf"""
+create_ldap_cleanup(request, ldap_conn)
+create_conf_fixture(request,
+format_interactive_conf(ldap_conn, SCHEMA_RFC2307_BIS))
+create_sssd_fixture(request)
+
+
+@pytest.fixture
+def user_and_group_rfc2307(request, ldap_conn):
+"""
+Create an RFC2307 directory fixture with interactive SSSD conf,
+one user and one group
+"""
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+ent_list.add_user("user", 1001, 2000)
+ent_list.add_group("group", 2001)
+create_ldap_fixture(request, ldap_conn, ent_list)
+create_conf_fixture(request,
+format_interactive_conf(ldap_conn, SCHEMA_RFC2307))
+create_sssd_fixture(request)
+return None
+
+
+@pytest.fixture
+def user_and_groups_rfc2307_bis(request, ldap_conn):
+"""
+Create an RFC2307bis directory fixture with interactive SSSD conf,
+one user and two groups
+"""
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
+ent_list.add_user("user", 1001, 2000)
+ent_list.add_group_bis("group1", 2001)
+ent_list.add_group_bis("group2", 2002)
+create_ldap_fixture(request, ldap_conn, ent_list)
+create_conf_fixture(request,
+

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Michal Židek]

On 10/12/2015 03:31 PM, Nikolai Kondrashov wrote:

On 10/08/2015 02:39 PM, Nikolai Kondrashov wrote:

On 10/08/2015 10:32 AM, Lukas Slebodnik wrote:

On (07/10/15 20:51), Nikolai Kondrashov wrote:

On 09/30/2015 06:15 PM, Nikolai Kondrashov wrote:

Hi everyone,

Here is a patch set fixing some things in integration tests and
adding more
LDAP tests:

 * Adding/removing a user/group/membership with rfc2307(bis)
schema.
 * Filtering users/groups with rfc2307(bis) schema.
 * The effect of override_homedir option.
 * The effect of fallback_homedir option.
 * The effect of override_shell option.
 * The effect of shell_fallback option.
 * The effect of default_shell option.
 * The effect of vetoed_shells option.

These are pretty basic, but I think they're good for the start.
Suggestions for more tests are welcome :)

NOTE: These still break test_memory_cache.py as seen in the
attached log file.
   We need to figure out why and do something with it.
Otherwise, the
   tests work fine.


Here's another version of the patch set. It's not complete, but
takes some
comments into account. Namely:

* Explicitly name the new arguments for ldap_ent.user and
  ldap_ent.List.add_user, instead of using "kwargs". Add "gecos"
too. This
  makes the function more suitable for Pavel Reichl's needs.
* Don't remove "_rfc2307" from function names anywhere.
* Use a string "schema" argument with configuration formatting
functions
  instead of boolean "bis" argument to support other schemas. Use
  constants to specify the values in invocations.
* Explicitly specify "enum" argument name when invoking
configuration
  formatting functions.
* Remove duplicate "blank" fixture function.

I'll continue working on the patch set. Namely adding tests without
enumeration, looking for and fixing the memory cache test failures
induced by
the new tests, trying to move commonly used fixtures and other
functions to a
module so we don't copy them.


The problem with failing test_add_remove_user after changing order with
test_memorycache is fixed with this patch set.


Hmm, curioser and curioser. I have no idea what that was.


Here's another version of the patch, which is still interfering with the
memory cache tests. However, I did some digging around and added a test
in a
separate patch which makes it easier to reproduce the issue. In this
test it
seems that the cache file is not invalidated with "sss_cache -E", if
there was
a LDAP enumeration refresh before that.

Other changes are:

 * Add full PEP8 cleanup for integration tests prior to adding more
tests


Could you please send the PEP8 changes in a separate thread? They
LGTM, but I would like to ACK them in a separate thread so that they
are not blocked by review of the other 2 patches (btw. I will look at
those as well soon).


   (can submit as a separate patch if necessary, otherwise feel free to
   merge)
 * Cleaned up the PEP8 errors in the new tests.

I'll not be able to work on this anymore this week, but will resume next
week.
Perhaps someone will be interested to look at that new test and figure
out the
failure reason in the meantime.

Nick


___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Nikolai Kondrashov]

On 10/08/2015 02:39 PM, Nikolai Kondrashov wrote:

On 10/08/2015 10:32 AM, Lukas Slebodnik wrote:

On (07/10/15 20:51), Nikolai Kondrashov wrote:

On 09/30/2015 06:15 PM, Nikolai Kondrashov wrote:

Hi everyone,

Here is a patch set fixing some things in integration tests and adding more
LDAP tests:

 * Adding/removing a user/group/membership with rfc2307(bis) schema.
 * Filtering users/groups with rfc2307(bis) schema.
 * The effect of override_homedir option.
 * The effect of fallback_homedir option.
 * The effect of override_shell option.
 * The effect of shell_fallback option.
 * The effect of default_shell option.
 * The effect of vetoed_shells option.

These are pretty basic, but I think they're good for the start.
Suggestions for more tests are welcome :)

NOTE: These still break test_memory_cache.py as seen in the attached log file.
   We need to figure out why and do something with it. Otherwise, the
   tests work fine.


Here's another version of the patch set. It's not complete, but takes some
comments into account. Namely:

* Explicitly name the new arguments for ldap_ent.user and
  ldap_ent.List.add_user, instead of using "kwargs". Add "gecos" too. This
  makes the function more suitable for Pavel Reichl's needs.
* Don't remove "_rfc2307" from function names anywhere.
* Use a string "schema" argument with configuration formatting functions
  instead of boolean "bis" argument to support other schemas. Use
  constants to specify the values in invocations.
* Explicitly specify "enum" argument name when invoking configuration
  formatting functions.
* Remove duplicate "blank" fixture function.

I'll continue working on the patch set. Namely adding tests without
enumeration, looking for and fixing the memory cache test failures induced by
the new tests, trying to move commonly used fixtures and other functions to a
module so we don't copy them.


The problem with failing test_add_remove_user after changing order with
test_memorycache is fixed with this patch set.


Hmm, curioser and curioser. I have no idea what that was.


Here's another version of the patch, which is still interfering with the
memory cache tests. However, I did some digging around and added a test in a
separate patch which makes it easier to reproduce the issue. In this test it
seems that the cache file is not invalidated with "sss_cache -E", if there was
a LDAP enumeration refresh before that.

Other changes are:

* Add full PEP8 cleanup for integration tests prior to adding more tests
  (can submit as a separate patch if necessary, otherwise feel free to
  merge)
* Cleaned up the PEP8 errors in the new tests.

I'll not be able to work on this anymore this week, but will resume next week.
Perhaps someone will be interested to look at that new test and figure out the
failure reason in the meantime.

Nick
>From 740b37d877ce90a790845c60c0a174188a15d3fa Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov 
Date: Fri, 9 Oct 2015 15:33:16 +0300
Subject: [PATCH 1/3] intg: Fix all PEP8 issues

---
 src/tests/intg/ds.py  | 14 -
 src/tests/intg/ds_openldap.py | 39 +
 src/tests/intg/ent.py | 47 ++
 src/tests/intg/ent_test.py| 68 +--
 src/tests/intg/ldap_test.py   | 56 +++
 src/tests/intg/util.py|  2 +-
 6 files changed, 121 insertions(+), 105 deletions(-)

diff --git a/src/tests/intg/ds.py b/src/tests/intg/ds.py
index fe93384..df08fac 100644
--- a/src/tests/intg/ds.py
+++ b/src/tests/intg/ds.py
@@ -35,13 +35,13 @@ class DS:
 admin_rdn   Administrator DN, relative to BASE_DN.
 admin_pwAdministrator password.
 """
-self.dir= dir
-self.port   = port
-self.ldap_url   = "ldap://localhost:; + str(self.port)
-self.base_dn= base_dn
-self.admin_rdn  = admin_rdn
-self.admin_dn   = admin_rdn + "," + base_dn
-self.admin_pw   = admin_pw
+self.dir = dir
+self.port = port
+self.ldap_url = "ldap://localhost:; + str(self.port)
+self.base_dn = base_dn
+self.admin_rdn = admin_rdn
+self.admin_dn = admin_rdn + "," + base_dn
+self.admin_pw = admin_pw
 
 def setup(self):
 """Setup the instance"""
diff --git a/src/tests/intg/ds_openldap.py b/src/tests/intg/ds_openldap.py
index ba7fde6..ab313cb 100644
--- a/src/tests/intg/ds_openldap.py
+++ b/src/tests/intg/ds_openldap.py
@@ -55,23 +55,23 @@ class DSOpenLDAP(DS):
 admin_pwAdministrator password.
 """
 DS.__init__(self, dir, port, base_dn, admin_rdn, admin_pw)
-self.run_dir= self.dir + "/var/run/ldap"
-self.pid_path   = self.run_dir + "/slapd.pid"
-self.conf_dir   = self.dir + 

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Lukas Slebodnik]

On (07/10/15 20:51), Nikolai Kondrashov wrote:
>On 09/30/2015 06:15 PM, Nikolai Kondrashov wrote:
>>Hi everyone,
>>
>>Here is a patch set fixing some things in integration tests and adding more
>>LDAP tests:
>>
>> * Adding/removing a user/group/membership with rfc2307(bis) schema.
>> * Filtering users/groups with rfc2307(bis) schema.
>> * The effect of override_homedir option.
>> * The effect of fallback_homedir option.
>> * The effect of override_shell option.
>> * The effect of shell_fallback option.
>> * The effect of default_shell option.
>> * The effect of vetoed_shells option.
>>
>>These are pretty basic, but I think they're good for the start.
>>Suggestions for more tests are welcome :)
>>
>>NOTE: These still break test_memory_cache.py as seen in the attached log file.
>>   We need to figure out why and do something with it. Otherwise, the
>>   tests work fine.
>
>Here's another version of the patch set. It's not complete, but takes some
>comments into account. Namely:
>
>* Explicitly name the new arguments for ldap_ent.user and
>  ldap_ent.List.add_user, instead of using "kwargs". Add "gecos" too. This
>  makes the function more suitable for Pavel Reichl's needs.
>* Don't remove "_rfc2307" from function names anywhere.
>* Use a string "schema" argument with configuration formatting functions
>  instead of boolean "bis" argument to support other schemas. Use
>  constants to specify the values in invocations.
>* Explicitly specify "enum" argument name when invoking configuration
>  formatting functions.
>* Remove duplicate "blank" fixture function.
>
>I'll continue working on the patch set. Namely adding tests without
>enumeration, looking for and fixing the memory cache test failures induced by
>the new tests, trying to move commonly used fixtures and other functions to a
>module so we don't copy them.
>
The problem with failing test_add_remove_user after changing order with
test_memorycache is fixed with this patch set. It would good to push
first 5 patches so we can avoid conflicts with other patches.
Then we can focus on inter test failures.

First 5 patches looks good to me.
But there are still new pep8 warnings

git diff HEAD~6..HEAD~ | pep8 --diff | wc -l

Jakub,
does it look good to you?

Pavel R.,
could these patches used with your tests for local override?

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Jakub Hrozek]

On Thu, Oct 08, 2015 at 09:32:14AM +0200, Lukas Slebodnik wrote:
> Jakub,
> does it look good to you?

LGTM (visual inspection only)
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Pavel Reichl]

On 10/08/2015 09:32 AM, Lukas Slebodnik wrote:


Pavel R.,
could these patches used with your tests for local override?

LS



I'll have to amend the patches, but that was agreed to. Seems that 
functionality I need is there so fine by me.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Lukas Slebodnik]

On (08/10/15 10:33), Pavel Reichl wrote:
>
>
>On 10/08/2015 09:32 AM, Lukas Slebodnik wrote:
>>
>>Pavel R.,
>>could these patches used with your tests for local override?
>>
>>LS
>>
>
>I'll have to amend the patches, but that was agreed to. Seems that 
>functionality I need is there so fine by me.

Nikolai,

please fix pep8 warnings in first 5 patches so we can push them
and unblock other patches.

It might be good to send it in separate thread because
it might take some time to address issues with odering of tests.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Nikolai Kondrashov]

On 10/08/2015 10:32 AM, Lukas Slebodnik wrote:

On (07/10/15 20:51), Nikolai Kondrashov wrote:

On 09/30/2015 06:15 PM, Nikolai Kondrashov wrote:

Hi everyone,

Here is a patch set fixing some things in integration tests and adding more
LDAP tests:

 * Adding/removing a user/group/membership with rfc2307(bis) schema.
 * Filtering users/groups with rfc2307(bis) schema.
 * The effect of override_homedir option.
 * The effect of fallback_homedir option.
 * The effect of override_shell option.
 * The effect of shell_fallback option.
 * The effect of default_shell option.
 * The effect of vetoed_shells option.

These are pretty basic, but I think they're good for the start.
Suggestions for more tests are welcome :)

NOTE: These still break test_memory_cache.py as seen in the attached log file.
   We need to figure out why and do something with it. Otherwise, the
   tests work fine.


Here's another version of the patch set. It's not complete, but takes some
comments into account. Namely:

* Explicitly name the new arguments for ldap_ent.user and
  ldap_ent.List.add_user, instead of using "kwargs". Add "gecos" too. This
  makes the function more suitable for Pavel Reichl's needs.
* Don't remove "_rfc2307" from function names anywhere.
* Use a string "schema" argument with configuration formatting functions
  instead of boolean "bis" argument to support other schemas. Use
  constants to specify the values in invocations.
* Explicitly specify "enum" argument name when invoking configuration
  formatting functions.
* Remove duplicate "blank" fixture function.

I'll continue working on the patch set. Namely adding tests without
enumeration, looking for and fixing the memory cache test failures induced by
the new tests, trying to move commonly used fixtures and other functions to a
module so we don't copy them.


The problem with failing test_add_remove_user after changing order with
test_memorycache is fixed with this patch set.


Hmm, curioser and curioser. I have no idea what that was.


It would good to push
first 5 patches so we can avoid conflicts with other patches.
Then we can focus on inter test failures.


Sure, good idea.


First 5 patches looks good to me.
But there are still new pep8 warnings

git diff HEAD~6..HEAD~ | pep8 --diff | wc -l


Yes, I was leaving that for today, forgot to mention above.

Thank you.

Nick
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Nikolai Kondrashov]

On 10/08/2015 11:39 AM, Lukas Slebodnik wrote:

On (08/10/15 10:33), Pavel Reichl wrote:



On 10/08/2015 09:32 AM, Lukas Slebodnik wrote:


Pavel R.,
could these patches used with your tests for local override?

LS



I'll have to amend the patches, but that was agreed to. Seems that 
functionality I need is there so fine by me.


Nikolai,

please fix pep8 warnings in first 5 patches so we can push them
and unblock other patches.


I'm doing that right now. However, should we really comply with the following
warnings?

E126 continuation line over-indented for hanging indent
E131 continuation line unaligned for hanging indent
E221 multiple spaces before operator
E241 multiple spaces after ','
E251 unexpected spaces around keyword / parameter equals

These complain about whitespace which is used to try to format the code better
for humans and is subjective. Do we agree with them? Or can we keep our
formatting?

Nick
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Lukas Slebodnik]

On (08/10/15 16:29), Nikolai Kondrashov wrote:
>On 10/08/2015 11:39 AM, Lukas Slebodnik wrote:
>>On (08/10/15 10:33), Pavel Reichl wrote:
>>>
>>>
>>>On 10/08/2015 09:32 AM, Lukas Slebodnik wrote:

Pavel R.,
could these patches used with your tests for local override?

LS

>>>
>>>I'll have to amend the patches, but that was agreed to. Seems that 
>>>functionality I need is there so fine by me.
>>
>>Nikolai,
>>
>>please fix pep8 warnings in first 5 patches so we can push them
>>and unblock other patches.
>
>I'm doing that right now. However, should we really comply with the following
>warnings?
>
>E126 continuation line over-indented for hanging indent
>E131 continuation line unaligned for hanging indent
>E221 multiple spaces before operator
>E241 multiple spaces after ','
>E251 unexpected spaces around keyword / parameter equals
>
>These complain about whitespace which is used to try to format the code better
>for humans and is subjective. Do we agree with them? Or can we keep our
>formatting?
>
I ask our collegues from FreeIPA team and they follow these pep8
recommendations. Most of us are not python experts so it would be good to
the follow the same quidelines as they.

They do not strict require 80 characters per line.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Pavel Reichl]

On 10/08/2015 03:50 PM, Nikolai Kondrashov wrote:

On 10/08/2015 04:48 PM, Lukas Slebodnik wrote:

On (08/10/15 16:29), Nikolai Kondrashov wrote:

On 10/08/2015 11:39 AM, Lukas Slebodnik wrote:

On (08/10/15 10:33), Pavel Reichl wrote:



On 10/08/2015 09:32 AM, Lukas Slebodnik wrote:


Pavel R.,
could these patches used with your tests for local override?

LS



I'll have to amend the patches, but that was agreed to. Seems that 
functionality I need is there so fine by me.


Nikolai,

please fix pep8 warnings in first 5 patches so we can push them
and unblock other patches.


I'm doing that right now. However, should we really comply with the following
warnings?

E126 continuation line over-indented for hanging indent
E131 continuation line unaligned for hanging indent
E221 multiple spaces before operator
E241 multiple spaces after ','
E251 unexpected spaces around keyword / parameter equals

These complain about whitespace which is used to try to format the code better
for humans and is subjective. Do we agree with them? Or can we keep our
formatting?


I ask our collegues from FreeIPA team and they follow these pep8
recommendations. Most of us are not python experts so it would be good to
the follow the same quidelines as they.


Argh. OK.


They do not strict require 80 characters per line.


I'd like to keep that requirement.

I'll try to follow with a patch fixing the rest of the issues in the code (not
just introduced with these patches).


We have a ticket for this. It is owned by Pavel Picka but I suppose he didn't 
had any time to work on this.
https://fedorahosted.org/sssd/ticket/2774



Nick
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH v2] intg: Add more LDAP tests

[Nikolai Kondrashov]

On 09/30/2015 06:15 PM, Nikolai Kondrashov wrote:

Hi everyone,

Here is a patch set fixing some things in integration tests and adding more
LDAP tests:

 * Adding/removing a user/group/membership with rfc2307(bis) schema.
 * Filtering users/groups with rfc2307(bis) schema.
 * The effect of override_homedir option.
 * The effect of fallback_homedir option.
 * The effect of override_shell option.
 * The effect of shell_fallback option.
 * The effect of default_shell option.
 * The effect of vetoed_shells option.

These are pretty basic, but I think they're good for the start.
Suggestions for more tests are welcome :)

NOTE: These still break test_memory_cache.py as seen in the attached log file.
   We need to figure out why and do something with it. Otherwise, the
   tests work fine.


Here's another version of the patch set. It's not complete, but takes some
comments into account. Namely:

* Explicitly name the new arguments for ldap_ent.user and
  ldap_ent.List.add_user, instead of using "kwargs". Add "gecos" too. This
  makes the function more suitable for Pavel Reichl's needs.
* Don't remove "_rfc2307" from function names anywhere.
* Use a string "schema" argument with configuration formatting functions
  instead of boolean "bis" argument to support other schemas. Use
  constants to specify the values in invocations.
* Explicitly specify "enum" argument name when invoking configuration
  formatting functions.
* Remove duplicate "blank" fixture function.

I'll continue working on the patch set. Namely adding tests without
enumeration, looking for and fixing the memory cache test failures induced by
the new tests, trying to move commonly used fixtures and other functions to a
module so we don't copy them.

Nick
>From d79fd27165d08efe07b1dafe52898a01ac06a4e3 Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov 
Date: Tue, 29 Sep 2015 20:00:14 +0300
Subject: [PATCH 1/6] intg: Get base DN from LDAP connection object

Don't use the global LDAP_BASE_DN in integration tests and fixtures, but
instead take it from the LDAP connection object (ldap_conn) passed to
them explicitly. This makes the tests and fixtures a bit more modular.
---
 src/tests/intg/ldap_test.py | 8 
 src/tests/intg/test_memory_cache.py | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
index ea114dc..0287a28 100644
--- a/src/tests/intg/ldap_test.py
+++ b/src/tests/intg/ldap_test.py
@@ -105,7 +105,7 @@ def create_sssd_fixture(request):
 
 @pytest.fixture
 def sanity_rfc2307(request, ldap_conn):
-ent_list = ldap_ent.List(LDAP_BASE_DN)
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
 ent_list.add_user("user1", 1001, 2001)
 ent_list.add_user("user2", 1002, 2002)
 ent_list.add_user("user3", 1003, 2003)
@@ -150,7 +150,7 @@ def sanity_rfc2307(request, ldap_conn):
 
 @pytest.fixture
 def simple_rfc2307(request, ldap_conn):
-ent_list = ldap_ent.List(LDAP_BASE_DN)
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
 ent_list.add_user('usr001', 181818, 181818)
 ent_list.add_group("group1", 181818)
 
@@ -181,7 +181,7 @@ def simple_rfc2307(request, ldap_conn):
 
 @pytest.fixture
 def sanity_rfc2307_bis(request, ldap_conn):
-ent_list = ldap_ent.List(LDAP_BASE_DN)
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
 ent_list.add_user("user1", 1001, 2001)
 ent_list.add_user("user2", 1002, 2002)
 ent_list.add_user("user3", 1003, 2003)
@@ -309,7 +309,7 @@ def test_sanity_rfc2307_bis(ldap_conn, sanity_rfc2307_bis):
 
 @pytest.fixture
 def refresh_after_cleanup_task(request, ldap_conn):
-ent_list = ldap_ent.List(LDAP_BASE_DN)
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
 ent_list.add_user("user1", 1001, 2001)
 
 ent_list.add_group_bis("group1", 2001, ["user1"])
diff --git a/src/tests/intg/test_memory_cache.py b/src/tests/intg/test_memory_cache.py
index 1f4ccb0..76d85fd 100644
--- a/src/tests/intg/test_memory_cache.py
+++ b/src/tests/intg/test_memory_cache.py
@@ -109,7 +109,7 @@ def create_sssd_fixture(request):
 
 
 def load_data_to_ldap(request, ldap_conn):
-ent_list = ldap_ent.List(LDAP_BASE_DN)
+ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn)
 ent_list.add_user("user1", 1001, 2001)
 ent_list.add_user("user2", 1002, 2002)
 ent_list.add_user("user3", 1003, 2003)
-- 
2.5.1

>From fb6f62e692c6acb03e046bb9dccce5b0fedb8265 Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov 
Date: Mon, 28 Sep 2015 16:12:48 +0300
Subject: [PATCH 2/6] intg: Add support for specifying all user attrs

Support passing all user attributes to ldap_ent.py's user-creation
functions, in integration tests.
---
 src/tests/intg/ldap_ent.py | 45 -
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git