Bug 74994 – Add a unittest for the Perl parser of prepare-ChangeLog

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kentaro Hara]

We can add unittests for the parsers of prepare-ChangeLog. In this bug, we add a unittest for the Perl parser, i.e. get_function_line_ranges_for_perl() in prepare-ChangeLog.

If this unittest looks good, I'll add unittests for C, Java, JavaScript and CSS in upcoming patches.

[Kentaro Hara]

Created attachment 120147 [details]
Patch

[Ryosuke Niwa]

Can we split this into few pieces? It's hard to review such a large patch.

[Kentaro Hara]

(In reply to comment #2)
> Can we split this into few pieces? It's hard to review such a large patch.

Thanks for the review. But as commented, actually this patch is just 30 lines (get_function_line_ranges_for_perl.pl). CodeGeneratorJS.pm is just copied from WebCore/bindings/scripts/.

[Kentaro Hara]

(In reply to comment #3)
> (In reply to comment #2)
> > Can we split this into few pieces? It's hard to review such a large patch.
> 
> Thanks for the review. But as commented, actually this patch is just 30 lines (get_function_line_ranges_for_perl.pl). CodeGeneratorJS.pm is just copied from WebCore/bindings/scripts/.

Ah, do you mean we should start from a more smaller unittest?

[Ryosuke Niwa]

Ah now I see what you're doing. Thanks for the clarification.

[Ryosuke Niwa]

(In reply to comment #5)
> Ah now I see what you're doing. Thanks for the clarification.

Starting from a smaller unit test will be nice.

[Kentaro Hara]

(In reply to comment #6)
> (In reply to comment #5)
> > Ah now I see what you're doing. Thanks for the clarification.
> 
> Starting from a smaller unit test will be nice.

Sure, the patch is coming.

[Kentaro Hara]

Created attachment 120158 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 120158 [details]
Patch

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

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:56
> +    if ($resetResults) {
> +        open FH, "> $expectedFile" or die "Cannot open $expectedFile: $!";
> +        print FH Data::Dumper->new([\@actualOutput])->Terse(1)->Indent(1)->Dump();
> +        close FH;
> +        next;
> +    }

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?

[Kentaro Hara]

(In reply to comment #9)
> (From update of attachment 120158 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120158&action=review
> 
> > Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:56
> > +    if ($resetResults) {
> > +        open FH, "> $expectedFile" or die "Cannot open $expectedFile: $!";
> > +        print FH Data::Dumper->new([\@actualOutput])->Terse(1)->Indent(1)->Dump();
> > +        close FH;
> > +        next;
> > +    }
> 
> 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.

> we'll end up duplicating this code in other unit tests, no?

I think that parser_unittests.pl can (or should) handle all unittests for the parser.

[David Kilzer (:ddkilzer)]

Comment on attachment 120158 [details]
Patch

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");

[Kentaro Hara]

Created attachment 120235 [details]
patch for commit

[Kentaro Hara]

Comment on attachment 120158 [details]
Patch

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

>> 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.)

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

Done.

> 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.

Hmm... the idea is OK but I would prefer explicitly describing the list of unittests here, to make it clear what you are testing. For now, I leave it as it is.

>> 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.)

Done. But instead of fileparse, I used s/^(.*)\.[^\.]*$/$1/, in order to prevent people who will add a new test from adding the test extension here.

>> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:43
>> +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++) {

Done.

>>>> 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.

Agreed.

>> 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");

Done. Thanks for the review!

[WebKit Review Bot]

Comment on attachment 120235 [details]
patch for commit

Clearing flags on attachment: 120235

Committed r103515: <http://trac.webkit.org/changeset/103515>