[2/2] framework: Provide more helpful error message when loading results fails

Submitted by Dylan Baker on Nov. 4, 2016, 7:49 p.m.

Details

Message ID 20161104194940.4382-2-dylan@pnwbakers.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Dylan Baker Nov. 4, 2016, 7:49 p.m.
Currently if there is a typo in a filename (which piglit uses to
determine if a particular result can be loaded) then a message about
an unsupported backend will be generated. Obviously, the more common
reason for this error is a typo or pointing at the wrong directory, not
that you've tried to load a result format piglit doesn't understand.

With that in mind this patch catches that exception in every case and
raises a PiglitFatalError, appending a message that this is probably a
typo or that you've pointed at the wrong directory.

cc: Brian Paul <brianp@vmware.com>
Signed-off-by: Dylan Baker <dylanx.c.baker@intel.com>
---
 framework/programs/summary.py | 10 +++++++++-
 framework/summary/console_.py | 10 +++++++++-
 framework/summary/html_.py    | 19 ++++++++++++++++---
 3 files changed, 34 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/framework/programs/summary.py b/framework/programs/summary.py
index e400d9a..784e084 100644
--- a/framework/programs/summary.py
+++ b/framework/programs/summary.py
@@ -186,7 +186,15 @@  def csv(input_):
                         help="JSON results file to be converted")
     args = parser.parse_args(unparsed)
 
-    testrun = backends.load(args.testResults)
+    try:
+        testrun = backends.load(args.testResults)
+    except backends.BackendError as e:
+        raise exceptions.PiglitFatalError(
+            six.text_type(e) + \
+            '\n'
+            'This might mean that you pointed at the wrong directory or a '
+            'typo in your file extension.')
+
 
     def write_results(output):
         for name, result in six.iteritems(testrun.tests):
diff --git a/framework/summary/console_.py b/framework/summary/console_.py
index e17a1d8..b8cdafb 100644
--- a/framework/summary/console_.py
+++ b/framework/summary/console_.py
@@ -30,6 +30,7 @@  import textwrap
 import six
 
 from framework import grouptools, backends
