Bug 31498 - Chromium's run_webkit_tests should move into svn.webkit.org as run-chromium-webkit-tests
Summary: Chromium's run_webkit_tests should move into svn.webkit.org as run-chromium-w...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-13 16:07 PST by Eric Seidel (no email)
Modified: 2010-01-31 15:56 PST (History)
10 users (show)

See Also:


Attachments
Patch (459.64 KB, patch)
2010-01-21 21:23 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
Start of described python program (1.61 KB, text/plain)
2010-01-22 08:12 PST, David Levin
no flags Details
file that changes any MixedCase function names to lower_case_with_underscores (6.25 KB, text/x-python-script)
2010-01-26 00:15 PST, Dirk Pranke
no flags Details
revised patch with pep-8 compliant function names and an initial pass at the licenses. Only for review - do not commit (512.40 KB, patch)
2010-01-28 13:55 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
ready for review + commit (license text updated (512.48 KB, patch)
2010-01-28 14:44 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (204.88 KB, patch)
2010-01-29 16:53 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (194.51 KB, patch)
2010-01-29 17:05 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (30.63 KB, patch)
2010-01-29 17:13 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (119.24 KB, patch)
2010-01-29 17:21 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (74.34 KB, patch)
2010-01-29 17:29 PST, Dirk Pranke
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-11-13 16:07:52 PST
Chromium's run_webkit_tests should move into svn.webkit.org as run-chromium-webkit-tests

Or some similar name.

http://src.chromium.org/viewvc/chrome/trunk/src/webkit/tools/layout_tests/run_webkit_tests.py?view=markup

There is actually a whole suite of python modules which support that file as well.

It's silly for this to live in src.chromium.org now that we can build a WebKit.dll out of svn.webkit.org.

Eventually I would like to unify run-webkit-tests and run_webkit_tests.py into a single script, but that will have to happen after moving the latter into svn.webkit.org

The chromium bug: http://code.google.com/p/chromium/issues/detail?id=23099

I'll send a note to webkit-dev announcing my intention.
Comment 1 Dirk Pranke 2009-11-19 15:33:41 PST
per discussion w/ eseidel, I'll start on this.
Comment 2 Dirk Pranke 2010-01-21 21:23:35 PST
Created attachment 47175 [details]
Patch
Comment 3 David Levin 2010-01-21 22:02:43 PST
Just some items that I noticed in passing (drive by comments):
1. WebKitTools/simplejson/README.webkit refers to LICENSE.txt and there is none.
2. None of the files appear to have their license in them. (For example, the files in Scripts/webkitpy/layout_tests refer to LICENSE but there is no such file -- even if there was, there is no such .)
3. Several files have TODO(name) vs FIXME (which is done in WebKit land).
4. The python method/function names don't seem to follow PEP8 (lowercase with underscores).
5. There seem to be a lot of chrome specific reference in here that aren't in WebKit at all (For example there is a reference to "chrome\test\data\ssl\certs\root_ca_cert.crt" and test_output_formatter.bat uses chromiumpaths, etc.)
Comment 4 Adam Barth 2010-01-21 22:25:09 PST
The patch is not correctly based.  Consider using webkit-patch upload.

Also, the patch is huge.  I'm not sure how to meaningfully review it.
Comment 5 Dirk Pranke 2010-01-21 23:00:49 PST
(In reply to comment #3)
> Just some items that I noticed in passing (drive by comments):
> 1. WebKitTools/simplejson/README.webkit refers to LICENSE.txt and there is
> none.

Hmm. The downstream version didn't have a LICENSE as far as I know. I'll hunt one down after the patch lands, if that's okay.

> 2. None of the files appear to have their license in them. (For example, the
> files in Scripts/webkitpy/layout_tests refer to LICENSE but there is no such
> file -- even if there was, there is no such .)

Odd. I'll take a look at that.

> 3. Several files have TODO(name) vs FIXME (which is done in WebKit land).

I'll clean that up after the patch lands, if that's okay ...

> 4. The python method/function names don't seem to follow PEP8 (lowercase with
> underscores).

The code was cleaned up to PEP-8 compliance as enforced by pep8.py ; I think we'll gradually rename the functions over time to get the rest of the cleanup.


> 5. There seem to be a lot of chrome specific reference in here that aren't in
> WebKit at all (For example there is a reference to
> "chrome\test\data\ssl\certs\root_ca_cert.crt" and test_output_formatter.bat
> uses chromiumpaths, etc.)

Yeah, the code doesn't run independently of chromium yet, and there's still a bunch of cleanup to do. I submitted this as-is per discussion with eseidel and some other people on #webkit who were keen to see it land so they could help contribute to the upstreaming and cleanup.
Comment 6 Dirk Pranke 2010-01-21 23:04:48 PST
(In reply to comment #4)
> The patch is not correctly based.  Consider using webkit-patch upload.

I don't understand what this means. I used webkit-patch post (I think). Is that different?

> 
> Also, the patch is huge.  I'm not sure how to meaningfully review it.
>

I'm expecting eseidel to review it :) Since the only intended purpose at the moment is to be able to run the chromium tests, you could compare it against the downstream version as a sanity check. 

Even if I were to attempt to upload it in somewhat separable chunks, most of them would still be huge :(
Comment 7 David Levin 2010-01-21 23:27:22 PST
(In reply to comment #5)
> > 4. The python method/function names don't seem to follow PEP8 (lowercase with
> > underscores).
> 
> The code was cleaned up to PEP-8 compliance as enforced by pep8.py ; I think
> we'll gradually rename the functions over time to get the rest of the cleanup.

The concern with this idea (and related responses) because once it lands, there is much less motivation to clean up things. (I've done it too unfortunately at times where I have less time to clean up something that has landed vs something that hasn't.) The "I think" and "gradually" above reinforce this perception.

Alternatively this could be cleaned up downstream and then landed in WebKit.
Comment 8 Adam Barth 2010-01-21 23:29:58 PST
(In reply to comment #6)
> (In reply to comment #4)
> > The patch is not correctly based.  Consider using webkit-patch upload.
> 
> I don't understand what this means. I used webkit-patch post (I think). Is that
> different?

Oh, you're right.  It is correctly based (created from the proper directory).  PrettyPatch tricked me.  Not sure why it didn't apply to TOT.
Comment 9 Eric Seidel (no email) 2010-01-22 03:51:03 PST
I don't think this patch want's a "code-review".  We know the code works, we know it will need further cleanuyp/style-adjusting, testing, reorg, etc. over time.

The one thing we do need to review, is licenses.  WebKit style is to have a license at the top of every file.  I also think we need to specify who the "chromium authors" Are since we dont have any of the chromium license files in our tree.

Other than that, I'm ready to rs=me this change.

So: we need to add a BSD license header to teh top of every file, with a list of copyright holders (I don't think the chromium authors counts here, or?)

At least thats my understanding of webkit's contribution requirements.  Please someone correct me if i've misunderstood.
Comment 10 David Levin 2010-01-22 08:12:37 PST
Created attachment 47206 [details]
Start of described python program

(In reply to comment #9)
> I don't think this patch want's a "code-review".  We know the code works, we
> know it will need further cleanuyp/style-adjusting, testing, reorg, etc. over
> time.

I'm *very* concerned about landing this code as is.

Landing it removes any urgency to fix its bad style. If the past is a guide, this will languish, and the file will be in the wrong style for a long time to come and serve as a bad example of style that is checked in. Then folks will use it as an example of style that WebKit except when code reviews are done -- also based on recent experience :(.

*Instead* one could take a little time to write a relatively simple python program to fix the function names and do this upstream. This may be a useful program if anything else needs to come from chromium (due to chromium's unfortunate python style):

Here's an outline of that program:

for each file in python patch
  for each line
     if line matches r"def\s+([A-Za-z_][A-Za-z_0-9])*\s*\(":
         get lower case function name
         put orig function name in dict with lower case name

for each file in python patch
  for each line
    for each item in dict
      if line matches r"\s+"+item_from_dict:
        replace item_from_dict with lowercase version
  write file

Now this may make some mistakes, but a code review should be able to verify it as well as running the program as a double check.

I attached a python program that I started a while ago (when I had some spare moments) which is headed in this direction but still needs work to finish it off.
Comment 11 Adam Barth 2010-01-22 11:24:48 PST
> So: we need to add a BSD license header to teh top of every file, with a list
> of copyright holders (I don't think the chromium authors counts here, or?)

In principle, you need to list all the Chromium Authors as they are the copyright holders.  In practice, you should look at the SVN log and see who actually touched the file.  Everyone employed by Google can be listed as Google, Inc because the code was produces as a work-for-hire.

I am not a lawyer but I play one on the Interwebs.
Comment 12 David Levin 2010-01-22 13:23:06 PST
Comment on attachment 47175 [details]
Patch

As I understand it, there will be another revision of the patch. r- for now.
Comment 13 Dirk Pranke 2010-01-26 00:15:44 PST
Created attachment 47389 [details]
file that changes any MixedCase function names to lower_case_with_underscores

I've attached the script I'm using to do the bulk-rename of any function and method names. There are a couple of big gotchas with the script:

1) If there are namespace collisions, they will not be caught. In other words, if you want to rename foo.Run() and not bar.Run(), you're out of luck.

2) It won't rename any occurrences of the names that appear inside strings or multi-line comments

And, obviously, it can't rename occurrences that you don't tell it about, so if other files are calling these functions, they'll break.

As far as I can tell, for the task at hand, none of these gotchas matter.
Comment 14 Dirk Pranke 2010-01-28 13:55:45 PST
Created attachment 47645 [details]
revised patch with pep-8 compliant function names and an initial pass at the licenses. Only for review - do not commit
Comment 15 Eric Seidel (no email) 2010-01-28 14:23:18 PST
We could do this commit in various pieces.  For one, it would be simple to add simplejson in one commit.

I'm not sure what review comments are really expected for a .5MB patch.  It's basically unreviewable at this size.

I'm also not sure it needs much review beyond "the licenses look right".  This is a huge hunk of code which will need to be worked over in th webkit repository, and would be impossible to work over via the review process on this bug.
Comment 16 Dirk Pranke 2010-01-28 14:44:27 PST
Created attachment 47651 [details]
ready for review + commit (license text updated
Comment 17 David Kilzer (:ddkilzer) 2010-01-28 22:04:39 PST
(In reply to comment #15)
> I'm not sure what review comments are really expected for a .5MB patch.  It's
> basically unreviewable at this size.
> 
> I'm also not sure it needs much review beyond "the licenses look right".  This
> is a huge hunk of code which will need to be worked over in th webkit
> repository, and would be impossible to work over via the review process on this
> bug.

This sounds a lot like comments for the patches attached to Bug 16401.  Why can't this script be made to run first before landing?  How will landing it in the repository help to get it fixed?
Comment 18 Dirk Pranke 2010-01-29 11:27:15 PST
(In reply to comment #17)
> (In reply to comment #15)
> > I'm not sure what review comments are really expected for a .5MB patch.  It's
> > basically unreviewable at this size.
> > 
> > I'm also not sure it needs much review beyond "the licenses look right".  This
> > is a huge hunk of code which will need to be worked over in th webkit
> > repository, and would be impossible to work over via the review process on this
> > bug.
> 
> This sounds a lot like comments for the patches attached to Bug 16401.  Why
> can't this script be made to run first before landing?  How will landing it in
> the repository help to get it fixed?

Hi David,

I think perhaps you misunderstand. The "file that changes any MixedCase function" script has already been run and we're not planning to land it; it's just attached for reference. Rather, we're talking about the last attachment ( https://bugs.webkit.org/attachment.cgi?id=47651 ), which are the actual files that we're upstreaming from chromium.org now that they've been reformatted using the script.
Comment 19 David Kilzer (:ddkilzer) 2010-01-29 12:42:41 PST
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > I'm not sure what review comments are really expected for a .5MB patch.  It's
> > > basically unreviewable at this size.
> > > 
> > > I'm also not sure it needs much review beyond "the licenses look right".  This
> > > is a huge hunk of code which will need to be worked over in th webkit
> > > repository, and would be impossible to work over via the review process on this
> > > bug.
> > 
> > This sounds a lot like comments for the patches attached to Bug 16401.  Why
> > can't this script be made to run first before landing?  How will landing it in
> > the repository help to get it fixed?
> 
> I think perhaps you misunderstand. The "file that changes any MixedCase
> function" script has already been run and we're not planning to land it; it's
> just attached for reference. Rather, we're talking about the last attachment (
> https://bugs.webkit.org/attachment.cgi?id=47651 ), which are the actual files
> that we're upstreaming from chromium.org now that they've been reformatted
> using the script.

No, I was referring to the run_webkit_tests script itself.  Why does checking it into the WebKit repository make it easier to work on (e.g., is the script really so big that one person can't fix it up before checking it in)?  What are the barriers to making it actually work (or do something useful) before being committed?
Comment 20 Dirk Pranke 2010-01-29 15:54:03 PST
(In reply to comment #19)
> No, I was referring to the run_webkit_tests script itself.  Why does checking
> it into the WebKit repository make it easier to work on (e.g., is the script
> really so big that one person can't fix it up before checking it in)?  What are
> the barriers to making it actually work (or do something useful) before being
> committed?

Oh, sorry, I misunderstood. The script as it is runs fine for running the chromium version of the test suite. The intent of checking this in at all was (a) to move this stuff upstream alongside the rest of the chromium port and (b) make this implementation available for other ports to use, since it has some advantages over the perl versions.

Eric and I are working on getting it to work for the base webkit ports, but there was demand (from Eric and some others) to check this in so that they could help with the porting, rather than waiting until I had every i dotted.
Comment 21 Dirk Pranke 2010-01-29 16:53:45 PST
Created attachment 47745 [details]
Patch
Comment 22 Dirk Pranke 2010-01-29 17:05:05 PST
Created attachment 47746 [details]
Patch
Comment 23 Eric Seidel (no email) 2010-01-29 17:06:14 PST
Comment on attachment 47746 [details]
Patch

Looks great!  Obviously we still have a lot of work here. Thank you for your persistence!
Comment 24 Dirk Pranke 2010-01-29 17:07:07 PST
Committed r54091: <http://trac.webkit.org/changeset/54091>
Comment 25 Dirk Pranke 2010-01-29 17:13:16 PST
Created attachment 47747 [details]
Patch
Comment 26 Eric Seidel (no email) 2010-01-29 17:16:03 PST
Comment on attachment 47747 [details]
Patch

Looks good to me.  Classes are *soo* much cleaner than what we have in the perl code.  This change is nice and small and seems pretty straightforward to me.

Thanks!
Comment 27 Eric Seidel (no email) 2010-01-29 17:18:14 PST
Comment on attachment 47747 [details]
Patch

Was committed as <http://trac.webkit.org/changeset/54093
Comment 28 Dirk Pranke 2010-01-29 17:21:56 PST
Created attachment 47749 [details]
Patch
Comment 29 Eric Seidel (no email) 2010-01-29 17:24:03 PST
Comment on attachment 47749 [details]
Patch

Looks non-harmful.  So glad to finally have this runable from webkit.org
Comment 30 Dirk Pranke 2010-01-29 17:25:30 PST
Committed r54094: <http://trac.webkit.org/changeset/54094>
Comment 31 Dirk Pranke 2010-01-29 17:29:46 PST
Created attachment 47750 [details]
Patch
Comment 32 Eric Seidel (no email) 2010-01-29 17:33:02 PST
Comment on attachment 47750 [details]
Patch

After talking in person, let's wait on this part of the patch, to see if anyone is actually still using this code.

Looks like we're otherwise done here!  And can close this bug.
Comment 33 Brian Weinstein 2010-01-31 00:43:50 PST
Would r54903 or r54904 have caused any changes in Windows expected results? Right around r54093-r54095 the number of failures on Windows went from 4 to 18. 

I sent a comment to the author of r54095, and just wanted to put one here to ask if this could have changed Windows expected results (especially in media/).
Comment 34 Dirk Pranke 2010-01-31 15:55:42 PST
(In reply to comment #33)
> Would r54903 or r54904 have caused any changes in Windows expected results?
> Right around r54093-r54095 the number of failures on Windows went from 4 to 18. 
> 
> I sent a comment to the author of r54095, and just wanted to put one here to
> ask if this could have changed Windows expected results (especially in media/).

Not likely. This code should be completely unused at the moment.
Comment 35 Brian Weinstein 2010-01-31 15:56:41 PST
(In reply to comment #34)
> (In reply to comment #33)
> > Would r54903 or r54904 have caused any changes in Windows expected results?
> > Right around r54093-r54095 the number of failures on Windows went from 4 to 18. 
> > 
> > I sent a comment to the author of r54095, and just wanted to put one here to
> > ask if this could have changed Windows expected results (especially in media/).
> 
> Not likely. This code should be completely unused at the moment.

Agreed, it looks like it was r54095. Sorry for the spam.