Bug 72356 – Don't use File::Slurp for run-leaks unit tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Kilzer (:ddkilzer)]

Don't use File::Slurp for run-leaks unit tests

[David Kilzer (:ddkilzer)]

Created attachment 115104 [details]
Patch v1

[WebKit Review Bot]

Attachment 115104 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpe..." exit_code: 1

Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> Attachment 115104 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpe..." exit_code: 1
> 
> Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
> Total errors found: 1 in 5 files

I'll fix this before landing.

[Daniel Bates]

Comment on attachment 115104 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=115104&action=review

> Tools/Scripts/webkitperl/run-leaks_unittest/RunLeaks.pm:28
> +use diagnostics;

Did you intend to leave this in?

> Tools/Scripts/webkitperl/run-leaks_unittest/RunLeaks.pm:34
> +sub read_file {

Throughout the various Perl scripts we use CamelCase for variable names and functions. For consistency, I suggest naming this function readFile().

> Tools/Scripts/webkitperl/run-leaks_unittest/RunLeaks.pm:35
> +    local $/;

I suggest importing the English module and then using $INPUT_RECORD_SEPARATOR here instead of the Perl magic variable $/ so as to improve the readability of this code, especially for people who may not be as familiar with Perl's magic variables. See <http://perldoc.perl.org/English.html> for more details.

You may even want to explain that you're undefining the $INPUT_RECORD_SEPARATOR so as to change the behavior of <FILE> such that it will read everything up to the end of the file. That being said, the function is named read_file().

> Tools/Scripts/webkitperl/run-leaks_unittest/RunLeaks.pm:42
> +my $runLeaksPath = File::Spec->catfile($FindBin::Bin, "..", "..", "run-leaks");

This is OK as-is. For your consideration, I suggest constructing the absolute path to Tools/Scripts/run-leaks using webkitdirs::sourceDir() for the base directory path so as to improve the readability of this line.

> Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v2.0-new.pl:123
> +my $actualOutput = RunLeaks::parseLeaksOutput(@input);

Is this correct? Notice parseLeaksOutput() expects an array reference: <http://trac.webkit.org/browser/trunk/Tools/Scripts/run-leaks?rev=100174#L121>.

> Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v2.0-old.pl:73
> +my $actualOutput = RunLeaks::parseLeaksOutput(@input);

Ditto.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> (From update of attachment 115104 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115104&action=review
> 
> > Tools/Scripts/webkitperl/run-leaks_unittest/RunLeaks.pm:28
> > +use diagnostics;
> 
> Did you intend to leave this in?

I can take it out.  It provided useful error messages when debugging.

> > Tools/Scripts/webkitperl/run-leaks_unittest/RunLeaks.pm:34
> > +sub read_file {
> 
> Throughout the various Perl scripts we use CamelCase for variable names and functions. For consistency, I suggest naming this function readFile().

Okay.  I was replacing the File::Slurp method and forgot about the naming convention.

> > Tools/Scripts/webkitperl/run-leaks_unittest/RunLeaks.pm:35
> > +    local $/;
> 
> I suggest importing the English module and then using $INPUT_RECORD_SEPARATOR here instead of the Perl magic variable $/ so as to improve the readability of this code, especially for people who may not be as familiar with Perl's magic variables. See <http://perldoc.perl.org/English.html> for more details.
> 
> You may even want to explain that you're undefining the $INPUT_RECORD_SEPARATOR so as to change the behavior of <FILE> such that it will read everything up to the end of the file. That being said, the function is named read_file().

Okay.

> > Tools/Scripts/webkitperl/run-leaks_unittest/RunLeaks.pm:42
> > +my $runLeaksPath = File::Spec->catfile($FindBin::Bin, "..", "..", "run-leaks");
> 
> This is OK as-is. For your consideration, I suggest constructing the absolute path to Tools/Scripts/run-leaks using webkitdirs::sourceDir() for the base directory path so as to improve the readability of this line.

Thanks, I'll look into that.

> > Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v2.0-new.pl:123
> > +my $actualOutput = RunLeaks::parseLeaksOutput(@input);
> 
> Is this correct? Notice parseLeaksOutput() expects an array reference: <http://trac.webkit.org/browser/trunk/Tools/Scripts/run-leaks?rev=100174#L121>.
> 
> > Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v2.0-old.pl:73
> > +my $actualOutput = RunLeaks::parseLeaksOutput(@input);
> 
> Ditto.

When I moved the eval statement into the package file, this changed.  Any ideas why?

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 115104 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=115104&action=review
> > 
> > > Tools/Scripts/webkitperl/run-leaks_unittest/RunLeaks.pm:42
> > > +my $runLeaksPath = File::Spec->catfile($FindBin::Bin, "..", "..", "run-leaks");
> > 
> > This is OK as-is. For your consideration, I suggest constructing the absolute path to Tools/Scripts/run-leaks using webkitdirs::sourceDir() for the base directory path so as to improve the readability of this line.
> 
> Thanks, I'll look into that.

I still have to do this in RunLeaks.pm to find the webkitdirs module:

use FindBin;
use lib File::Spec->catdir($FindBin::Bin, "..", "..");
use webkitdirs;

But building the path to run-leaks reads much nicer:

my $runLeaksPath = File::Spec->catfile(sourceDir(), "Tools", "Scripts", "run-leaks");

Thanks!

> > > Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v2.0-new.pl:123
> > > +my $actualOutput = RunLeaks::parseLeaksOutput(@input);
> > 
> > Is this correct? Notice parseLeaksOutput() expects an array reference: <http://trac.webkit.org/browser/trunk/Tools/Scripts/run-leaks?rev=100174#L121>.
> > 
> > > Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v2.0-old.pl:73
> > > +my $actualOutput = RunLeaks::parseLeaksOutput(@input);
> > 
> > Ditto.
> 
> When I moved the eval statement into the package file, this changed.  Any ideas why?

Heh...it's been a while since I thought about prototypes in perl, so I had to refresh my memory.  This is actually the way arrayref prototypes work!

If you look at "man perlsub" in the "Prototypes" section, prototypes were intended to let you write subroutines that work like built-in functions.  So declaring this:

sub mypop(\@);

means you call the method using an array, not an arrayref:

mypop(@myarray);

I don't remember them being that confusing before, but apparently they've always been like this.  :)

I'm not sure why parseLeaksOutput(\@input) worked before when the eval statement was in each *.pl file.  Perhaps the "\@" was being escaped as "@", so we were passing in an array of arguments containing one arrayref?

In any case, the current behavior now makes sense.

[David Kilzer (:ddkilzer)]

Created attachment 115173 [details]
Patch v2

Addressed Dan's feedback.

[Daniel Bates]

Comment on attachment 115173 [details]
Patch v2

Thanks for updating the patch!

[WebKit Review Bot]

Comment on attachment 115173 [details]
Patch v2

Clearing flags on attachment: 115173

Committed r100287: <http://trac.webkit.org/changeset/100287>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.