[Piglit] [PATCH] CL: don't reporting failed cases as PASS

[Xing, Homer]

Thanks for reviewing and pointing out the problems. I have refined the patch, and sent it out.
The new version is http://lists.freedesktop.org/archives/piglit/2013-August/006799.html

From: Dylan Baker [mailto:baker.dylan.c at gmail.com]
Sent: Saturday, August 17, 2013 1:15 AM
To: Tom Stellard
Cc: Xing, Homer; piglit at lists.freedesktop.org
Subject: Re: [Piglit] [PATCH] CL: don't reporting failed cases as PASS


First, this is not a CL specific problem. This is part of the core piglit framework. Please only use CL: in the header if fixing a CL test or CL specific framework

On Fri, Aug 16, 2013 at 6:38 AM, Tom Stellard <tom at stellard.net<mailto:tom at stellard.net>> wrote:
On Fri, Aug 16, 2013 at 11:28:23AM +0800, Homer Hsing wrote:
> If in multiple subtests, first failed, last passed, then subtests
> was looked as "passed". Because "interpretResult" function overwrote
> previous results. See framework/exectest.py, interpretResult():
>
>            if piglit.startswith('subtest'):
>                if not 'subtest' in results:
>                    results['subtest'] = {}
> HERE->           results['subtest'].update(eval(piglit[7:]))
>
> For instance, if test result was
>  ["subtest {'clz char1' : 'fail'}", "subset {'clz char1' : 'pass'}"]
> then results['subtest'] would be 'pass'.
>
> "doRun" function also overwrote old results. See framework/core.py,
>
>          if 'subtest' in result and len(result['subtest'].keys()) > 1:
>              for test in result['subtest'].keys():
> HERE->           result['result'] = result['subtest'][test]
>
> Thus if some subtests failed, but others passed, then final result might
> be "PASS".
>
> This patch fixes the bug.
>
> Signed-off-by: Homer Hsing <homer.xing at intel.com<mailto:homer.xing at intel.com>>
Reviewed-by: Tom Stellard <thomas.stellard at amd.com<mailto:thomas.stellard at amd.com>>

> ---
>  framework/core.py     | 3 ++-
>  framework/exectest.py | 7 ++++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/framework/core.py b/framework/core.py
> index 108b517..ec62231 100644
> --- a/framework/core.py
> +++ b/framework/core.py
> @@ -471,7 +471,8 @@ class Test:
>
>              if 'subtest' in result and len(result['subtest'].keys()) > 1:
>                  for test in result['subtest'].keys():
> -                    result['result'] = result['subtest'][test]
> +                    if 'result' not in result or result['result'] == 'pass':
> +                        result['result'] = result['subtest'][test] # safe update

This isn't really fixing the problem. For example if we have the follow subtest results in order: pass, warn, fail, crash; then the final result will be warn, even though crash is the most serious status, and the one that should be displayed. It also means that if a subtest has status 'skip', that will overwrite 'pass', which is also not a desireable behavior.

>                      json_writer.write_dict_item(path + '/' + test, result)
>              else:
>                  json_writer.write_dict_item(path, result)
> diff --git a/framework/exectest.py b/framework/exectest.py
> index 6ee550c..37e70bd 100644
> --- a/framework/exectest.py
> +++ b/framework/exectest.py
> @@ -218,6 +218,11 @@ class PlainExecTest(ExecTest):
>          self.command[0] = testBinDir + self.command[0]
>
>      def interpretResult(self, out, returncode, results):
> +        def safe_update(dest, src):
> +            for k in src:
> +                if k not in dest or dest[k] == 'pass':
> +                    dest[k] = src[k]
> +

This has the same problem as outlined above

>          outlines = out.split('\n')
>          outpiglit = map(lambda s: s[7:],
>                          filter(lambda s: s.startswith('PIGLIT:'), outlines))
> @@ -228,7 +233,7 @@ class PlainExecTest(ExecTest):
>                      if piglit.startswith('subtest'):
>                          if not 'subtest' in results:
>                              results['subtest'] = {}
> -                        results['subtest'].update(eval(piglit[7:]))
> +                        safe_update(results['subtest'], eval(piglit[7:]))
>                      else:
>                          results.update(eval(piglit))
>                  out = '\n'.join(filter(lambda s: not s.startswith('PIGLIT:'),
> --
> 1.8.1.2
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org<mailto:Piglit at lists.freedesktop.org>
> http://lists.freedesktop.org/mailman/listinfo/piglit
_______________________________________________
Piglit mailing list
Piglit at lists.freedesktop.org<mailto:Piglit at lists.freedesktop.org>
http://lists.freedesktop.org/mailman/listinfo/piglit

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130819/c46eff10/attachment.html>