[1/5] framework/html: guard against errors writing individual test results

Submitted by Marek Olšák on May 2, 2018, 8:32 p.m.

Details

Message ID 20180502203247.11284-1-maraeo@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Marek Olšák May 2, 2018, 8:32 p.m.
From: Nicolai Hähnle <nicolai.haehnle@amd.com>

---
 framework/summary/html_.py | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/framework/summary/html_.py b/framework/summary/html_.py
index f7fdc8576..512b42c24 100644
--- a/framework/summary/html_.py
+++ b/framework/summary/html_.py
@@ -24,20 +24,21 @@ 
 
 from __future__ import (
     absolute_import, division, print_function, unicode_literals
 )
 import errno
 import getpass
 import os
 import shutil
 import sys
 import tempfile
+import traceback
 
 import mako
 from mako.lookup import TemplateLookup
 import six
 
 # a local variable status exists, prevent accidental overloading by renaming
 # the module
 from framework import backends, exceptions, core
 
 from .common import Results, escape_filename, escape_pathname
@@ -106,27 +107,30 @@  def _make_testrun_info(results, destination, exclude=None):
 
         # Then build the individual test results
         for key, value in six.iteritems(each.tests):
             html_path = os.path.join(destination, name,
                                      escape_filename(key + ".html"))
             temp_path = os.path.dirname(html_path)
 
             if value.result not in exclude:
                 core.check_dir(temp_path)
 
-                with open(html_path, 'wb') as out:
-                    out.write(_TEMPLATES.get_template(
-                        'test_result.mako').render(
-                            testname=key,
-                            value=value,
-                            css=os.path.relpath(result_css, temp_path),
-                            index=os.path.relpath(index, temp_path)))
+                try:
+                    with open(html_path, 'wb') as out:
+                        out.write(_TEMPLATES.get_template(
+                            'test_result.mako').render(
+                                testname=key,
+                                value=value,
+                                css=os.path.relpath(result_css, temp_path),
+                                index=os.path.relpath(index, temp_path)))
+                except OSError as e:
+                    traceback.print_exc()
 
 
 def _make_comparison_pages(results, destination, exclude):
     """Create the pages of comparisons."""
     pages = frozenset(['changes', 'problems', 'skips', 'fixes',
                        'regressions', 'enabled', 'disabled'])
 
     # Index.html is a bit of a special case since there is index, all, and
     # alltests, where the other pages all use the same name. ie,
     # changes.html, changes, and page=changes.

Comments

Quoting Marek Olšák (2018-05-02 13:32:43)
> From: Nicolai Hähnle <nicolai.haehnle@amd.com>
> 
> ---
>  framework/summary/html_.py | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/framework/summary/html_.py b/framework/summary/html_.py
> index f7fdc8576..512b42c24 100644
> --- a/framework/summary/html_.py
> +++ b/framework/summary/html_.py
> @@ -24,20 +24,21 @@
>  
>  from __future__ import (
>      absolute_import, division, print_function, unicode_literals
>  )
>  import errno
>  import getpass
>  import os
>  import shutil
>  import sys
>  import tempfile
> +import traceback
>  
>  import mako
>  from mako.lookup import TemplateLookup
>  import six
>  
>  # a local variable status exists, prevent accidental overloading by renaming
>  # the module
>  from framework import backends, exceptions, core
>  
>  from .common import Results, escape_filename, escape_pathname
> @@ -106,27 +107,30 @@ def _make_testrun_info(results, destination, exclude=None):
>  
>          # Then build the individual test results
>          for key, value in six.iteritems(each.tests):
>              html_path = os.path.join(destination, name,
>                                       escape_filename(key + ".html"))
>              temp_path = os.path.dirname(html_path)
>  
>              if value.result not in exclude:
>                  core.check_dir(temp_path)
>  
> -                with open(html_path, 'wb') as out:
> -                    out.write(_TEMPLATES.get_template(
> -                        'test_result.mako').render(
> -                            testname=key,
> -                            value=value,
> -                            css=os.path.relpath(result_css, temp_path),
> -                            index=os.path.relpath(index, temp_path)))
> +                try:
> +                    with open(html_path, 'wb') as out:
> +                        out.write(_TEMPLATES.get_template(
> +                            'test_result.mako').render(
> +                                testname=key,
> +                                value=value,
> +                                css=os.path.relpath(result_css, temp_path),
> +                                index=os.path.relpath(index, temp_path)))
> +                except OSError as e:
> +                    traceback.print_exc()

