On 10/21/2015 11:31 AM, Nikolai Kondrashov wrote:
On 10/20/2015 05:15 PM, Pavel Reichl wrote:
>
>
> On 10/20/2015 03:12 PM, Nikolai Kondrashov wrote:
>> Hi Pavel,
>>
>> On 10/13/2015 12:22 AM, Pavel Reichl wrote:
>>> Please see updated patchset.
>>
>> Thank you for working on this. Please see my comments below.
>>
>> First of all, we need the test in Makefile.am, so it is included into
>> distribution.
>>
>> I'm not sure if we really need this split into three patches, to me a single
>> patch seems better. Anyone against?
>
> Well, I don't like huge patches, but I probably won't cry if we decide to
> merge them. :-)
Yeah, it'll be big, but it'll be simple - just adding a file. Simpler than
three separate patches anyway. At least for me it would have been easier to
review this change as a monolithic patch.
OK, lets compromise, I'll merge first 2 patches - the core tests. I'll keep the
3rd patch, which tests bugs that were already reported and fixed, as a separate patch.
>> Shouldn't a global variable name be uppercase?
> OK, might be I just replace it with
>
> def get_user_export_file():
> return "user_export_file"
Erm, no, I'd rather keep the lowercase variable then :) If we don't have a
convention for uppercase global variables, then it's fine with me.
Actually I asked IPA guys and they agree with upper case - so fine by me. I just wanted to
work around the fact that there are no constants in python so I thought using function
would solve this.
But I can live with USER_EXPORT_FILE.
>>
>>> +
>>> +
>>> +(a)pytest.fixture
>>> +def sanity_rfc2307(request, ldap_conn):
>>
>> Does this and other function have to have "_rfc2307" in them?
>> Do you plan tests for other schemas?
> No, would you propose a better name? Does "set_test_user_env()" makes
sense?
Oh, no, this wouldn't be right. Fixtures are supposed to be named after
particular object or state they provide, they shouldn't be actions (even
though they're functions).
I was just suggesting removing "_rfc2307" from them if you're not going to
be
running with other schemas and thus wouldn't also need e.g. "_rfc2307_bis"
version anytime soon.
Otherwise, "sanity" is OK and is supposed to mean the most basic test there
could be to ensure running the test suite even makes sense, or such.
Thanks for explaining meaning of sanity. It was a bit mystery to me :-)
A fixture
named thusly would indicate it belongs to such test. Since so far
it's the
only fixture we needed, there is no sense in naming it otherwise, but given we
add more we'll need to be more inventive.
>>> + restart_sssd()
>>
>> Why do we need to restart SSSD here?
>
> After override you have to restart SSSD to make the change available.
Hmm, I didn't find anything about this in sss_override(8).
Have found a documentation bug?
Probably yes, patch is already on the list. Thanks!