Bug 33124 – Break the Perl unit tests into separate files

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

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.

[Adam Barth]

Maybe "PerlModules" ?

[David Kilzer (:ddkilzer)]

(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.

[Adam Barth]

> 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.

[Chris Jerdonek]

(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.

[Chris Jerdonek]

(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.

[Chris Jerdonek]

Created attachment 45769 [details]
Proposed patch

This also renames test-webkit-perl to test-webkitperl to match the new folder.

[Chris Jerdonek]

(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.)

[Eric Seidel (no email)]

Comment on attachment 45769 [details]
Proposed patch

looks OK.

[Chris Jerdonek]

(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.

[Chris Jerdonek]

Created attachment 45780 [details]
Proposed patch 2

Updated as described here:

https://bugs.webkit.org/show_bug.cgi?id=33135#c5

[WebKit Review Bot]

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

[David Kilzer (:ddkilzer)]

Comment on attachment 45780 [details]
Proposed patch 2

r=me

[David Kilzer (:ddkilzer)]

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", $_)

[David Kilzer (:ddkilzer)]

$ 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>

[David Kilzer (:ddkilzer)]

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>.