[Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise
Tomas Babej
tbabej at redhat.com
Thu Feb 26 20:18:00 UTC 2015
ACK.
Pushed to:
ipa-4-1: ddd7fb6a68fd413b1561eab9c29bac18882e5efd
master: ae4ee6b53376bb7f3d1b4707c4e105c91b5cd8ab
On 02/26/2015 05:58 PM, Gabe Alford wrote:
> 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
> <mailto: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
>> <mailto: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/90f7fde3/attachment.htm>
More information about the Freeipa-devel
mailing list