This makes me really nervous. What are you trying to catch, and why is it
a good idea to print a traceback and continue?

Dylan

>  
>  
>  def _make_comparison_pages(results, destination, exclude):
>      """Create the pages of comparisons."""
>      pages = frozenset(['changes', 'problems', 'skips', 'fixes',
>                         'regressions', 'enabled', 'disabled'])
>  
>      # Index.html is a bit of a special case since there is index, all, and
>      # alltests, where the other pages all use the same name. ie,
>      # changes.html, changes, and page=changes.
> -- 
> 2.17.0
> 
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit
On Wed, May 2, 2018 at 4:51 PM, Dylan Baker <dylan@pnwbakers.com> wrote:

> Quoting Marek Olšák (2018-05-02 13:32:43)
> > From: Nicolai Hähnle <nicolai.haehnle@amd.com>
> >
> > ---
> >  framework/summary/html_.py | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/framework/summary/html_.py b/framework/summary/html_.py
> > index f7fdc8576..512b42c24 100644
> > --- a/framework/summary/html_.py
> > +++ b/framework/summary/html_.py
> > @@ -24,20 +24,21 @@
> >
> >  from __future__ import (
> >      absolute_import, division, print_function, unicode_literals
> >  )
> >  import errno
> >  import getpass
> >  import os
> >  import shutil
> >  import sys
> >  import tempfile
> > +import traceback
> >
> >  import mako
> >  from mako.lookup import TemplateLookup
> >  import six
> >
> >  # a local variable status exists, prevent accidental overloading by
> renaming
> >  # the module
> >  from framework import backends, exceptions, core
> >
> >  from .common import Results, escape_filename, escape_pathname
> > @@ -106,27 +107,30 @@ def _make_testrun_info(results, destination,
> exclude=None):
> >
> >          # Then build the individual test results
> >          for key, value in six.iteritems(each.tests):
> >              html_path = os.path.join(destination, name,
> >                                       escape_filename(key + ".html"))
> >              temp_path = os.path.dirname(html_path)
> >
> >              if value.result not in exclude:
> >                  core.check_dir(temp_path)
> >
> > -                with open(html_path, 'wb') as out:
> > -                    out.write(_TEMPLATES.get_template(
> > -                        'test_result.mako').render(
> > -                            testname=key,
> > -                            value=value,
> > -                            css=os.path.relpath(result_css, temp_path),
> > -                            index=os.path.relpath(index, temp_path)))
> > +                try:
> > +                    with open(html_path, 'wb') as out:
> > +                        out.write(_TEMPLATES.get_template(
> > +                            'test_result.mako').render(
> > +                                testname=key,
> > +                                value=value,
> > +                                css=os.path.relpath(result_css,
> temp_path),
> > +                                index=os.path.relpath(index,
> temp_path)))
> > +                except OSError as e:
> > +                    traceback.print_exc()
>
> This makes me really nervous. What are you trying to catch, and why is it
> a good idea to print a traceback and continue?
>

Nicolai, do you know the answer to this one?

