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) + \
> + unindent("""
> + [nss]
> + filter_users = {filter_users_str}
> + filter_users_in_groups = {filter_users_in_groups}
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.
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.
Do you have any particular design for this in mind?
> + conf = \
> + format_basic_conf(ldap_conn, SCHEMA_RFC2307, enum=True) + \
> + unindent("""
> + [nss]
> + filter_groups = {filter_groups_str}
Same as above, please do not generate the conf file inside the test, but
split it to multiple tests.
Please see my comment right above.
> + conf = \
> + format_basic_conf(ldap_conn, SCHEMA_RFC2307_BIS, enum=True) + \
> + unindent("""
> + [nss]
> + filter_groups = {filter_groups_str}
The same thing as above.
Please see my comment above.
> Add a memcache invalidation failure integration test. It fails as
is,
> and passes if ldap_enumeration_refresh_timeout in the first half is set
> to e.g. 30. The sss_nss_check_header function was observed to always
> return 0 for the second half of the test, when it was failing.
This test passes for me with the memcache workaround. I guess we do
not need it then. The reproducer for the failure can be achieved
by removing the workaround (which we will do once we fix the
root cause).
Do you mean we should not merge it, remove it from the patchset? Or add it
along with a workaround, so after we fix the failure and remove the workaround
it stays on the lookout?
Just for info, this is git diff with
changes I made when testing your patches:
I see, thank you!
Sincerely,
Nick