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
On 02/17/2015 03:29 PM, Gabe Alford wrote:
Hello,
I was wondering if I could get a review of this patch.
Thanks,
Gabe
On Thursday, January 29, 2015, Gabe Alford <redhatri...@gmail.com
<mailto:redhatri...@gmail.com>> wrote:
Hello,
Here is a patch for
https://fedorahosted.org/freeipa/ticket/4029 I added test cases
for valid and invalid advice.
Thanks,
Gabe
On Wed, Jan 14, 2015 at 10:23 AM, Tomas Babej <tba...@redhat.com
<javascript:_e(%7B%7D,'cvml','tba...@redhat.com');>> wrote:
On 01/14/2015 06:13 PM, Gabe Alford wrote:
On Wed, Jan 14, 2015 at 10:05 AM, Tomas Babej
<tba...@redhat.com
<javascript:_e(%7B%7D,'cvml','tba...@redhat.com');>> wrote:
On 01/14/2015 06:00 PM, Tomas Babej wrote:
On 01/14/2015 05:37 PM, Tomas Babej wrote:
On 01/14/2015 02:55 PM, Gabe Alford wrote:
Hello,
In looking into
https://fedorahosted.org/freeipa/ticket/4029 I am
wondering if there should be separate ipa-advise test,
Yes/No? Could be handy in the future to test more
ipa-advise output? Or should this test be added to the
test_legacy_clients.py?
Thanks,
Gabe
On Tue, Dec 2, 2014 at 9:21 PM, Gabe Alford
<redhatri...@gmail.com
<javascript:_e(%7B%7D,'cvml','redhatri...@gmail.com');>>
wrote:
Hello,
I was going to try my hand at attempting a patch
for ipa-tests. However in wanting to test my
patch, I am not sure how to run ipa-tests to check
if it works or not. Documentation is not really
clear on what needs to be done to start a test and
run a test. This is for
https://fedorahosted.org/freeipa/ticket/4029
I have attached the patch that I have yet to
really test with ipa-test. Any help on how to test
the patch running ipa-tests would be great. Of
course, if one of the reviewers looks at the patch
and looks good, then I would be happy with that as
well.
Thanks,
Gabe
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
<javascript:_e(%7B%7D,'cvml','Freeipa-devel@redhat.com');>
https://www.redhat.com/mailman/listinfo/freeipa-devel
Hello,
TL;DR: feel free to create a separate ipa-advise test
file. Test requested in this ticket really does not
belong to the legacy clients feature test.
As for the any new tests that might come: I think tests
for ipa-advise that are specific to that particular
feature should be tested with that feature, more so, if
they contain parts that are supposed to work
copy-pasted. If a tests, however, tests a general
behaviour of ipa-advise, it should live in the
ipa-advise namespace, hence separate test file.
HTH,
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej |freeipa.org <http://freeipa.org>
The attached patch looks fine, although, please also
test for a non-zero return code number.
Upon hitting send I noticed you did not include
raiseonerr=False into the run_command call. You need to
do that, otherwise a exception will be raised, since
ipa-advise exited with non-zero return code.
Thanks Tomas.
Which do you prefer: a test_advise.py or an update to the
existing patch?
A new test file, as I pointed out in the second email :) sorry
for splitting.
However, it would be the best if you could spin up a positive
test as well (maybe listing out available advices), not just
this negative one, to justify the overhead reinstalling IPA
for testing this feature.
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej |freeipa.org <http://freeipa.org>
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej |freeipa.org <http://freeipa.org>
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej |freeipa.org <http://freeipa.org>
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
