We should add unittests for the CSS parser (i.e. get_function_line_ranges_for_css()) of prepare-ChangeLog.
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!