[Piglit] [PATCH v2 07/11] summary: Build subdirectories for subtest result pages
Kenneth Graunke
kenneth at whitecape.org
Sat May 25 03:00:05 PDT 2013
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>
>
More information about the Piglit
mailing list