diff mbox

[XTF,3/3] xtf-runner: regularise runner exit code

Message ID 1469115891-2269-4-git-send-email-wei.liu2@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Liu July 21, 2016, 3:44 p.m. UTC
Report the first "ERROR" and "FAILURE" if found, otherwise report "SKIP"
if found. Eventually if everything is ok the exit code will be 0.

See runner code for numeric exit code space.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xtf-runner | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Andrew Cooper July 21, 2016, 7:04 p.m. UTC | #1
On 21/07/2016 16:44, Wei Liu wrote:
> Report the first "ERROR" and "FAILURE" if found, otherwise report "SKIP"
> if found. Eventually if everything is ok the exit code will be 0.
>
> See runner code for numeric exit code space.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  xtf-runner | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/xtf-runner b/xtf-runner
> index 17ce933..ebe5c27 100755
> --- a/xtf-runner
> +++ b/xtf-runner
> @@ -249,17 +249,23 @@ def run_tests(args):
>      if not len(tests):
>          raise RunnerError("No tests to run")
>  
> -    rc = 0
> +    rc = exit_code('SUCCESS')

This logic would be easier to express if you use the indices of
all_results as a measure of severity.

e.g.

rc = all_results.index('SUCCESS')

>      results = []
>  
>      for test in tests:
>  
>          res = run_test(test)
> -        if res != "SUCCESS":
> -            rc = 1
> +        if res in ("ERROR", "FAILURE") and rc == exit_code('SUCCESS'):
> +            rc = exit_code(res)

res_idx = all_results.index(res)
if res_idx > rc:
    rc = res_idx

>  
>          results.append(res)
>  
> +    if rc == exit_code('SUCCESS'):
> +        for res in results:
> +            if res == 'SKIP':
> +                rc = exit_code('SKIP')
> +                break
> +
>      print "\nCombined test results:"
>  
>      for test, res in zip(tests, results):

And down at the end here:

return exit_code(all_results[rc])

Finally, please update the epilog text in main() to enumerate the exit
codes.

~Andrew
Wei Liu July 22, 2016, 9:29 a.m. UTC | #2
On Thu, Jul 21, 2016 at 08:04:59PM +0100, Andrew Cooper wrote:
> On 21/07/2016 16:44, Wei Liu wrote:
> > Report the first "ERROR" and "FAILURE" if found, otherwise report "SKIP"
> > if found. Eventually if everything is ok the exit code will be 0.
> >
> > See runner code for numeric exit code space.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  xtf-runner | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/xtf-runner b/xtf-runner
> > index 17ce933..ebe5c27 100755
> > --- a/xtf-runner
> > +++ b/xtf-runner
> > @@ -249,17 +249,23 @@ def run_tests(args):
> >      if not len(tests):
> >          raise RunnerError("No tests to run")
> >  
> > -    rc = 0
> > +    rc = exit_code('SUCCESS')
> 
> This logic would be easier to express if you use the indices of
> all_results as a measure of severity.
> 
> e.g.
> 
> rc = all_results.index('SUCCESS')
> 
> >      results = []
> >  
> >      for test in tests:
> >  
> >          res = run_test(test)
> > -        if res != "SUCCESS":
> > -            rc = 1
> > +        if res in ("ERROR", "FAILURE") and rc == exit_code('SUCCESS'):
> > +            rc = exit_code(res)
> 
> res_idx = all_results.index(res)
> if res_idx > rc:
>     rc = res_idx

I intended to report the first "error" or "failure" encountered.

This would cause a FAILURE to overwrite previous ERROR result. Is that
what you want? 

Wei.
Andrew Cooper July 22, 2016, 9:35 a.m. UTC | #3
On 22/07/16 10:29, Wei Liu wrote:

> On Thu, Jul 21, 2016 at 08:04:59PM +0100, Andrew Cooper wrote:
>> On 21/07/2016 16:44, Wei Liu wrote:
>>> Report the first "ERROR" and "FAILURE" if found, otherwise report "SKIP"
>>> if found. Eventually if everything is ok the exit code will be 0.
>>>
>>> See runner code for numeric exit code space.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>>   xtf-runner | 12 +++++++++---
>>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xtf-runner b/xtf-runner
>>> index 17ce933..ebe5c27 100755
>>> --- a/xtf-runner
>>> +++ b/xtf-runner
>>> @@ -249,17 +249,23 @@ def run_tests(args):
>>>       if not len(tests):
>>>           raise RunnerError("No tests to run")
>>>   
>>> -    rc = 0
>>> +    rc = exit_code('SUCCESS')
>> This logic would be easier to express if you use the indices of
>> all_results as a measure of severity.
>>
>> e.g.
>>
>> rc = all_results.index('SUCCESS')
>>
>>>       results = []
>>>   
>>>       for test in tests:
>>>   
>>>           res = run_test(test)
>>> -        if res != "SUCCESS":
>>> -            rc = 1
>>> +        if res in ("ERROR", "FAILURE") and rc == exit_code('SUCCESS'):
>>> +            rc = exit_code(res)
>> res_idx = all_results.index(res)
>> if res_idx > rc:
>>      rc = res_idx
> I intended to report the first "error" or "failure" encountered.
>
> This would cause a FAILURE to overwrite previous ERROR result. Is that
> what you want?

