[webkit-reviews] review granted: [Bug 74994] Add a unittest for the Perl parser of prepare-ChangeLog : [Attachment 120158] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 74994: Add a unittest for the Perl parser of prepare-ChangeLog
https://bugs.webkit.org/show_bug.cgi?id=74994

Attachment 120158: Patch
https://bugs.webkit.org/attachment.cgi?id=120158&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=120158&action=review


r=me, but consider the comments.  Thanks!

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:34
> +my @testFiles = ("perl_unittests.pl");

Could use qw(perl_unittests.pl) here so you don't have to use as many double
quotes and commas later.

If you create a naming convention for tests (much like you did for expected
results), this script could simply read the files that match the naming
convention without even listing them here.

(Both of these are suggestions that don't block the review.)

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:37
> +my @expectedFiles = map { File::Spec->catdir($FindBin::Bin, "resources", $_
. "-expected.txt") } @testFiles;

Instead of naming the expected results "perl_unittests.pl-expected.txt", it
would be a little nicer to strip off the file extension from the original name:


use File::Basename;
my $fileName = fileparse($_, qw(.pl));
# or without a temp variable:

my @expectedFiles = map { File::Spec->catdir($FindBin::Bin, "resources",
scalar(fileparse($_, qw(.pl))) . "-expected.txt") } @testFiles;

(Maybe that's too hard to read in one line, though.)

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:43
> +plan(tests => scalar @testFiles);
> +for (my $index = 0; $index < @testFiles; $index++) {

You could pull "scalar @testFiles" out into a $testCount variable to make this
read a little better:

my $testCount = scalar @testFiles;
plan(tests => $testCount);
for (my $index = 0; $index < $testCount; $index++) {

>>> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:56
>>> +	 }
>> 
>> Do we really want to support resetting the results? If anything, we should
be adding some unit-test framework feature that allows us to do this.
Otherwise, we'll end up duplicating this code in other unit tests, no?
> 
> - As for prepare-ChangeLog unittests, there should be some way to reset
results when we add a new unittest. Manually copying the test results to
*-expected.txt is cumbersome and error-prone. So we should somehow support
--reset-results.
> 
> - However, supporting --reset-results in ./Tools/Scripts/test-webkitperl is
not a good idea. This is because in other existing unittests (VCSUtils_unittest
and run-leaks_unittest), expected results are directly written in their Perl
scripts, and thus they cannot support the --reset-results option.
> 
> In summary, --reset-results is a specific option to unittests for the
prepare-ChangeLog parser. With this observation, I guess that supporting
--reset-results in parser_unittests.pl would be a modest solution.

I think --reset-results on this script is fine for now.  We can always revisit
this decision later.

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:63
> +    is_deeply(\@actualOutput, $expectedOutput, "Tests
get_function_line_ranges_for_perl() using CodeGenerator.pm");

This comment needs to be updated so it's more generic.	I suggest using the
$inputFile name, for example:

is_deeply(\@actualOutput, $expectedOutput, "Tests $inputFile");