Bug 159074 – parser_unittests.pl should not hardcode list of tests

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

[David Kilzer (:ddkilzer)]

Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl currently hardcodes the list of tests that it runs.

It should read them dynamically from the 'resources' subdirectory.

[David Kilzer (:ddkilzer)]

Created attachment 281936 [details]
Patch v1

[Daniel Bates]

Comment on attachment 281936 [details]
Patch v1

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

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:48
> +    ".pl" => "get_function_line_ranges_for_perl",
> +    ".py" => "get_function_line_ranges_for_python",
> +    ".java" => "get_function_line_ranges_for_java",
> +    ".cpp" => "get_function_line_ranges_for_cpp",
> +    ".js" => "get_function_line_ranges_for_javascript",
> +    ".css" => "get_selector_line_ranges_for_css",
> +    ".swift" => "get_function_line_ranges_for_swift",

For your consideration, I suggest that we sort these lines by hash key such that the first line is for key ".cpp".

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:146
> +        next if m/^\./;

The leading 'm' is not needed when '/' is used as regular expression delimiter.

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:148
> +        next if m/-expected.txt$/;

The leading 'm' is not needed when '/' is used as regular expression delimiter.

Take the current filename ($_) to be  "A-expectedAtxt". Then the regular expression will match because the '.' (period) between the name of the file and its extension is not escaped and will be interpreted as a pattern metacharacter. This is unlikely to cause an issue in practice. One way to fix this issue is to escape the '.' by preceding it with a forward slash ('\') as you did on line 146. Alternatively you can disable interpretation of pattern metacharacters in the expression "-expected.txt" by placing it inside \Q...\E such that the regular expression reads /\Q-expected.txt\E$/.

[David Kilzer (:ddkilzer)]

Comment on attachment 281936 [details]
Patch v1

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

Thanks Dan!

>> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:48
>> +    ".swift" => "get_function_line_ranges_for_swift",
> 
> For your consideration, I suggest that we sort these lines by hash key such that the first line is for key ".cpp".

Good call!  Will fix.

>> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:146
>> +        next if m/^\./;
> 
> The leading 'm' is not needed when '/' is used as regular expression delimiter.

Will fix.

>> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:148
>> +        next if m/-expected.txt$/;
> 
> The leading 'm' is not needed when '/' is used as regular expression delimiter.
> 
> Take the current filename ($_) to be  "A-expectedAtxt". Then the regular expression will match because the '.' (period) between the name of the file and its extension is not escaped and will be interpreted as a pattern metacharacter. This is unlikely to cause an issue in practice. One way to fix this issue is to escape the '.' by preceding it with a forward slash ('\') as you did on line 146. Alternatively you can disable interpretation of pattern metacharacters in the expression "-expected.txt" by placing it inside \Q...\E such that the regular expression reads /\Q-expected.txt\E$/.

Good catch--wrote the regex too quickly!  :)  Will also fix the m// operator.

[David Kilzer (:ddkilzer)]

Committed r202433: <http://trac.webkit.org/changeset/202433>

[Konstantin Tokarev]

In final version of patch, I think 

  next if $_ eq EXPECTED_RESULTS_SUFFIX;

would be more readable then

  next if /\Q@{[EXPECTED_RESULTS_SUFFIX]}\E$/;

[Konstantin Tokarev]

Oops, ignore this.