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