Summary: | Rewrite the CSS parser of prepare-ChangeLog with unittests | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||||
Component: | Tools / Tests | Assignee: | Kentaro Hara <haraken> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, ddkilzer, rniwa | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 73531 | ||||||||
Attachments: |
|
Description
Kentaro Hara
2011-12-25 02:07:01 PST
Created attachment 120509 [details]
Patch
Created attachment 121095 [details]
rebased patch for review
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. 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. Committed r104402: <http://trac.webkit.org/changeset/104402> (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! |