+from framework import exceptions
 from .common import Results
 
 __all__ = [
@@ -100,7 +101,14 @@  def _print_result(results, list_):
 def console(results, mode):
     """ Write summary information to the console """
     assert mode in ['summary', 'diff', 'incomplete', 'all'], mode
-    results = Results([backends.load(r) for r in results])
+    try:
+        results = Results([backends.load(r) for r in results])
+    except backends.BackendError as e:
+        raise exceptions.PiglitFatalError(
+            six.text_type(e) + \
+            '\n'
+            'This might mean that you pointed at the wrong directory or a '
+            'typo in your file extension.')
 
     # Print the name of the test and the status from each test run
     if mode == 'all':
diff --git a/framework/summary/html_.py b/framework/summary/html_.py
index f7fdc85..4983b62 100644
--- a/framework/summary/html_.py
+++ b/framework/summary/html_.py
@@ -172,7 +172,14 @@  def html(results, destination, exclude):
     heavy lifting, this method just passes it a bunch of dicts and lists
     of dicts, which mako turns into pretty HTML.
     """
-    results = Results([backends.load(i) for i in results])
+    try:
+        results = Results([backends.load(r) for r in results])
+    except backends.BackendError as e:
+        raise exceptions.PiglitFatalError(
+            six.text_type(e) + \
+            '\n'
+            'This might mean that you pointed at the wrong directory or a '
+            'typo in your file extension.')
 
     _copy_static_files(destination)
     _make_testrun_info(results, destination, exclude)
@@ -181,8 +188,14 @@  def html(results, destination, exclude):
 
 def feat(results, destination, feat_desc):
     """Produce HTML feature readiness summary."""
-
-    feat_res = FeatResults([backends.load(i) for i in results], feat_desc)
+    try:
+        feat_res = FeatResults([backends.load(i) for i in results], feat_desc)
+    except backends.BackendError as e:
+        raise exceptions.PiglitFatalError(
+            six.text_type(e) + \
+            '\n'
+            'This might mean that you pointed at the wrong directory or a '
+            'typo in your file extension.')
 
     _copy_static_files(destination)
     _make_testrun_info(feat_res, destination)

Comments

On 11/04/2016 01:49 PM, Dylan Baker wrote:
> Currently if there is a typo in a filename (which piglit uses to
> determine if a particular result can be loaded) then a message about
> an unsupported backend will be generated. Obviously, the more common
> reason for this error is a typo or pointing at the wrong directory, not
> that you've tried to load a result format piglit doesn't understand.
>
> With that in mind this patch catches that exception in every case and
> raises a PiglitFatalError, appending a message that this is probably a
> typo or that you've pointed at the wrong directory.
>
> cc: Brian Paul <brianp@vmware.com>
> Signed-off-by: Dylan Baker <dylanx.c.baker@intel.com>
> ---
>   framework/programs/summary.py | 10 +++++++++-
>   framework/summary/console_.py | 10 +++++++++-
>   framework/summary/html_.py    | 19 ++++++++++++++++---
>   3 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/framework/programs/summary.py b/framework/programs/summary.py
> index e400d9a..784e084 100644
> --- a/framework/programs/summary.py
> +++ b/framework/programs/summary.py
> @@ -186,7 +186,15 @@ def csv(input_):
>                           help="JSON results file to be converted")
>       args = parser.parse_args(unparsed)
>
> -    testrun = backends.load(args.testResults)
> +    try:
> +        testrun = backends.load(args.testResults)
> +    except backends.BackendError as e:
> +        raise exceptions.PiglitFatalError(
> +            six.text_type(e) + \
> +            '\n'
> +            'This might mean that you pointed at the wrong directory or a '
> +            'typo in your file extension.')
> +
>
>       def write_results(output):
>           for name, result in six.iteritems(testrun.tests):
> diff --git a/framework/summary/console_.py b/framework/summary/console_.py
> index e17a1d8..b8cdafb 100644
> --- a/framework/summary/console_.py
> +++ b/framework/summary/console_.py
> @@ -30,6 +30,7 @@ import textwrap
>   import six
>
>   from framework import grouptools, backends
> +from framework import exceptions
>   from .common import Results
>
>   __all__ = [
> @@ -100,7 +101,14 @@ def _print_result(results, list_):
>   def console(results, mode):
>       """ Write summary information to the console """
>       assert mode in ['summary', 'diff', 'incomplete', 'all'], mode
> -    results = Results([backends.load(r) for r in results])
> +    try:
> +        results = Results([backends.load(r) for r in results])
> +    except backends.BackendError as e:
> +        raise exceptions.PiglitFatalError(
> +            six.text_type(e) + \
> +            '\n'
> +            'This might mean that you pointed at the wrong directory or a '
> +            'typo in your file extension.')
>
>       # Print the name of the test and the status from each test run
>       if mode == 'all':
> diff --git a/framework/summary/html_.py b/framework/summary/html_.py
> index f7fdc85..4983b62 100644
> --- a/framework/summary/html_.py
> +++ b/framework/summary/html_.py
> @@ -172,7 +172,14 @@ def html(results, destination, exclude):
>       heavy lifting, this method just passes it a bunch of dicts and lists
>       of dicts, which mako turns into pretty HTML.
>       """
> -    results = Results([backends.load(i) for i in results])
> +    try:
> +        results = Results([backends.load(r) for r in results])
> +    except backends.BackendError as e:
> +        raise exceptions.PiglitFatalError(
> +            six.text_type(e) + \
> +            '\n'
> +            'This might mean that you pointed at the wrong directory or a '
> +            'typo in your file extension.')
>
>       _copy_static_files(destination)
>       _make_testrun_info(results, destination, exclude)
> @@ -181,8 +188,14 @@ def html(results, destination, exclude):
>
>   def feat(results, destination, feat_desc):
>       """Produce HTML feature readiness summary."""
> -
> -    feat_res = FeatResults([backends.load(i) for i in results], feat_desc)
> +    try:
> +        feat_res = FeatResults([backends.load(i) for i in results], feat_desc)
> +    except backends.BackendError as e:
> +        raise exceptions.PiglitFatalError(
> +            six.text_type(e) + \
> +            '\n'
> +            'This might mean that you pointed at the wrong directory or a '
> +            'typo in your file extension.')
>
>       _copy_static_files(destination)
>       _make_testrun_info(feat_res, destination)
>

This works, but
1. seems like a lot of duplicated code.
2. How about a simpler error message like: "Unable to open '%s'"

Thanks.

-Brian
Quoting Brian Paul (2016-11-04 13:18:21)
> On 11/04/2016 01:49 PM, Dylan Baker wrote:
[snip]
> 
> This works, but
> 1. seems like a lot of duplicated code.
> 2. How about a simpler error message like: "Unable to open '%s'"

But that's not the problem. The function that it's failing in is responsible for
deciding which backend to send the file to to be opened, not opening the file
itself. That message would make me think that the file was corrupt, or invalid
JSON, not that I'd typoed the file extension or pointed at the wrong directory.

What about: "unknown file extension '%s'"?

Dylan
On 11/04/2016 02:49 PM, Dylan Baker wrote:
> Quoting Brian Paul (2016-11-04 13:18:21)
>> On 11/04/2016 01:49 PM, Dylan Baker wrote:
> [snip]
>>
>> This works, but
>> 1. seems like a lot of duplicated code.
>> 2. How about a simpler error message like: "Unable to open '%s'"
>
> But that's not the problem. The function that it's failing in is responsible for
> deciding which backend to send the file to to be opened, not opening the file
> itself. That message would make me think that the file was corrupt, or invalid
> JSON, not that I'd typoed the file extension or pointed at the wrong directory.
>
> What about: "unknown file extension '%s'"?

Ah, OK.  When I try "nonexistant.json.bz2" I get "No such file or 
directory".

I guess I'd like to see that message if the input file doesn't exist, 
regardless of extension.  But if the file does exist and its type isn't 
recognized, report something like "unknown file type/extension".

Does that seem reasonable?  That's kind of what gcc does.

-Brian
Quoting Brian Paul (2016-11-04 16:02:38)
> On 11/04/2016 02:49 PM, Dylan Baker wrote:
> > Quoting Brian Paul (2016-11-04 13:18:21)
> >> On 11/04/2016 01:49 PM, Dylan Baker wrote:
> > [snip]
> >>
> >> This works, but
> >> 1. seems like a lot of duplicated code.
> >> 2. How about a simpler error message like: "Unable to open '%s'"
> >
> > But that's not the problem. The function that it's failing in is responsible for
> > deciding which backend to send the file to to be opened, not opening the file
> > itself. That message would make me think that the file was corrupt, or invalid
> > JSON, not that I'd typoed the file extension or pointed at the wrong directory.
> >
> > What about: "unknown file extension '%s'"?
> 
> Ah, OK.  When I try "nonexistant.json.bz2" I get "No such file or 
> directory".
> 
> I guess I'd like to see that message if the input file doesn't exist, 
> regardless of extension.  But if the file does exist and its type isn't 
> recognized, report something like "unknown file type/extension".
> 
> Does that seem reasonable?  That's kind of what gcc does.
> 
> -Brian
> 

Yup that sounds reasonable, I'll send a v2.

Dylan