On Fri, 2011-04-08 at 07:27 -0700, John Admanski wrote:
> 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.
^ Exactly what I thought when writing this method.
>
> 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 thought of something similar, passing an additional parameter that
would then make run_test to return a tuple (boolean, detail) rather than
a string, but that would be awful and potentially cause problems for
other users.
>
> 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.
>
Well, I can cook such a method for server_job. I've just sent a v2 that
does not contemplate that yet though. v2 does not modify this particular
patch.
> -- 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
