Bug 75202 – Rewrite the CSS parser of prepare-ChangeLog with unittests

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

[Kentaro Hara]

We should add unittests for the CSS parser (i.e. get_function_line_ranges_for_css()) of prepare-ChangeLog.

[Kentaro Hara]

Created attachment 120509 [details]
Patch

[Kentaro Hara]

Created attachment 121095 [details]
rebased patch for review

[David Kilzer (:ddkilzer)]

Comment on attachment 121095 [details]
rebased patch for review

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

It's great to be able to test this code!  r=me

I would really like to see test cases for the warn conditions, and please consider listing the method instead of just the language in %testFiles, but that shouldn't block landing this patch.

> Tools/Scripts/prepare-ChangeLog:1385
> +        foreach my $token (split m-(\{|\}|/\*|\*/)-, $_) {

Nit: I've typically seen m## used instead of m-- when m// is undesirable, but this is okay.

> Tools/Scripts/prepare-ChangeLog:1388
> +                    warn "mismatched brace found in $fileName\n" if $inBrace;

Would be nice to have a test for this case.

> Tools/Scripts/prepare-ChangeLog:1393
> +                    warn "mismatched brace found in $fileName\n" if !$inBrace;

Would be nice to have a test for this case.

> Tools/Scripts/prepare-ChangeLog:1402
> +                warn "mismatched comment found in $fileName\n" if !$inComment;

Would be nice to have a test for this case.

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:59
> +    my $parser = $test->{language} eq "css" ?
> +       eval "\\&PrepareChangeLog::get_selector_line_ranges_for_css" :
> +       eval "\\&PrepareChangeLog::get_function_line_ranges_for_$test->{language}";

At this point, I think it would be easier to store the method name instead of just the language in %testFiles:

my %testFiles = (
    "perl_unittests.pl" => "get_function_line_ranges_for_perl",
    "python_unittests.py" => "get_function_line_ranges_for_python",
    "java_unittests.java" => "get_function_line_ranges_for_java",
    "cpp_unittests.cpp" => "get_function_line_ranges_for_cpp",
    "css_unittests.css" => "get_selector_line_ranges_for_css",
);

Then this becomes simpler to read (change 'language' to 'method' in @testSet):

    my $parser = eval "\\&PrepareChangeLog::$test->{method}";

I'm not going to withhold a review for this, but I think you should consider it.

[David Kilzer (:ddkilzer)]

Comment on attachment 121095 [details]
rebased patch for review

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

>> Tools/Scripts/prepare-ChangeLog:1385
>> +        foreach my $token (split m-(\{|\}|/\*|\*/)-, $_) {
> 
> Nit: I've typically seen m## used instead of m-- when m// is undesirable, but this is okay.

Nevermind...I see m-- and s-- are used elsewhere in prepare-ChangeLog.

[Kentaro Hara]

Committed r104402: <http://trac.webkit.org/changeset/104402>

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 121095 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121095&action=review
> I would really like to see test cases for the warn conditions, and please consider listing the method instead of just the language in %testFiles, but that shouldn't block landing this patch.

Then let me land this patch for now. I'll write a follow-up patch for those test cases.

> > Tools/Scripts/prepare-ChangeLog:1385
> > +        foreach my $token (split m-(\{|\}|/\*|\*/)-, $_) {
> 
> Nit: I've typically seen m## used instead of m-- when m// is undesirable, but this is okay.
> Nevermind...I see m-- and s-- are used elsewhere in prepare-ChangeLog.

Yeah. (But I prefer m## personally too:-)

> > Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:59
> > +    my $parser = $test->{language} eq "css" ?
> > +       eval "\\&PrepareChangeLog::get_selector_line_ranges_for_css" :
> > +       eval "\\&PrepareChangeLog::get_function_line_ranges_for_$test->{language}";
> 
> At this point, I think it would be easier to store the method name instead of just the language in %testFiles:
> 
> my %testFiles = (
>     "perl_unittests.pl" => "get_function_line_ranges_for_perl",
>     "python_unittests.py" => "get_function_line_ranges_for_python",
>     "java_unittests.java" => "get_function_line_ranges_for_java",
>     "cpp_unittests.cpp" => "get_function_line_ranges_for_cpp",
>     "css_unittests.css" => "get_selector_line_ranges_for_css",
> );
> 
> Then this becomes simpler to read (change 'language' to 'method' in @testSet):
> 
>     my $parser = eval "\\&PrepareChangeLog::$test->{method}";

Fixed it and committed. Thanks!