[SSSD] [PATCH] Add integration tests

Michal Židek mzidek at redhat.com
Wed Mar 18 01:06:16 UTC 2015


On 03/10/2015 11:21 AM, Nikolai Kondrashov wrote:
> On 02/27/2015 07:13 PM, Nikolai Kondrashov wrote:
>> Hi everyone,
>>
>> This is my attempt to add basic integration tests. There are almost no
>> tests
>> there at the moment and this is mostly about the infrastructure and
>> the way we
>> might do it.
>>
>> I will be glad to answer any questions and receive any comments or
>> suggestions. I'm sure I did a lot of things in a wrong way :)
>>
>> Thank you!
>>
>> CI: http://sssd-ci.duckdns.org/logs/job/8/40/summary.html
>
> And a fresh CI result:
> http://sssd-ci.duckdns.org/logs/job/9/22/summary.html
>
> It still works.
>
> Nick

Hi Nick,

I have some comments.

I would remove the function "unindent(text)". Just use
the strings already unindented in the functions. For example
before passing string to conf_fixture(), create a helper
variable sssd_conf_string like this:

      sssd_conf_string = "[sssd]"\
                         "debug_level = 0xffff"\
                         "config_file_version = 2"\
                         ...etc

and call the fixture with the sssd_conf_string as
argument. I would also rename the conf_fixture() to
create_sssd_conf_fixture.

There are some places in the patch where you
create complex data structures inside function calls
where function arguments are.
It is more readable to create these complex structures
before the function call and then just pass variables
to the functions.

(nitpick) The function "shell" could have better name.
In general it is better if functions are named as some actions
(for example have verbs as part of identifier). It makes
the code easier to read. In this case something like
"start_shell" would be appropriate IMO.

I also do not like how the entries are passed to the
ldap_fixture. IMO it would be better to create a class
(for example LDAPEntriesList) with methods like
addUser
addGroup
addGroupBis
getList

The instance of that class would then be passed as an
argument to the ldap_fixture. So the code would look
something like:

ldap_entries = LDAPEntriesListt()
ldap_entries.addUser("user1", 1001, 2001)
ldap_entries.addUser(...)
ldap_entries.addGroup(...)
... etc etc
ldap_fixture(request, ldap_conn, ldap_entries)

Then you would call ldap.add_s on every entry
in ldap_entries.getList.

==
The abstract entry in src/tests/intg/ent.py and
especially the implemented operations with it
seem to be unnecessary to me. They are used as
building block for one big assert in the test.
What we could do instead is to implement some
helper assert functions like:

assert_get_user_by_uid(ldap_conn, uid, expected_result)
assert_get_user_by_name(ldap_conn, name, expected_result)
... etc

The expected_result can be a map (for example), and
inside the assert functions we will check only for
existing entries. Like (pseudocode):

if 'name' in expected_results.keys():
   assert (name == pwd.pw_name)
if 'uid' in ...

Also the fact that that you use the pattern
list as both the source for input to the getpw/grXX
functions as well as expected results makes it
impossible to check for aliases, so we should not
go that way. In general I think the input for test and
expeced results should be two diffrent things.

==
Also I am not really sure why the src/tests/intg/slapd_setup
and src/tests/intg/slapd_teardown are written in bash.
It would be IMO better to write it all in python. The
few external tools/programs can be called from python and
the rest would be more readable if it was written
in python (like the url_encode function, which to seems
to be somehow cryptic in bash).

==
To the make initgrcheck target. Currently if I
call 'make initgrcheck' twice in a row the whole thing
is build from scratch even if I added just one test. Would it
be possible to make it somehow smarter and rebuild only
the modified files like make normally does?
But I am OK if this is done as separate ticket and do
not want to block the patches because of this.

Also it may be good if some IPA guy took a look at the python
code, since they have more experience with python testing
and maybe they will see something we don't.

Other than that, I think the needed functionality is there and
it is one we really want to have in the test suite, so I am happy to
see it becoming reality :)

PS: I have not checked the part of the patch that deals with
the build system (Makefile.am etc). Anyone (Lukas?) feel free
to take over reviewing this part (I will most likely not be
available during Wednesday, so see you on Thursday :) ).



More information about the sssd-devel mailing list