[SSSD] [PATCH v3] Add integration tests

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Fri May 15 17:19:58 UTC 2015


Thanks for a quick answer, Michal!

See my (short) answers below.

On 05/15/2015 07:14 PM, Michal Židek wrote:
> On 05/15/2015 03:39 PM, Nikolai Kondrashov wrote:
>>> One of the patches generates whitespace warnings:
>>>
>>> Applying: Add integration tests
>>> /home/user/gitrepo/sssd/.git/rebase-apply/patch:419: new blank line at
>>> EOF.
>>> +
>>> /home/user/gitrepo/sssd/.git/rebase-apply/patch:1163: new blank line
>>> at EOF.
>>> +
>>> /home/user/gitrepo/sssd/.git/rebase-apply/patch:1276: new blank line
>>> at EOF.
>>> +
>>> /home/user/gitrepo/sssd/.git/rebase-apply/patch:1535: new blank line
>>> at EOF.
>>
>> Hmm, I was putting these there on purpose, to have each function end with
>> separating whitespace for ease of re-arranging, and to have some whitespace
>> below the last function when going to the end of the file, so it doesn't
>> stick
>> to editor decorations, making it harder to read. These are new files and
>> I'm
>> not adding spurious whitespace to existing files here.
>>
>> Is this a problem? I can remove it if it is.
>
> I would not call it a problem, but it is better if the git
> am applies without unnecessary warnings and I do not think
> that the newlines in the end add any value (but it may depend
> on editor). I would remove them.
>
> Also I just realized that you use 2 blank lines between functions.
> I think it is a custom to use just one in SSSD (at least this
> is how it is in most of the SSSD code). IMO one is enough.

I was following PEP 8:
https://www.python.org/dev/peps/pep-0008/#blank-lines

I think for Python code it makes more sense than for C code, because Python
doesn't use braces, which add a bit of vertical air and you can use single
empty line inside functions.

Anyway I'll remove them if it is better for sssd.

>> What if we try to make them simpler? Let's remove list/uid/gid/name
>> arguments.
>> Just make diff/assert_passwd/group accept only patterns and always check
>> everything. This way the difference between "contains" and "contains_only"
>> will make sense as we're always checking against the entry list retrieved
>> by get(pw|gr)ent.
>
> Ok, that sounds good IMO.

Great! Will do then.

>>> Also make the diff function _diff, we will not use it outside the
>>> module.
>>
>> I'm not so sure about that. Some corner-case tests might need some special
>> matching and assertions which won't be worth adding to ent.py. At least
>> until
>> they prove useful elsewhere. Since we can't make assertion backtrace
>> start at
>> arbitrary point up-stack, building functions on top of the exposed assert_*
>> functions wouldn't be desirable, and using "diff" in these custom functions
>> would be more appropriate. It will also be more difficult to form coherent
>> error messages on assertion failures if building on top of assert_*
>> functions.
>>
>> Still, it will be easy to expose "diff" later if necessary, and I can do as
>> you request.
>>
>
> You are right that the function could be useful under some
> circumstances, but we shall strive toward clean API, not toward
> a bunch of inconsistent functions that may be useful under some
> circumstances. We have this problem with our sysdb API, where
> we have a whole lot of functions useful under some circumstances
> and overall it is mess.
>
> The diff function exposes too much internal dirty little secrets
> that the user should not care about. For example it takes the
> description map as an argument. None of the functions that
> take the description map as an argument should ever be made
> public. This is for internal use only, to make the asserts
> produce better error messages. If we found our selves in the
> situation where we would like to use the diff function
> directly in the tests, it is probably time to introduce new
> assert_ function to the API, that can use the diff internally.
>
> Also the diff_passwd and diff_group should be made static.
> The fact that we use some diffs is really of no importance
> to the assert_ function caller.

Well, I was talking about the case where we might want to add an assert_
function which makes sense for one test file only and is too specific (at
least for the start) to be put into ent.py.

Still, if we find that a problem, we can always expose these, so let's start
with them hidden.

>>> And one more thing. Use all the assert_ functions in the example
>>> tests. It will serve as example/documentation for later usage.
>>
>> These weren't example tests, just some actual tests to start with.
>> However, I
>> can add some more tests that would use these, no problem. It will take some
>> more time, though.
>>
>
> Well they are the first tests that use this new code, so they
> will be examples for others when writing new tests. It will
> be helpful for developers to see the function in real context.
> So I think it is worth the effort and the tests can be really
> simple. Additionally we will test the assert_ functions
> themselves this way (this is IMO important).

Alright, will do.

Nick



More information about the sssd-devel mailing list