[webkit-reviews] review requested: [Bug 36195] Add layout test flakiness (diagnostics) dashboard to TestResultServer appengine : [Attachment 51545] Patch per comment #4

[bugzilla-daemon at webkit.org]

Victor Wang <victorw at chromium.org> has asked  for review:
Bug 36195: Add layout test flakiness (diagnostics) dashboard to
TestResultServer appengine
https://bugs.webkit.org/show_bug.cgi?id=36195

Attachment 51545: Patch per comment #4
https://bugs.webkit.org/attachment.cgi?id=51545&action=review

------- Additional Comments from Victor Wang <victorw at chromium.org>
Thanks for your good inputs, see my comments inline.

(In reply to comment #4)
> (From update of attachment 51466 [details])
> + self.response.out.write(files[0].data)
> 
> Mime type?  Character set?
done

> 
> + self.response.out.write("\n".join(errors))
> 
> XSS?
ya, thanks for spotting this. Actually no need to write errors to response.out.
The error messages is set in response.status. Removed.


> 
> + DashboardFile.delete_file(file)
> 
> CSRF?  Public delete file URL?
Added code to only allow AE admins to do this and only shows the "delete" links
in file list for admin account.

> 
> + http://src.chromium.org/
> 
> Wrong project?
For now, the flakiness dashboard source code is still in chromium tree as it
has code that are shared with other chromium tests. I think we will either move
them upstream or have an upstream version so that non-chromium port can be
benefit from this tool.


> 
> + url = SVN_PATH_DASHBOARD + name
> 
> URL encoding?
done

> 
> Tests???
There is a JAVA version of AE test framework but no python version yet
(confirmed with Google AE team). I prefer waiting until a python version is
released instead of making my own version for AE testing like datastore,
blobstore, handler etc, thoughts? I could file a bug for adding test for
AppEngine once it is ready if needed...