Don't use File::Slurp for run-leaks unit tests
Created attachment 115104 [details] Patch v1
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.
(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.
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.
(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?
(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.
Created attachment 115173 [details] Patch v2 Addressed Dan's feedback.
Comment on attachment 115173 [details] Patch v2 Thanks for updating the patch!
Comment on attachment 115173 [details] Patch v2 Clearing flags on attachment: 115173 Committed r100287: <http://trac.webkit.org/changeset/100287>
All reviewed patches have been landed. Closing bug.