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.
Maybe "PerlModules" ?
(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.
> 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.
(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.
(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.
Created attachment 45769 [details] Proposed patch This also renames test-webkit-perl to test-webkitperl to match the new folder.
(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 on attachment 45769 [details] Proposed patch looks OK.
(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.
Created attachment 45780 [details] Proposed patch 2 Updated as described here: https://bugs.webkit.org/show_bug.cgi?id=33135#c5
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 on attachment 45780 [details] Proposed patch 2 r=me
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", $_)
$ 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 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>.