Bug 33124 - Break the Perl unit tests into separate files
Summary: Break the Perl unit tests into separate files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-03 14:54 PST by Chris Jerdonek
Modified: 2010-01-04 07:50 PST (History)
8 users (show)

See Also:


Attachments
Proposed patch (46.84 KB, patch)
2010-01-03 23:51 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 2 (47.82 KB, patch)
2010-01-04 01:19 PST, Chris Jerdonek
ddkilzer: review+
cjerdonek: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2010-01-03 14:54:15 PST
In bug 33098, comment 15, David K. suggested breaking the Perl unit tests up into separate files in their own folder(s).

I'm thinking the following:

/Scripts
    /perl
        /VCSUtils_unittests (one folder per .pm file)
            fixChangeLog.pl
            runPatchCommand.pl
            etc. (one file per subroutine)

The reason for the extra directory (perl) is to have a place other than the top level Scripts folder for supporting Perl files (similar to Python's modules folder).  For example, we could eventually move VCSUtils.pm into /perl to be a sibling of /VCSUtils_unittests, and so on.
Comment 1 Adam Barth 2010-01-03 14:58:44 PST
Maybe "PerlModules" ?
Comment 2 David Kilzer (:ddkilzer) 2010-01-03 15:05:17 PST
(In reply to comment #1)
> Maybe "PerlModules" ?

Should "modules" be renamed to "PythonModules" too?  I don't know if there is a convention is for python module directories.
Comment 3 Adam Barth 2010-01-03 15:11:34 PST
> Should "modules" be renamed to "PythonModules" too?  I don't know if there is a
> convention is for python module directories.

Probably.  Eric and I have a bunch of FIXIT tasks that we're going to try to knock off soon.  I'll add this to the list.
Comment 4 Chris Jerdonek 2010-01-03 15:59:44 PST
(In reply to comment #2)
> (In reply to comment #1)
> > Maybe "PerlModules" ?
> 
> Should "modules" be renamed to "PythonModules" too?  I don't know if there is a
> convention is for python module directories.

Yeah, there are:

http://www.python.org/dev/peps/pep-0008/

The convention is to use all lowercase.  (Packages are essentially folders of modules and nested packages.)

I had some thoughts on this here:

https://bugs.webkit.org/show_bug.cgi?id=32773#c11

One consideration is how the name will look in our code.  In our Python scripts in the top level Scripts folder, we will be writing things like "import <directory_name>.bugzilla", etc.

So I think something like "webkitpy" (or "pywebkit") might be good.  That way we will be writing things like:

import webkitpy.style.checker;
import webkitpy.bugzilla...
import webkitpy.scm...

It's also in the style of other Python names (e.g. pydoc, numpy, etc).

A couple nits about putting the word "modules" in the name is that (1) it seems redundant, and (2) we may also have packages immediately below.  For example, the style-related code will I think become a style package.

https://bugs.webkit.org/show_bug.cgi?id=32971

The whole folder could be viewed as a master "webkit" package of all our Python code.

In terms of naming the Perl folder, I don't know if the package/module terminology in Perl is completely parallel to Python's or not, but in either case I'm not sure whether the extra word "says" much more.
Comment 5 Chris Jerdonek 2010-01-03 16:24:36 PST
(In reply to comment #4)

Here's another idea--

pykit
perlkit

On a different note, webkitpy would let us ensure that the folders appear near each other when viewed alphabetically:

webkitperl 
webkitpy

The Python style guide also says package names should be short.
Comment 6 Chris Jerdonek 2010-01-03 23:51:20 PST
Created attachment 45769 [details]
Proposed patch

This also renames test-webkit-perl to test-webkitperl to match the new folder.
Comment 7 Chris Jerdonek 2010-01-03 23:53:52 PST
(In reply to comment #6)
> Created an attachment (id=45769) [details]
> Proposed patch
> 
> This also renames test-webkit-perl to test-webkitperl to match the new folder.

Oh, and we need to make sure that the following file has the "allow-tabs" property set (not sure about the exact name of the property):

Scripts/webkitperl/VCSUtils_unittest/runPatchCommand.pl

(Previously, it was VCSUtils_unittest.pl that had this property.)
Comment 8 Eric Seidel (no email) 2010-01-03 23:57:30 PST
Comment on attachment 45769 [details]
Proposed patch

looks OK.
Comment 9 Chris Jerdonek 2010-01-04 00:43:19 PST
(In reply to comment #8)
> (From update of attachment 45769 [details])
> looks OK.

Thanks, Eric!  I'll let one of you guys manually commit it since I don't have commit status and there's a good chance this won't work with cq.

As a reminder, test-webkitperl needs to have a+x (same as its copy source), and Scripts/webkitperl/VCSUtils_unittest/runPatchCommand.pl needs to have the SVN allow-tabs property (again, same as its corresponding copy source).

Hopefully it will land without any problems.
Comment 10 Chris Jerdonek 2010-01-04 01:19:33 PST
Created attachment 45780 [details]
Proposed patch 2

Updated as described here:

https://bugs.webkit.org/show_bug.cgi?id=33135#c5
Comment 11 WebKit Review Bot 2010-01-04 02:52:14 PST
Attachment 45780 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Skipping input 'WebKitTools/Scripts/test-webkit-perl': Can't open for reading
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/runPatchCommand.pl:74:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/runPatchCommand.pl:75:  Line contains tab character.  [whitespace/tab] [5]
Skipping input 'WebKitTools/Scripts/VCSUtils_unittest.pl': Can't open for reading
Total errors found: 2
Comment 12 David Kilzer (:ddkilzer) 2010-01-04 06:27:33 PST
Comment on attachment 45780 [details]
Proposed patch 2

r=me
Comment 13 David Kilzer (:ddkilzer) 2010-01-04 06:55:44 PST
Comment on attachment 45780 [details]
Proposed patch 2

> Index: WebKitTools/Scripts/test-webkitperl
> +# Use an absolute path so this script can be run from any directory.
> +$scriptsDir = $FindBin::Bin;
> +$scriptsDir =~ s|/+$||; # Remove trailing '/'
> +
> +runtests("$scriptsDir/VCSUtils_unittest.pl");

FWIW, we usually use File::Spec->catfile($scriptDir, "VCSUtils_unittest.pl") to concatenate paths.  Then you don't have to worry about trailing slashes.

> Index: WebKitTools/Scripts/test-webkitperl
> +@files = map "$scriptsDir/webkitperl/VCSUtils_unittest/$_", @files; # prepend

Same here:  File::Spec->catfile($scriptsDir, "webkitperl/VCSUtils_unittest", $_)
Comment 14 David Kilzer (:ddkilzer) 2010-01-04 07:09:44 PST
$ svn commit WebKitTools
Sending        WebKitTools/ChangeLog
Sending        WebKitTools/Scripts/VCSUtils.pm
Deleting       WebKitTools/Scripts/VCSUtils_unittest.pl
Deleting       WebKitTools/Scripts/test-webkit-perl
Sending        WebKitTools/Scripts/test-webkit-scripts
Adding         WebKitTools/Scripts/test-webkitperl
Adding         WebKitTools/Scripts/webkitperl
Adding         WebKitTools/Scripts/webkitperl/VCSUtils_unittest
Adding         WebKitTools/Scripts/webkitperl/VCSUtils_unittest/fixChangeLogPatch.pl
Adding         WebKitTools/Scripts/webkitperl/VCSUtils_unittest/generatePatchCommand.pl
Adding         WebKitTools/Scripts/webkitperl/VCSUtils_unittest/runPatchCommand.pl
Transmitting file data .......
Committed revision 52732.

<http://trac.webkit.org/changeset/52732>
Comment 15 David Kilzer (:ddkilzer) 2010-01-04 07:50:15 PST
Comment on attachment 45780 [details]
Proposed patch 2

> Index: WebKitTools/Scripts/test-webkitperl
> +# VCSUtils subroutines
> +my @files = (
> +    "fixChangeLogPatch.pl",
> +    "generatePatchCommand.pl",
> +    "runPatchCommand.pl",
> +);
> +
> +@files = map "$scriptsDir/webkitperl/VCSUtils_unittest/$_", @files; # prepend
> +
> +runtests(@files);

In the future, consider making this dynamic by reading in file names from within Perl using readdir() and friends, or using globbing like this:  @files = <$scriptsDir/webkitperl/*_unittest/*.pl>.