Thanks,
Marek
Quoting Nicolai Hähnle (2018-05-03 01:43:52)
> On 02.05.2018 22:57, Marek Ol\u0161ák wrote:
> > On Wed, May 2, 2018 at 4:51 PM, Dylan Baker <dylan@pnwbakers.com 
> > <mailto:dylan@pnwbakers.com>> wrote:
> > 
> >     Quoting Marek Ol\u0161ák (2018-05-02 13:32:43)
> >      > From: Nicolai Hähnle <nicolai.haehnle@amd.com
> >     <mailto:nicolai.haehnle@amd.com>>
> >      >
> >      > ---
> >      >  framework/summary/html_.py | 18 +++++++++++-------
> >      >  1 file changed, 11 insertions(+), 7 deletions(-)
> >      >
> >      > diff --git a/framework/summary/html_.py b/framework/summary/html_.py
> >      > index f7fdc8576..512b42c24 100644
> >      > --- a/framework/summary/html_.py
> >      > +++ b/framework/summary/html_.py
> >      > @@ -24,20 +24,21 @@
> >      >
> >      >  from __future__ import (
> >      >      absolute_import, division, print_function, unicode_literals
> >      >  )
> >      >  import errno
> >      >  import getpass
> >      >  import os
> >      >  import shutil
> >      >  import sys
> >      >  import tempfile
> >      > +import traceback
> >      >
> >      >  import mako
> >      >  from mako.lookup import TemplateLookup
> >      >  import six
> >      >
> >      >  # a local variable status exists, prevent accidental overloading
> >     by renaming
> >      >  # the module
> >      >  from framework import backends, exceptions, core
> >      >
> >      >  from .common import Results, escape_filename, escape_pathname
> >      > @@ -106,27 +107,30 @@ def _make_testrun_info(results,
> >     destination, exclude=None):
> >      >
> >      >          # Then build the individual test results
> >      >          for key, value in six.iteritems(each.tests):
> >      >              html_path = os.path.join(destination, name,
> >      >                                       escape_filename(key + ".html"))
> >      >              temp_path = os.path.dirname(html_path)
> >      >
> >      >              if value.result not in exclude:
> >      >                  core.check_dir(temp_path)
> >      >
> >      > -                with open(html_path, 'wb') as out:
> >      > -                    out.write(_TEMPLATES.get_template(
> >      > -                        'test_result.mako').render(
> >      > -                            testname=key,
> >      > -                            value=value,
> >      > -                            css=os.path.relpath(result_css,
> >     temp_path),
> >      > -                            index=os.path.relpath(index,
> >     temp_path)))
> >      > +                try:
> >      > +                    with open(html_path, 'wb') as out:
> >      > +                        out.write(_TEMPLATES.get_template(
> >      > +                            'test_result.mako').render(
> >      > +                                testname=key,
> >      > +                                value=value,
> >      > +                                css=os.path.relpath(result_css,
> >     temp_path),
> >      > +                                index=os.path.relpath(index,
> >     temp_path)))
> >      > +                except OSError as e:
> >      > +                    traceback.print_exc()
> > 
> >     This makes me really nervous. What are you trying to catch, and why
> >     is it
> >     a good idea to print a traceback and continue?
> > 
> > 
> > Nicolai, do you know the answer to this one?
> 
> Some test cases have *really* long names. Especially if your test runs 
> also have long names and you're writing the results in a somewhat nested 
> place on the filesystem, writing can fail with a "filename too long" 
> kind of error here.
> 
> It is super annoying when the entire summary process fails because of 
> something silly like that: most of the time, you don't really need the 
> results files anyway.
> 
> I suppose it would be better to use shorter filenames, but when I looked 
> into it it wasn't entirely trivial to do that...
> 
> Cheers,
> Nicolai

Okay, that definitely seems like a good error to handle. I'd like to avoid
catching other errors that we should stop on like ENOPERM, so could we do
something like:

import errno

...

except OSError as e:
    if e.errno == errno.ENAMETOOLONG:
        print('WARN: file "{}" too long'.format(html_name))
    else:
        raise

Dylan

> 
> 
> > 
> > Thanks,
> > Marek
> > 
>