Bug 36195 - Add layout test flakiness (diagnostics) dashboard to TestResultServer appengine
Summary: Add layout test flakiness (diagnostics) dashboard to TestResultServer appengine
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Victor Wang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-16 14:52 PDT by Victor Wang
Modified: 2010-03-26 11:14 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (22.08 KB, patch)
2010-03-16 15:34 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Proposed patch - remove extra empty line (20.58 KB, patch)
2010-03-16 15:39 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Proposed Patch (21.19 KB, patch)
2010-03-23 16:10 PDT, Victor Wang
abarth: review-
Details | Formatted Diff | Diff
Patch per comment #4 (23.46 KB, patch)
2010-03-24 14:45 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Correct app name and remove a few unused lines. (23.23 KB, patch)
2010-03-24 14:49 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Remove CRs in index.yaml (24.72 KB, patch)
2010-03-25 17:40 PDT, Victor Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Wang 2010-03-16 14:52:01 PDT
Host layout test flakiness dashboard in TestResultServer appengine. Flakiness dashboard is a tool (used by chromium) to monitor layout test status and help diagnose layout test regressions.
Comment 1 Victor Wang 2010-03-16 15:34:43 PDT
Created attachment 50845 [details]
Proposed patch
Comment 2 Victor Wang 2010-03-16 15:39:32 PDT
Created attachment 50846 [details]
Proposed patch - remove extra empty line
Comment 3 Victor Wang 2010-03-23 16:10:15 PDT
Created attachment 51466 [details]
Proposed Patch
Comment 4 Adam Barth 2010-03-24 00:41:18 PDT
Comment on attachment 51466 [details]
Proposed Patch

+ self.response.out.write(files[0].data)

Mime type?  Character set?

+ self.response.out.write("\n".join(errors))

XSS?

+ DashboardFile.delete_file(file)

CSRF?  Public delete file URL?

+ http://src.chromium.org/

Wrong project?

+ url = SVN_PATH_DASHBOARD + name

URL encoding?

Tests???
Comment 5 Victor Wang 2010-03-24 14:45:54 PDT
Created attachment 51545 [details]
Patch per comment #4

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...
Comment 6 Victor Wang 2010-03-24 14:49:34 PDT
Created attachment 51547 [details]
Correct app name and remove a few unused lines.
Comment 7 Adam Barth 2010-03-25 16:06:09 PDT
Comment on attachment 51547 [details]
Correct app name and remove a few unused lines.

Thanks for addressing my feedback.  I'm not super excited about the dependency on the chromium source server, but I don't see a better solution in the meantime.

We have another AppEngine app in QueueStatusServer, which also doesn't have any test coverage.  I'm deathly afraid of touching it for that reason.  A testing framework would certainly be nice.  :)
Comment 8 Victor Wang 2010-03-25 17:40:08 PDT
Created attachment 51700 [details]
Remove CRs in index.yaml

Looks like mac-ews does not like CRs in index.yaml (generated by AE while I am running on windows), remove CRs in index.yaml. Hope this will make mac-ews happy.
Comment 9 WebKit Commit Bot 2010-03-26 11:14:51 PDT
Comment on attachment 51700 [details]
Remove CRs in index.yaml

Clearing flags on attachment: 51700

Committed r56635: <http://trac.webkit.org/changeset/56635>
Comment 10 WebKit Commit Bot 2010-03-26 11:14:58 PDT
All reviewed patches have been landed.  Closing bug.