<div dir="ltr">From my reading of the current summary code it doesn't handle cases of malicious tests using shell expansion. <div> </div><div>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()</div> <div> </div><div style>Otherwise I'll make the changes.</div></div><div class="gmail_extra"> <div class="gmail_quote">On Sat, May 25, 2013 at 3:00 AM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On 05/17/2013 09:31 AM, Dylan Baker wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote> </div> 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.<div><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Signed-off-by: Dylan Baker <<a href="mailto:baker.dylan.c@gmail.com" target="_blank">baker.dylan.c@gmail.com</a>> --- 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 </blockquote> </div></div> 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"<div class="im"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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, <a href="http://each.name" target="_blank">each.name</a>)) + # Create an index file for each test run file = open(path.join(destination, <a href="http://each.name" target="_blank">each.name</a>, "index.html"), 'w') file.write(testindex.render(name = <a href="http://each.name" target="_blank">each.name</a>, 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! </blockquote> </div> This English is very broken. I'm not sure it needs explaining either.<div class="im"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + tPath = path.join(destination, <a href="http://each.name" target="_blank">each.name</a>, 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) </blockquote> </div> If you want, you could use checkDir. But this is also fine. "ensure" (typo)<div class="im"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + file = open(path.join(destination, <a href="http://each.name" target="_blank">each.name</a>, - 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() </blockquote> </div> I'd probably use "with open(...) as file:" syntax here, but that's just me...it's your call.<div class="HOEnZb"><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> # 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 @@ Status: ${status} Result: ${status} </div> - <a href="../index.html">Back to summary</a> + <a href="${index}">Back to summary</a> <h2>Details</h2> <table> <tr> @@ -42,6 +42,6 @@ </td> </tr> </table> - <a href="../index.html">Back to summary</a> + <a href="${index}">Back to summary</a> </body> </html> </blockquote> </div></div></blockquote></div> </div>