[SSSD] [PATCHSET] intg: sss_override

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Wed Oct 21 09:31:38 UTC 2015


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.

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

>>
>>> +
>>> +
>>> + at 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. 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?

>> Shouldn't we also check that there are no other users there?
>
> If you mean name overrides that should be dropped ATM then yes, I can add the check.

Yes. Good!

>>
>>> +
>>> +    # reload overrides
>>> +    subprocess.call(["sss_override", "user-import", override_user_filename])
>>> +
>>> +    restart_sssd()
>>
>> Why do we need to restart SSSD here?
>
> I'm not so sure as with override but I think so. I'll ask the author.

Thanks!

>> All of the above said, I would really prefer to see this test split into a few
>> smaller ones, each invoking a (possibly shared) py.test fixture separately.
>> Since py.test doesn't really support nested tests (argh!), we'll need to have
>> them flat.
>
> OK, I'll give it a thought.

Thank you!

>> E.g.:
>>
>>      * simple override (both with and without FQNs)
>>          add entries
>>          assert entries are not overriden
>>          add override
>>          test entries are overriden
>>      * root override
>>          add entries
>>          assert entries are not overriden
>>          add override to root
>>          test entries are not overriden
>>      * override of override
> AFAIK this is not supported.

Well, it was just an example - I might not understand everything right.

>>          add entries
>>          assert entries are not overriden
>>          add override
>>          assert entries are overriden
>>          add override of override
>>          test overrides are overriden
>>      * override removal
>>          add entries
>>          assert entries are not overriden
>>          add overrides
>>          assert entries are overriden
>>          remove overrides
>>          test entries are not overriden
>>      * override export/import
>>          add entries
>>          assert entries are not overriden
>>          add overrides
>>          assert entries are overriden
>>          export overrides
>>          remove overrides
>>          assert entries are not overriden
>>          import overrides
>>          test entries are overriden
>>
>> Making a single test out of all these is tempting, but they are testing
>> different assertions and shouldn't depend on each other. Otherwise reading
>> such a merged test and debugging a failure will be difficult.
>
> Yes, that's true. On the other hand only because of complexity of the tested
> environment we found out about 'use after free' bug, however we would find
> that probably even with simple test run under valgrind...

Well, finding bugs is great, of course. However, I think making tests
more complicated so we can accidentally stumble into another bug is not
productive. Rather we should write more specific, but independent tests.
In this case add another, which tests for that specific issue we found, and
then try to think of some more test scenarios.

Thank you for taking note of my comments, Pavel!

Nick


More information about the sssd-devel mailing list