Summary: | Break the Perl unit tests into separate files | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Jerdonek <cjerdonek> | ||||||
Component: | Tools / Tests | Assignee: | Chris Jerdonek <cjerdonek> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, aroben, cjerdonek, darin, ddkilzer, eric, hamaji, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Chris Jerdonek
2010-01-03 14:54:15 PST
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>. |