Bug 77250 - webkit-perf.appspot.com should have an ability to merge tests
Summary: webkit-perf.appspot.com should have an ability to merge tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Website (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 77037
  Show dependency treegraph
 
Reported: 2012-01-27 15:22 PST by Ryosuke Niwa
Modified: 2012-01-30 15:06 PST (History)
5 users (show)

See Also:


Attachments
Adds admin/merge-tests (13.62 KB, patch)
2012-01-27 15:55 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-01-27 15:22:08 PST
When tests are moved or renamed, we need to merge multiple Test objects. Provide an interface to do this on an admin page.
Comment 1 Ryosuke Niwa 2012-01-27 15:55:46 PST
Created attachment 124388 [details]
Adds admin/merge-tests
Comment 2 WebKit Review Bot 2012-01-27 16:00:21 PST
Attachment 124388 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Websites/webkit-perf.appspot..." exit_code: 1

Websites/webkit-perf.appspot.com/merge_tests_handler.py:45:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Ryosuke Niwa 2012-01-27 16:02:05 PST
The style error is caused by ";" in content-type http header. Unfortunately, we use a third-party script for python style check so it's non-trivial to fix it.
Comment 4 Ryosuke Niwa 2012-01-30 13:53:59 PST
Ping reviewers. This patch is blocking my patch to add memcache.
Comment 5 Adam Barth 2012-01-30 14:23:07 PST
Comment on attachment 124388 [details]
Adds admin/merge-tests

I'm not sure what sort of review you're looking for here.  Generally, we use PEP8 style in our python (but without the 80 col limit).  Names like mergedResults don't match that style, but given that this is an addition to code imported from Mozilla, we should probably match their style.  Are you planning to contribute these changes upstream?

In any case, I'm inclined to rubber-stamp these sorts of changes.
Comment 6 Ryosuke Niwa 2012-01-30 14:51:17 PST
Thanks for the review.

(In reply to comment #5)
> (From update of attachment 124388 [details])
> I'm not sure what sort of review you're looking for here.  Generally, we use PEP8 style in our python (but without the 80 col limit).  Names like mergedResults don't match that style, but given that this is an addition to code imported from Mozilla, we should probably match their style.

They use names with underscores? I can change them since all the python code I'm checking into webkit won't be shared with Mozilla.

> Are you planning to contribute these changes upstream?

No. Python backend is specific to WebKit. In fact, all patches I've contributed so far only contains WebKit-specific code. As I've outlined in http://trac.webkit.org/changeset/105971, we need to import some code from https://github.com/mozilla/graphs but it's all in front-end (css, js, html).
Comment 7 Adam Barth 2012-01-30 14:55:00 PST
> They use names with underscores? I can change them since all the python code I'm checking into webkit won't be shared with Mozilla.

Yeah.  I'm surprised check-webkit-style didn't complain.  (Maybe it stops at the first error in each file?)
Comment 8 Ryosuke Niwa 2012-01-30 15:06:44 PST
Comment on attachment 124388 [details]
Adds admin/merge-tests

Clearing flags on attachment: 124388

Committed r106292: <http://trac.webkit.org/changeset/106292>
Comment 9 Ryosuke Niwa 2012-01-30 15:06:47 PST
All reviewed patches have been landed.  Closing bug.