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

[Dylan Baker]

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 at pnwbakers.com 
> > <mailto:dylan at pnwbakers.com>> wrote:
> > 
> >     Quoting Marek Ol\u0161ák (2018-05-02 13:32:43)
> >      > From: Nicolai Hähnle <nicolai.haehnle at amd.com
> >     <mailto:nicolai.haehnle at 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
> > 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20180503/9e9434cc/attachment.sig>