On Thu, Apr 26, 2012 at 07:34, Iustin Pop <[email protected]> wrote:
> On Thu, Apr 26, 2012 at 12:07:02AM +0200, Michael Hanselmann wrote:
>> This script can be used to check if an instance is running or stopped at
>> various points during a QA run. Environment variables are used to pass
>> the most essential information.
>
> Not sure if this is a good approach. You rely on manually annotating the
> generic instance function with sprinkled CheckInstance tests.
>
> I think we should not do such manual annotation and instead annotate the
> instance tests themselves with a running yes/no flag after it. In the
> sense that after each test, we should automatically check whether the
> instance is running or not.
It seems to me that each test should check that the instance is in the
correct state, without any flag (each test should know what the
correct state is). Also, a call like "RunTest(qa_utils.CheckInstance,
instance)" is not very explicit about what it checks; it is clearer
when you have two forms of the test (e.g., CheckInstanceIsRunning and
CheckInstanceIsStopped) or if you force the parameter running to be
specified:
def CheckInstance(instance, running=None):
assert running is not None
Btw, shouldn't the docstring for CheckInstance describe what the
function does and what the parameters are?
Bernardo
>> ---
>> qa/ganeti-qa.py | 36 ++++++++++++++++++++++++++++++++----
>> qa/qa-sample.json | 3 +++
>> qa/qa_config.py | 19 +++++++++++++++++++
>> qa/qa_utils.py | 35 +++++++++++++++++++++++++++++++++++
>> 4 files changed, 89 insertions(+), 4 deletions(-)
>>
>> diff --git a/qa/ganeti-qa.py b/qa/ganeti-qa.py
>> index d35ebd6..01a2b1e 100755
>> --- a/qa/ganeti-qa.py
>> +++ b/qa/ganeti-qa.py
>> @@ -72,7 +72,7 @@ def _DescriptionOf(fn):
>> return desc.rstrip(".")
>>
>>
>> -def RunTest(fn, *args):
>> +def RunTest(fn, *args, **kwargs):
>> """Runs a test after printing a header.
>>
>> """
>> @@ -85,7 +85,7 @@ def RunTest(fn, *args):
>> print _FormatHeader("%s start %s" % (tstart, desc))
>>
>> try:
>> - retval = fn(*args)
>> + retval = fn(*args, **kwargs)
>> return retval
>> finally:
>> tstop = datetime.datetime.now()
>> @@ -93,7 +93,7 @@ def RunTest(fn, *args):
>> print _FormatHeader("%s time=%s %s" % (tstop, tdelta, desc))
>>
>>
>> -def RunTestIf(testnames, fn, *args):
>> +def RunTestIf(testnames, fn, *args, **kwargs):
>> """Runs a test conditionally.
>>
>> @param testnames: either a single test name in the configuration
>> @@ -101,7 +101,7 @@ def RunTestIf(testnames, fn, *args):
>>
>> """
>> if qa_config.TestEnabled(testnames):
>> - RunTest(fn, *args)
>> + RunTest(fn, *args, **kwargs)
>> else:
>> tstart = datetime.datetime.now()
>> desc = _DescriptionOf(fn)
>
> Up to here LGTM, these can go as a separate patch.
>
> thanks,
> iustin
