[Piglit] [PATCH v2 07/11] summary: Build subdirectories for subtest result pages
Dylan Baker
baker.dylan.c at gmail.com
Sun May 26 01:20:01 PDT 2013
>From my reading of the current summary code it doesn't handle cases of
malicious tests using shell expansion.
On the other hand I'm certain that my code does not do *any* expansion, so
you would end up with folders named
'/home/foo/tests/~shaders/$HOME/blah/blah/blah.html', since python doesn't
expand *anything* implicitly, and I have not used os.path.expandvars() or
os.path.expanduser()
Otherwise I'll make the changes.
On Sat, May 25, 2013 at 3:00 AM, Kenneth Graunke <kenneth at whitecape.org>wrote:
> On 05/17/2013 09:31 AM, Dylan Baker wrote:
>
>> With 10,000+ tests all living in the same folder it can be hard to
>> identify a single file, especially when trying to work with the HTML
>> generation itself. This patch changes the behavior so that each test
>> has a directory tree for the group results with tests under it (ie
>> spec/GLSL 1.0/foo/bar/test.html)
>>
>> A side effect of this is that the group names of tests don't have to be
>> altered to create a valid name (since the slashes in the group names
>> just become the slashes in the folder structure) and this was the single
>> most expensive operation of HTML generation (about 1/3 of the total
>> time). This is a tidy little speedup.
>>
>
> I'm nervous about skipping sanitizing of output filenames entirely. It
> seems kind of dangerous. For example, I could (accidentally or
> maliciously) create a file named tests/shaders/~.shader_test or
> tests/shaders/$HOME.frag and it'll automatically create tests of similar
> names. I suppose since '/' can't be part of a filename, it might work out
> fine, but I'd rather not have to think about it...
>
> Regarding the speed, as I mentioned earlier, I think your implementation
> of sanitizePath could be a lot more efficient. tr/translate would be much
> faster, for example.
>
> You could probably also just sanitize it once (instead of twice) and pass
> the path name around. Dunno.
>
> I'm not opposed to putting things in subdirectories.
>
>
> Signed-off-by: Dylan Baker <baker.dylan.c at gmail.com>
>> ---
>> framework/summary.py | 76 ++++++++++++++++--------------**
>> ----------------
>> templates/test_result.mako | 6 ++--
>> 2 files changed, 30 insertions(+), 52 deletions(-)
>>
>> diff --git a/framework/summary.py b/framework/summary.py
>> index 867dbad..9f2a924 100644
>> --- a/framework/summary.py
>> +++ b/framework/summary.py
>> @@ -472,8 +472,7 @@ class BuildHTMLIndex(list):
>> """
>> self.append({'type': 'testResult',
>> 'class': text,
>> - 'href': path.join(self._sanitizePath(**group),
>> - self._sanitizePath(href +
>> ".html")),
>> + 'href': path.join(group, href + ".html"),
>> 'text': text})
>>
>> def _subtestResult(self, group, text):
>> @@ -489,29 +488,6 @@ class BuildHTMLIndex(list):
>> 'class': text,
>> 'text': text})
>>
>> - # This will be removed from here in a later patch
>> - def _sanitizePath(self, path):
>> - """
>> - Private: convert the testname to only contain valid characters
>> for
>> - a path
>> -
>> - This attempts to faithfully reconstruct the output of
>> - pigli-summary-html.**testPathToHtmlFilename, but without the
>> use of
>> - lambda. The advantage to not using lambda is readability, and the
>> - desire to make future changes to the way that sub-tests are
>> written
>> - """
>> - valid_chars = frozenset("_-.%s%s" % (string.ascii_letters,
>> - string.digits))
>> - out = []
>> - for c in path:
>> - if c in valid_chars:
>> - out.append(c)
>> - elif c == "/":
>> - out.append("__")
>> - else:
>> - out.append("_")
>> - return ''.join(out)
>> -
>>
>> class NewSummary:
>> """
>> @@ -635,10 +611,17 @@ class NewSummary:
>> output_encoding = "utf-8",
>> module_directory = ".makotmp")
>>
>> + # Although these are only referenced once, that refernce is in a
>> loop
>> + # and they are less expensive to find once and store than find
>> + # 10,000+ times
>>
>
> This is kind of a "You don't say" moment - not doing things in a loop is
> of course faster. I don't think you need to explain it. If you'd like to
> keep the comment, there's a typo: "reference"
>
>
> + resultCss = path.join(destination, "result.css")
>> + index = path.join(destination, "index.html")
>> +
>> # Iterate across the tests creating the various test specific
>> files
>> for each in self.results:
>> os.mkdir(path.join(**destination, each.name))
>>
>> + # Create an index file for each test run
>> file = open(path.join(destination, each.name,
>> "index.html"), 'w')
>> file.write(testindex.render(**name = each.name,
>> time = each.time,
>> @@ -649,15 +632,32 @@ class NewSummary:
>>
>> # Then build the individual test results
>> for key, value in each.tests.iteritems():
>> +
>> + # This is also improves performance, by cutting the
>> number of
>> + # times path.join and path.dirname is by 3, that's
>> 20,000+
>> + # times!
>>
>
> This English is very broken. I'm not sure it needs explaining either.
>
>
> + tPath = path.join(destination, each.name,
>> path.dirname(key))
>> +
>> + # os.makedirs is very annoying, it throws an OSError if
>> the
>> + # path requested already exists, so do this check to
>> esnure
>> + # that it doesn't
>> + if not path.exists(tPath):
>> + os.makedirs(tPath)
>>
>
> If you want, you could use checkDir. But this is also fine.
> "ensure" (typo)
>
>
> +
>> file = open(path.join(destination,
>> each.name,
>> - self._sanitizePath(key +
>> ".html")), 'w')
>> + key + ".html"),
>> + 'w')
>> file.write(testfile.render(**testname = key,
>> status = value['result'],
>> returncode =
>> value['returncode'],
>> time = value['time'],
>> info = value['info'],
>> - command =
>> value['command']))
>> + command = value['command'],
>> + css =
>> path.relpath(resultCss,
>> +
>> tPath),
>> + index = index))
>> +
>> file.close()
>>
>
> I'd probably use "with open(...) as file:" syntax here, but that's just
> me...it's your call.
>
>
>
>> # Finally build the root html files: index, regressions, etc
>> @@ -811,25 +811,3 @@ class NewSummary:
>> assert(len(currentStatus) == 1)
>>
>> return counts, status
>> -
>> - def _sanitizePath(self, path):
>> - """
>> - Private: convert the testname to only contain valid characters
>> for
>> - a path
>> -
>> - This attempts to faithfully reconstruct the output of
>> - pigli-summary-html.**testPathToHtmlFilename, but without the
>> use of
>> - lambda. The advantage to not using lambda is readability, and the
>> - desire to make future changes to the way that sub-tests are
>> written
>> - """
>> - valid_chars = frozenset("_-.%s%s" % (string.ascii_letters,
>> - string.digits))
>> - out = []
>> - for c in path:
>> - if c in valid_chars:
>> - out.append(c)
>> - elif c == "/":
>> - out.append("__")
>> - else:
>> - out.append("_")
>> - return ''.join(out)
>> diff --git a/templates/test_result.mako b/templates/test_result.mako
>> index 1f944cb..7de0af7 100644
>> --- a/templates/test_result.mako
>> +++ b/templates/test_result.mako
>> @@ -5,7 +5,7 @@
>> <head>
>> <meta http-equiv="Content-Type" content="text/html;
>> charset=UTF-8" />
>> <title>${testname} - Details</title>
>> - <link rel="stylesheet" href="../result.css"
>> type="text/css" />
>> + <link rel="stylesheet" href="${css}" type="text/css" />
>> </head>
>> <body>
>> <h1>Results for ${testname}</h1>
>> @@ -14,7 +14,7 @@
>> <p><b>Status:</b> ${status}</p>
>> <p><b>Result:</b> ${status}</p>
>> </div>
>> - <p><a href="../index.html">Back to summary</a></p>
>> + <p><a href="${index}">Back to summary</a></p>
>> <h2>Details</h2>
>> <table>
>> <tr>
>> @@ -42,6 +42,6 @@
>> </td>
>> </tr>
>> </table>
>> - <p><a href="../index.html">Back to summary</a></p>
>> + <p><a href="${index}">Back to summary</a></p>
>> </body>
>> </html>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130526/72148e10/attachment.html>
More information about the Piglit
mailing list