On Tue, Mar 30, 2010 at 2:02 PM, Michael Goldish <mgold...@redhat.com> wrote:
>
> ----- "Jason Wang" <jasow...@redhat.com> wrote:
>
>> The patch let the profilers could be specified through configuration
>> file. kvm_stat was kept as the default profiler.
>
> Looks good. Some minor style comments:
>
>> Signed-off-by: Jason Wang <jasow...@redhat.com>
>> ---
>> client/tests/kvm/kvm_utils.py | 23
>> ++++++++++-------------
>> client/tests/kvm/tests_base.cfg.sample | 2 +-
>> 2 files changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/client/tests/kvm/kvm_utils.py
>> b/client/tests/kvm/kvm_utils.py
>> index 8531c79..a73d5d4 100644
>> --- a/client/tests/kvm/kvm_utils.py
>> +++ b/client/tests/kvm/kvm_utils.py
>> @@ -866,24 +866,21 @@ def run_tests(test_list, job):
>> if dependencies_satisfied:
>> test_iterations = int(dict.get("iterations", 1))
>> test_tag = dict.get("shortname")
>> - # Setting up kvm_stat profiling during test execution.
>> - # We don't need kvm_stat profiling on the build tests.
>> - if dict.get("run_kvm_stat") == "yes":
>> - profile = True
>> - else:
>> - # None because it's the default value on the base_test class
>> - # and the value None is specifically checked there.
>> - profile = None
>> + # Setting up profilers during test execution.
>> + profilers = dict.get("profilers")
>> + if profilers is not None:
>
> I think it's nicer and shorter to say "if profilers" instead of
> "if profilers is not None".
> Better yet, use 'profilers = dict.get("profilers", "")' so that if
> profilers isn't defined, or if the user said 'profilers = ""', you can
> still call profilers.split(), i.e.:
>
> profilers = dict.get("profilers", "")
> for profiler in profilers.split():
> job.profilers.add(profiler)
>
> and then you don't need the 'if'.
> This is also relevant to the job.profilers.delete() code below.
>
>> + for profiler in profilers.split():
>> + job.profilers.add(profiler)
>>
>> - if profile:
>> - job.profilers.add('kvm_stat')
>> # We need only one execution, profiled, hence we're passing
>> # the profile_only parameter to job.run_test().
>> current_status = job.run_test("kvm", params=dict, tag=test_tag,
>> iterations=test_iterations,
>> - profile_only=profile)
>> - if profile:
>> - job.profilers.delete('kvm_stat')
>> + profile_only= profilers is not
>> None)
>
> AFAIK, profile_only needs to be either True or None (Lucas, please correct
> me if I'm wrong).
You're right, Michael, the test class code checks specifically for
None. I have just reviewed the 2nd version of the patch and it looks
good. Applied:
http://autotest.kernel.org/changeset/4365
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
