On Thu, Apr 7, 2011 at 11:25 PM, Gregory P. Smith <g...@google.com> wrote:
>
> On Thu, Apr 7, 2011 at 10:08 PM, Lucas Meneghel Rodrigues
> <l...@redhat.com>wrote:
>
>> For the KVM test, we have for a long time a system of
>> 'test dependencies': If a dependency test, such as guest
>> installation has failed, it is not possible to run subsequent
>> tests for obvious reasons. There are minor failures though,
>> that don't necessarily block other tests, such as some specific
>> types of image problems found on a guest after the routine check.
>>
>> However, job.run_test() returns a boolean, making it not possible
>> to fail a test, so we are not ignoring problems, and at the same
>> time, execute dependent tests, because the failure was deemed to
>> be non-fatal. Therefore, introduce and additional method,
>> job.run_test_detail(), that returns the test status (see all
>> possible statuses on client/common_lib/error.py), rather than
>> a boolean. With this finer grained control, we can implement the
>> kvm subtest dependency system in a better way.
>>
>> Strategy: refactor the run_test method into _run_test_base
>> and creating the 2 implementations, run_test, and
>> run_test_detail in terms of _run_test_base.
>>
>> Risk: High (touches a very very visible test writer API)
>> Tests executed: run_test continues to work, run_test_detail
>> provides the correct return codes and change does not break
>> job_unittest.
>>
>
> This makes sense and seems like a reasonable approach. Just looking at the
> diff I don't see any problems with it.
>
>
I agree that it seems reasonable. It's a bit ugly to have to add a new
method just to return more information than run_test does; but return values
aren't really extensible in a backwards compatible way.
An alternative would be to allow run_test to take an (optional) parameter
where the caller could pass in a dictionary to be populated with additional
info like the resulting test status. Perhaps that's a cure worse than the
disease. :) I don't see a big problem with the run_test_detail design, I
just thought I'd throw a different suggestion out there.
I do think that any API extension like this should be done for server jobs
as well, though. It's unfortunate that it still requires updating both
client/bin/job.py and server/server_job.py, but the run_* methods never
really made it up into the base_job class.
-- John
> a Python style nit though:
>
> + (subdir, testname, group_func, timeout) = self._run_test_base(url,
> +
> *args,
> +
> **dargs)
>
> reads easier as:
>
> + subdir, testname, group_func, timeout = self._run_test_base(
> + url, *args, **dargs)
>
> to me.
>
> -Greg
>
>
>> Signed-off-by: Lucas Meneghel Rodrigues <l...@redhat.com>
>> ---
>> client/bin/job.py | 54
>> +++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 files changed, 50 insertions(+), 4 deletions(-)
>>
>> diff --git a/client/bin/job.py b/client/bin/job.py
>> index 97412c0..9effcdb 100644
>> --- a/client/bin/job.py
>> +++ b/client/bin/job.py
>> @@ -539,10 +539,9 @@ class base_client_job(base_job.base_job):
>> raise error.UnhandledTestError(e)
>>
>>
>> - @_run_test_complete_on_exit
>> - def run_test(self, url, *args, **dargs):
>> + def _run_test_base(self, url, *args, **dargs):
>> """
>> - Summon a test object and run it.
>> + Prepares arguments and run functions to run_test and
>> run_test_detail.
>>
>> @param url A url that identifies the test to run.
>> @param tag An optional keyword argument that will be added to the
>> @@ -550,7 +549,11 @@ class base_client_job(base_job.base_job):
>> @param subdir_tag An optional keyword argument that will be added
>> to the subdir name.
>>
>> - @returns True if the test passes, False otherwise.
>> + @returns:
>> + subdir: Test subdirectory
>> + testname: Test name
>> + group_func: Actual test run function
>> + timeout: Test timeout
>> """
>> group, testname = self.pkgmgr.get_package_name(url, 'test')
>> testname, subdir, tag = self._build_tagged_test_name(testname,
>> dargs)
>> @@ -573,6 +576,25 @@ class base_client_job(base_job.base_job):
>> else:
>> self.record('GOOD', subdir, testname, 'completed
>> successfully')
>>
>> + return (subdir, testname, group_func, timeout)
>> +
>> +
>> + @_run_test_complete_on_exit
>> + def run_test(self, url, *args, **dargs):
>> + """
>> + Summon a test object and run it.
>> +
>> + @param url A url that identifies the test to run.
>> + @param tag An optional keyword argument that will be added to the
>> + test and subdir name.
>> + @param subdir_tag An optional keyword argument that will be added
>> + to the subdir name.
>> +
>> + @returns True if the test passes, False otherwise.
>> + """
>> + (subdir, testname, group_func, timeout) =
>> self._run_test_base(url,
>> +
>> *args,
>> +
>> **dargs)
>> try:
>> self._rungroup(subdir, testname, group_func, timeout)
>> return True
>> @@ -585,6 +607,30 @@ class base_client_job(base_job.base_job):
>> # UnhandledTestError that is caught above.
>>
>>
>> + @_run_test_complete_on_exit
>> + def run_test_detail(self, url, *args, **dargs):
>> + """
>> + Summon a test object and run it, returning test status.
>> +
>> + @param url A url that identifies the test to run.
>> + @param tag An optional keyword argument that will be added to the
>> + test and subdir name.
>> + @param subdir_tag An optional keyword argument that will be added
>> + to the subdir name.
>> +
>> + @returns Test status
>> + @see: client/common_lib/error.py, exit_status
>> + """
>> + (subdir, testname, group_func, timeout) =
>> self._run_test_base(url,
>> +
>> *args,
>> +
>> **dargs)
>> + try:
>> + self._rungroup(subdir, testname, group_func, timeout)
>> + return 'GOOD'
>> + except error.TestBaseException, detail:
>> + return detail.exit_status
>> +
>> +
>> def _rungroup(self, subdir, testname, function, timeout, *args,
>> **dargs):
>> """\
>> subdir:
>> --
>> 1.7.4.2
>>
>> _______________________________________________
>> Autotest mailing list
>> Autotest@test.kernel.org
>> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
>>
>
>
> _______________________________________________
> Autotest mailing list
> Autotest@test.kernel.org
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
>
>
_______________________________________________
Autotest mailing list
Autotest@test.kernel.org
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
