[Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

[Gabe Alford]

Yeah. That makes more sense. Updated patch attached.

Thanks,

Gabe

On Wed, Feb 25, 2015 at 3:55 PM, Tomas Babej <tbabej at redhat.com> wrote:

>  Hi Gabe,
>
> sorry for not being clear. This approach will not work:
>
> +class TestAdvice(BaseTestInvalidAdvice,
> +                 BaseTestFedoraAuthconfig,
> +                 BaseTestFreeBSDNSSPAM,
> +                 BaseTestGenericNSSPAM,
> +                 BaseTestGenericSSSDBefore19,
> +                 BaseTestRedHatNSS,
> +                 BaseTestRedHatNSSPAM,
> +                 BaseTestRedHatSSSDBefore19,
> +                 BaseTestAdvice):
> +    pass
>
> By combining all the base classes into one, you will not get the desired
> effect (which is to run the test_advice method for each advice_id). Let me
> explain why:
>
> The test runner works in the following way: it inspects any discovered
> class which name begins with "Test", and executes each its method, which
> names begins with "test" as a test case.
>
> If the test runner inspects the TestAdvice class, the only method
> beggining with "test", which it will see, is the "test_advice" which was
> inherited back from BaseTestAdvice class. So we can safely conclude the
> test runner will only run 1 test case.
>
> Which one, you may ask? Well, since the test_advice behaviour is fully
> determined by the values of "advice_id", "advice_regex" and "raiseerr"
> attributes, let's look at their values in TestAdvice class. This class does
> not define attirbutes with such names, so we move along the inheritance
> chain (also called MRO) - the first class from which we inherit is
> BaseTestInvalidAdvice, and this class defines all three mentioned
> attributes.
>
> Hence the only test method will be run the test for invalid advice :)
>
> Now, how to fix this? The easiest approach would be to abandon the
> approach with the separate classes, and map each class to a test method in
> the TestAdvice class, like this (from the top of my head):
>
> +class TestAdvice(IntegrationTest):
> +    topology = 'line'
> +
> +    def test_invalid_advice(self):
> +        advice_id = 'invalid-advise-param'
> +        advice_regex = "invalid[\s]+\'advice\'.*"
> +        raiseerr = False
> +        # Obtain the advice from the server
> +        tasks.kinit_admin(self.master)
> +        result = self.master.run_command(['ipa-advise', self.advice_id],
> +                                         raiseonerr=self.raiseerr)
> +
> +        if not result.stdout_text:
> +            advice = result.stderr_text
> +        else:
> +            advice = result.stdout_text
> +
> +        assert re.search(self.advice_regex, advice, re.S)
> +
> +    def test_advice_fedora_authconfig(self):
> +        advice_id = 'config-fedora-authconfig'
> +        advice_regex = "\#\!\/bin\/sh.*" \
> +                       "authconfig[\s]+\-\-enableldap[\s]+" \
> +                       "\-\-ldapserver\=.*[\s]+\-\-enablerfc2307bis[\s]+"
> \
> +                       "\-\-enablekrb5"
> +        raiseonerr = True
> +        # Obtain the advice from the server
> +        tasks.kinit_admin(self.master)
> +        result = self.master.run_command(['ipa-advise', self.advice_id],
> +                                         raiseonerr=self.raiseerr)
> +
> +        if not result.stdout_text:
> +            advice = result.stderr_text
> +        else:
> +            advice = result.stdout_text
> +
> +        assert re.search(self.advice_regex, advice, re.S)
>
> ... the same for the remaining 6 cases
>
> Now, this pattern has lots of duplicated code which can be extracted to a
> helper method, I just thought it would help to be more explicit to get the
> idea across. In the end you can achieve the same level of conciseness than
> with the separate test classes. Good luck!
>
> HTH,
>
> Tomas
>
>
>
>
> On 02/25/2015 03:52 PM, Gabe Alford wrote:
>
>  No worries about the delay. Thanks for taking the time! Updated patch
> attached.
>
>  Thanks,
>
>  Gabe
>
> On Tue, Feb 24, 2015 at 11:03 AM, Tomas Babej <tbabej at redhat.com> wrote:
>
>>  Hi Gabe,
>>
>> sorry for the delay. Here comes the review!
>>
>  1.) All the tests fail, since the IPA master is not installed at all:
>>
>>     def test_advice(self):
>>         # Obtain the advice from the server
>> >       tasks.kinit_admin(self.master)
>>
>> test_integration/test_advise.py:37:
>> _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
>> test_integration/tasks.py:484: in kinit_admin
>>     stdin_text=host.config.admin_password)
>> ../pytest_multihost/host.py:222: in run_command
>>     command.wait(raiseonerr=raiseonerr)
>> _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
>>
>> self = <pytest_multihost.transport.SSHCommand object at 0x7f09c0530c90>
>> raiseonerr = True
>>
>>     def wait(self, raiseonerr=True):
>>         """Wait for the remote process to exit
>>
>>             Raises an excption if the exit code is not 0, unless raiseonerr is
>>             true.
>>             """
>>         if self._done:
>>             return self.returncode
>>
>>         self._end_process()
>>
>>         self._done = True
>>
>>         if raiseonerr and self.returncode:
>>             self.log.error('Exit code: %s', self.returncode)
>> >           raise subprocess.CalledProcessError(self.returncode, self.argv)
>> E           CalledProcessError: Command '['kinit', 'admin']' returned non-zero exit status 1
>>
>>
>> Similiarly for other tests. This is caused by the fact that you did not
>> set topology in the BaseTestAdvise class, like this:
>>
>> --- a/ipatests/test_integration/test_advise.py
>> +++ b/ipatests/test_integration/test_advise.py
>> @@ -31,6 +31,7 @@ class BaseTestAdvise(IntegrationTest, object):
>>      advice_id = None
>>      raiseerr = None
>>      advice_regex = ''
>> +    topology = 'line'
>>
>> 2.) BaseTestAdvise inherits from IntegrationTest and from object.
>> Explicitly specifying object as superclass is not needed, IntegrationTest
>> already inherits from it.
>>
>> 3.) I think there is no good incentive to separate the test cases into
>> mutliple classes. Each test class adds overhead of installing and
>> uninstalling IPA server, to guarantee a clean and sane environment.
>> However, it seems to be an overkill for testing ipa-advise command, which
>> should be read-only anyway. By squashing the tests into one test class, we
>> will decrease the run time of this test more than 8-fold.
>>
>> 4.) The patch adds a whitespace error.
>>
>> The test cases themselves are looking fine, and when I fixed the missing
>> topology, they all passed. So this is a question of fixing the above
>> issues, and we should be ready to push.
>>
>> Tomas
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150226/25b973f0/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rga-0039-3-ipatests-Add-tests-for-valid-and-invalid-ipa-advise.patch
Type: application/octet-stream
Size: 5855 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150226/25b973f0/attachment.obj>