[SSSD] [PATCH] Add integration tests

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Wed Mar 18 14:48:34 UTC 2015


Hi Michal,

Thanks for the review! Please see my replies below.

On 03/18/2015 03:06 AM, Michal Židek wrote:
> 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.

My idea for unindent() is to avoid typing and caring for extra syntax for long
multiline text, which I noticed is often used in integration tests for
configuration generation.

Compare these two:

     unindent("""\
         [sssd]                               "[sssd]"\
         debug_level         = 0xffff         "debug_level         = 0xffff"\
         config_file_version = 2              "config_file_version = 2"\
         domains             = LDAP           "domains             = LDAP"\
         services            = nss, pam       "services            = nss, pam"\
                                              ""\
         [nss]                                "[nss]"\
         debug_level         = 0xffff         "debug_level         = 0xffff"\
                                              ""\
         [pam]                                "[pam]"\
         debug_level         = 0xffff         "debug_level         = 0xffff"\
                                              ""\
         [domain/LDAP]                        "[domain/LDAP]"\
         debug_level         = 0xffff         "debug_level         = 0xffff"\
         enumerate           = true           "enumerate           = true"\
         ldap_schema         = rfc2307        "ldap_schema         = rfc2307"\
         id_provider         = ldap           "id_provider         = ldap"\
         auth_provider       = ldap           "auth_provider       = ldap"\
         sudo_provider       = ldap           "sudo_provider       = ldap"\
         ldap_uri            = %s             "ldap_uri            = %s"\
         ldap_search_base    = %s             "ldap_search_base    = %s"
     """)

The right case needs more typing and attention when entering for the first
time, and when editing you need to be careful to not delete either of the
quotes or the backslash. Which you will inevitably do from time to time. It's
also more noisy to the eye and thus a bit harder to read, IMO.

However, if the right-hand option is more comfortable, I'll change it,
no problem.

Regarding a helper variable, I agree it is useful for long arguments, when
function invocations have many arguments. Otherwise, it seems to me, the
indirection of a single-use variable is a comprehension overhead and increases
the invoking function length.

I'll change it, if you think it's best. Would you like me to replace all long
arguments with variables?


> I would also rename the conf_fixture() to
> create_sssd_conf_fixture.

I understand your argument about having a verb in the function name, although
I find it redundant sometimes. However, shall I then rename the other
fixture-creating functions to have "create_" prefix, as well?


> 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.

Ah, I see that you would prefer all long arguments moved to variables.
Alright, will do.


> (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.

Alright. Although I think it should rather be "run_shell", as there is no
"stop_shell" and the function returns when the shell returns. Will also fix
the function description to make that clear.


> 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.

Although, IMO, this is more verbose and complicated, I can do that.
I would like to avoid CamelCase if possible, though.


> ==
> 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.

Yes, I agree the big assert is useless for debugging a test failure.
I'll be working on improving that. But maybe we can submit this as is for the
start, just to have something.

That module is more of a stub, just to get going. However, we still need to
filter "root" out of returned user and group lists and if we want generic
pattern matching against returned entries we'll need to convert them to
generic dictionaries, instead of using what pwd and grp modules return.
So I think, we can use the basics of that module.


> 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 ...

Yeah, we can implement the separate functions for all kinds of assertions and
tests, sure. I just didn't think them through and we don't have the tests that
need them, yet.

I would like to write as little code as possible, and we'll need to do
essentially the same thing for both rfc2307 and rfc2307bis sanity tests, where
we need to verify the whole set of returned users and groups. So perhaps it
will still be worth it to have a single function to verify the whole of it,
only perhaps having assertions *inside* of it, not outside.


> 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

Hmm, this is just a way to save on arguments here.

> makes it impossible to check for aliases, so we should not go that way.

I lack knowledge here. Can you explain what aliases are here, and how we
should check for them?

> 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).

slapd_setup/teardown are written in Bash for brevity and flexibility. Bash is
designed for easy integration of programs and file I/O. In short, Python needs
more syntax and semantic overhead to do these specific things, so growing and
experimenting with slapd setup/teardown is going to be easier and faster with
Bash, IMO. Although, yeah, some things might be cryptic and once it gets over
certain size and complexity it will become less manageable (at that point we
certainly should move to Python). It's the usual special- vs general-purpose
language trade-off.

Still, if for some reason Python is considered more suitable at this point,
I can rewrite it, no problem.

> ==
> 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.

Yeah, I also don't like this, but haven't found a good way to fix it, yet.

Theoretically, a development process might be like this:

     1. Configure a special integration check build.
     2. Build.
     3. Install.
     4. Run integration tests.
     5. Change tests.
     6. Go to 2.

Maybe we can make another top-level target which would do steps 2 to 4 and
invoke it from "intgcheck".

We might also need a separate target for "uninstalling", so we can remove the
temporary prefix directory, and also a single target invoking all of the above
for simplicity.

> 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.

Yes, that would be good. Although, please don't consider the Python code
anywhere close to final, this patch is mostly about integrating integration
tests into our build/process in general.

> 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 :)

Great! Thank you :)

> 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 :) ).

Yes, I agree it needs attention.

Thanks again, Michal!

Nick



More information about the sssd-devel mailing list