[webkit-reviews] review granted: [Bug 97276] modify old-run-webkit-tests to support TestExpectations files a little : [Attachment 165444] update w/ review feedback

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Dirk Pranke
<dpranke at chromium.org>'s request for review:
Bug 97276: modify old-run-webkit-tests to support TestExpectations files a
little
https://bugs.webkit.org/show_bug.cgi?id=97276

Attachment 165444: update w/ review feedback
https://bugs.webkit.org/attachment.cgi?id=165444&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165444&action=review


> Tools/ChangeLog:20
> +	   * Scripts/old-run-webkit-tests:
> +	   (readSkippedFiles):

This change log entry is out-of-date. Specifically, it doesn't mention the
added function processSkippedFileEntry.

> Tools/Scripts/old-run-webkit-tests:2537
> +		   # This logic roughly mirrors the logic in
test_expectations.py _tokenize_line but we

Nit: For your consideration, I suggest that we write "_tokenize_line" as
"_tokenize_line()" to make it explicit that this is a function even though it's
implied by the phrase "the logic in".

> Tools/Scripts/old-run-webkit-tests:2543
> +		       if ((index($token, "BUG") == 0) || (index($token, "//")
== 0)) {

You may want to consider encapsulating "index(..., ...) == 0" into a
convenience function, say startsWith, so as to improve the readability of this
function. startsWith() would take a string S and a prefix P and return whether
S begins with P. Then we can write the if condition of this line as:

startsWith($token, "BUG") || startsWith($token, "//")

Alternatively, we could use $_	and regular expressions to write the TOKEN
labeled foreach loop as:

TOKEN: foreach (split(/\s+/, $line)) {
    if (/^BUG/ || m|^//|) {
	# Ignore any lines with the old-style syntax.
	next LINE;
    }
    ...
}

You can see examples of using $_ and regular expressions for prefix matching in
tools/Scripts/VCSUtils.pm. In particular,  VCSUtils::parseSvnDiffHeader().
Notice the use of m|| (above) so as to avoid escaping each '/' in "//". See
<http://perldoc.perl.org/perlop.html#Regexp-Quote-Like-Operators> and
<http://perldoc.perl.org/perlop.html#Quote-and-Quote-like-Operators> for more
details.

> Tools/Scripts/old-run-webkit-tests:2547
> +		       if ((index($token, "webkit.org/b/") == 0) ||
(index($token, "Bug(") == 0)) {

See my remark above on some ideas you may want to consider so as to improve the
readability of this line.

> Tools/Scripts/old-run-webkit-tests:2551
> +		       } elsif ($token eq '[') {

Please use double quotes around string literals instead of single quotes. We
tend to use double quotes around string literals even when the contents don't
need to be interpolated. Using double quotes instead of single quotes for
string literals will also make this line match the quote style we've used up to
this point in the patch.

> Tools/Scripts/old-run-webkit-tests:2552
> +			   if ($state eq 'start') {

Please use double quotes around string literals instead of single quotes.

> Tools/Scripts/old-run-webkit-tests:2553
> +			       $state = 'configuration';

Ditto.

> Tools/Scripts/old-run-webkit-tests:2555
> +		       } elsif ($token eq ']') {

Ditto.

> Tools/Scripts/old-run-webkit-tests:2556
> +			   if ($state eq 'configuration') {

Ditto.

> Tools/Scripts/old-run-webkit-tests:2557
> +			       $state = 'name';

Ditto.

> Tools/Scripts/old-run-webkit-tests:2559
> +		       } elsif (($state eq 'name') || ($state eq 'start')) {

Ditto; Also, the inner parentheses (around each disjunct) aren't necessary.

> Tools/Scripts/old-run-webkit-tests:2576
> +    my ($skipped, $basename, $constraintPath) = @_;

Maybe a more descriptive name for $basename would be $listname? This would
match the name of the parameter with the same purpose in processIgnoreTests().