When running more than one test, the overall result should be the most 
severe.  So yes, a subsequent FAILURE should override an ERROR.

~Andrew
Ian Jackson July 25, 2016, 11:25 a.m. UTC | #4
Andrew Cooper writes ("Re: [PATCH XTF 3/3] xtf-runner: regularise runner exit code"):
> On 22/07/16 10:29, Wei Liu wrote:
> > This would cause a FAILURE to overwrite previous ERROR result. Is that
> > what you want?
> 
> When running more than one test, the overall result should be the most 
> severe.  So yes, a subsequent FAILURE should override an ERROR.

"Error" is surely a more severe problem than "Failure".
In particular, Error might mask a failure.

Ian.
Andrew Cooper July 25, 2016, 12:09 p.m. UTC | #5
On 25/07/16 12:25, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH XTF 3/3] xtf-runner: regularise runner exit code"):
>> On 22/07/16 10:29, Wei Liu wrote:
>>> This would cause a FAILURE to overwrite previous ERROR result. Is that
>>> what you want?
>> When running more than one test, the overall result should be the most 
>> severe.  So yes, a subsequent FAILURE should override an ERROR.
> "Error" is surely a more severe problem than "Failure".

No.  Error means "something unexpected went wrong", while Failure is
"the test worked, and identified that the area under test is defective".

Error is typically a test case bug or infrastructure issue.

> In particular, Error might mask a failure.

Indeed it could.  Either should cause a prompt investigation and effort
to resolve the issues, but one must be more severe than the other and
this is the way things are specified.

~Andrew
Ian Jackson July 25, 2016, 5:05 p.m. UTC | #6
Andrew Cooper writes ("Re: [PATCH XTF 3/3] xtf-runner: regularise runner exit code"):
> On 25/07/16 12:25, Ian Jackson wrote:
> > In particular, Error might mask a failure.
> 
> Indeed it could.  Either should cause a prompt investigation and effort
> to resolve the issues, but one must be more severe than the other and
> this is the way things are specified.

Is this really how XenRT expects things to be explained to it ?

This seems to be a very unusual approach.  Normally, in any program,
the unbounded class of errors will be always reported in the exit
status.

If we are running xtf inside osstest, I trust we aren't relying on
this aggregated exit status ?

Ian.
Wei Liu July 26, 2016, 8:57 a.m. UTC | #7
on Mon, Jul 25, 2016 at 06:05:21PM +0100, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH XTF 3/3] xtf-runner: regularise runner exit code"):
> > On 25/07/16 12:25, Ian Jackson wrote:
> > > In particular, Error might mask a failure.
> > 
> > Indeed it could.  Either should cause a prompt investigation and effort
> > to resolve the issues, but one must be more severe than the other and
> > this is the way things are specified.
> 
> Is this really how XenRT expects things to be explained to it ?
> 
> This seems to be a very unusual approach.  Normally, in any program,
> the unbounded class of errors will be always reported in the exit
> status.
> 
> If we are running xtf inside osstest, I trust we aren't relying on
> this aggregated exit status ?
> 

No, we don't rely on aggregated exit status.

Wei.

> Ian.
diff mbox

Patch

diff --git a/xtf-runner b/xtf-runner
index 17ce933..ebe5c27 100755
--- a/xtf-runner
+++ b/xtf-runner
@@ -249,17 +249,23 @@  def run_tests(args):
     if not len(tests):
         raise RunnerError("No tests to run")
 
-    rc = 0
+    rc = exit_code('SUCCESS')
     results = []
 
     for test in tests:
 
         res = run_test(test)
-        if res != "SUCCESS":
-            rc = 1
+        if res in ("ERROR", "FAILURE") and rc == exit_code('SUCCESS'):
+            rc = exit_code(res)
 
         results.append(res)
 
+    if rc == exit_code('SUCCESS'):
+        for res in results:
+            if res == 'SKIP':
+                rc = exit_code('SKIP')
+                break
+
     print "\nCombined test results:"
 
     for test, res in zip(tests, results):