Bug 33200

Summary: Generate list of Perl unit test files dynamically
Product: WebKit Reporter: Chris Jerdonek <cjerdonek>
Component: Tools / TestsAssignee: Chris Jerdonek <cjerdonek>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, cjerdonek, commit-queue, darin, ddkilzer, eric, hamaji, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
ddkilzer: review-, ddkilzer: commit-queue-
Proposed patch 2 none

Description Chris Jerdonek 2010-01-05 00:37:22 PST
As David K. suggested here:

https://bugs.webkit.org/show_bug.cgi?id=33124#c15
Comment 1 Chris Jerdonek 2010-01-05 00:47:30 PST
Created attachment 45869 [details]
Proposed patch

Is there a preferred ordering of the "use" statements at the top?
Comment 2 WebKit Review Bot 2010-01-05 00:49:59 PST
style-queue ran check-webkit-style on attachment 45869 [details] without any errors.
Comment 3 David Kilzer (:ddkilzer) 2010-01-05 06:54:06 PST
Comment on attachment 45869 [details]
Proposed patch

These changes look fine, but I noticed that the script isn't using strict mode and doesn't have warnings enabled:

> Index: WebKitTools/Scripts/test-webkitperl

- #/usr/bin/perl
+ #/usr/bin/perl -w

> @@ -30,6 +30,7 @@
>  
>  # Runs unit tests of WebKit Perl code.

+ use strict;

> +use File::Spec;
>  use FindBin;
>  use Test::Harness;
>  use lib $FindBin::Bin; # so this script can be run from any directory.

These two changes will require all variables to be declared (using 'my').  Sorry I didn't catch this earlier!
Comment 4 Chris Jerdonek 2010-01-05 07:22:13 PST
(In reply to comment #3)
> (From update of attachment 45869 [details])
> These changes look fine, but I noticed that the script isn't using strict mode
> and doesn't have warnings enabled:
> ...
> These two changes will require all variables to be declared (using 'my'). 
> Sorry I didn't catch this earlier!

No problem -- thanks for noticing it now!
Comment 5 Chris Jerdonek 2010-01-05 07:23:06 PST
Created attachment 45887 [details]
Proposed patch 2
Comment 6 WebKit Review Bot 2010-01-05 07:28:41 PST
style-queue ran check-webkit-style on attachment 45887 [details] without any errors.
Comment 7 David Kilzer (:ddkilzer) 2010-01-05 09:54:17 PST
Comment on attachment 45887 [details]
Proposed patch 2

> Index: WebKitTools/Scripts/test-webkitperl
> ===================================================================
> --- WebKitTools/Scripts/test-webkitperl	(revision 52795)
> +++ WebKitTools/Scripts/test-webkitperl	(working copy)
> @@ -30,23 +30,20 @@
>  
>  # Runs unit tests of WebKit Perl code.
>  
> +use strict;
> +use warnings;

Adding "-w" to the "#!/usr/bin/perl" line actually catches slightly more issues than "use warnings" (IIRC), but I don't think it really matters for this script.

r=me
Comment 8 WebKit Commit Bot 2010-01-05 11:51:40 PST
Comment on attachment 45887 [details]
Proposed patch 2

Clearing flags on attachment: 45887

Committed r52817: <http://trac.webkit.org/changeset/52817>
Comment 9 WebKit Commit Bot 2010-01-05 11:51:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Chris Jerdonek 2010-01-05 13:07:03 PST
(In reply to comment #7)

Sorry I missed that part of your comment.  Is "use warnings" redundant with "-w"?  The other Perl scripts seem to have both.
Comment 11 David Kilzer (:ddkilzer) 2010-01-05 13:18:50 PST
(In reply to comment #10)
> Sorry I missed that part of your comment.  Is "use warnings" redundant with
> "-w"?  The other Perl scripts seem to have both.

Having both is technically redundant, but doesn't hurt anything.  In this specific case, I think "-w" is equivalent to "use warnings", so it doesn't need both.

See "man warnings" and "man perllexwarn" for details.
Comment 12 Eric Seidel (no email) 2010-01-05 14:27:50 PST
http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Tests%29/builds/8891 seems to think this change caused a regression.  I don't believe it.
Comment 13 Eric Seidel (no email) 2010-01-05 14:36:10 PST
Filed bug 33230 about the failures seen on the bot.  I don't think they're related to this change.