Bug 97276 – modify old-run-webkit-tests to support TestExpectations files a little

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

modify old-run-webkit-tests to support TestExpectations files a little

[Dirk Pranke]

Created attachment 165021 [details]
Patch

[Dirk Pranke]

Once this patch lands, I plan to go through the existing Skipped files, merge their entries into the TestExpectations files, and then delete the Skipped files.

Once all that's done, I will remove support for Skipped files from NRWT, and I think we can safely start dropping the "new-" from NRWT.

[Ryosuke Niwa]

Adding some perl devs.

[Ryosuke Niwa]

Comment on attachment 165021 [details]
Patch

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

> Tools/Scripts/old-run-webkit-tests:2608
> +                    if (!fileShouldBeIgnored($skipped)) {
> +                        if (!$constraintPath) {
> +                            push(@ARGV, $skipped);
> +                        } elsif ($skipped =~ /^($constraintPath)/ || ("LayoutTests/".$skipped) =~ /^($constraintPath)/ ) {
> +                            push(@ARGV, $skipped);
> +                        } elsif ($constraintPath =~ /^("LayoutTests\/".$skipped)/ || $constraintPath =~ /^($skipped)/) {
> +                            push(@ARGV, $constraintPath);
> +                        }
> +                    } elsif ($verbose) {
> +                        print "    $skipped\n";
> +                    }
> +                } else {
> +                    if ($verbose) {
> +                        print "    $skipped\n";
> +                    }
> +                }

Rather than duplicating code, can we extract a routine?

[Dirk Pranke]

Comment on attachment 165021 [details]
Patch

Sure. Patch coming shortly.

[Dirk Pranke]

Created attachment 165034 [details]
use common code for --ignore flags, ignore old-style syntax entries for robustness

[Eric Seidel (no email)]

dbates is your perl man these days.

[Daniel Bates]

Comment on attachment 165034 [details]
use common code for --ignore flags, ignore old-style syntax entries for robustness

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

> Tools/Scripts/old-run-webkit-tests:2540
> +                # everything.  We also don't do any error checking since we assume
> +                # check-webkit-style and new-run-webkit-tests would have caught the errors already.

For the second sentence, I take it you are talking about the EWS bots, that run check-webkit-style and new-run-webkit-tests, catching such errors, right?

> Tools/Scripts/old-run-webkit-tests:2542
> +                # there's a need for it

Nit: Missing a period at the end of this sentence.

> Tools/Scripts/old-run-webkit-tests:2547
> +                    # ignore any lines with the old-style syntax.

Nit: ignore => Ignore

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

This code is syntactically incorrect. I take it you meant to write the first disjunction as:

index($token, "BUG") != -1

(*) Additionally, when the if conditional evaluates to true the code in the associated if block only skips current token instead of the entire line. This behavior disagrees with the comment above the if-statement.

On another note, is it necessary for us to handle the old style syntax? Briefly searching through the TestExpectations files, I couldn't find any usage of the modifier BUG and the only usage of C++-style comments is in file LayoutTests/platform/wk2/TestExpectations, <http://trac.webkit.org/browser/trunk/LayoutTests/platform/wk2/TestExpectations>. Moreover, we don't properly parse LayoutTests/platform/wk2/TestExpectations because of (*). I know that you mentioned that this code doesn't perform error checking. It seems wrong to continue processing the line when we encounter the start of a C++-style comment given that we added code to ignore the old style syntax.

> Tools/Scripts/old-run-webkit-tests:2551
> +                    # Note we don't bother looking for chromium or v8 bugs

Nit: Note => "Note, " (notice the presence of the comma and space characters after "Note").

AND

chromium => Chromium
v8 => V8

(since these are proper nouns)

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

Notice that index() returns whether the search string is a substring of the specified string. In _tokenize_line(), we check that the token is prefixed with the search string, <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py?rev=129265#L286>. Did you intend to make this conditional more lenient than its counterpart in _tokenize_line() for these bug types?

[Dirk Pranke]

Thanks for the detailed review!

(In reply to comment #8)
> (From update of attachment 165034 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165034&action=review
> 
> > Tools/Scripts/old-run-webkit-tests:2540
> > +                # everything.  We also don't do any error checking since we assume
> > +                # check-webkit-style and new-run-webkit-tests would have caught the errors already.
> 
> For the second sentence, I take it you are talking about the EWS bots, that run check-webkit-style and new-run-webkit-tests, catching such errors, right?
> 

I was speaking more generally, thinking that nrwt is much more widely used than orwt these days and so it's unlikely that errors in a TestExpectations file will be caught by someone running orwt first (unless of couse that person introduced the error locally).

> > Tools/Scripts/old-run-webkit-tests:2542
> > +                # there's a need for it
> 
> Nit: Missing a period at the end of this sentence.
> 

Will fix.

> > Tools/Scripts/old-run-webkit-tests:2547
> > +                    # ignore any lines with the old-style syntax.
> 
> Nit: ignore => Ignore
> 

Will fix.

> > Tools/Scripts/old-run-webkit-tests:2548
> > +                    if (index($token, "BUG" != -1) || index($token, "//") != -1) {
> 
> This code is syntactically incorrect. I take it you meant to write the first disjunction as:
> 
> index($token, "BUG") != -1
> 

Yes. Puzzling; I'm fairly sure I've tested this after running this line and I'm not sure why this would've parsed.

> (*) Additionally, when the if conditional evaluates to true the code in the associated if block only skips current token instead of the entire line. This behavior disagrees with the comment above the if-statement.
>

Oh, good catch. The intent is to skip the whole line.
 
> On another note, is it necessary for us to handle the old style syntax? Briefly searching through the TestExpectations files, I couldn't find any usage of the modifier BUG and the only usage of C++-style comments is in file LayoutTests/platform/wk2/TestExpectations, <http://trac.webkit.org/browser/trunk/LayoutTests/platform/wk2/TestExpectations>. Moreover, we don't properly parse LayoutTests/platform/wk2/TestExpectations because of (*). I know that you mentioned that this code doesn't perform error checking. It seems wrong to continue processing the line when we encounter the start of a C++-style comment given that we added code to ignore the old style syntax.
> 

Right, that would definitely be wrong. It's not intended to handle the old syntax, just to not do something weird. If we didn't special-case the old syntax we can treat "BUG(x)" and/or "//" as test names and get confused down the line.

> > Tools/Scripts/old-run-webkit-tests:2551
> > +                    # Note we don't bother looking for chromium or v8 bugs
> 
> Nit: Note => "Note, " (notice the presence of the comma and space characters after "Note").
> 
> AND
> 
> chromium => Chromium
> v8 => V8
> 
> (since these are proper nouns)
> 

Will fix.

> > Tools/Scripts/old-run-webkit-tests:2552
> > +                    if (index($token, "webkit.org/b/") != -1 || index($token, "Bug(") != -1) {
> 
> Notice that index() returns whether the search string is a substring of the specified string. In _tokenize_line(), we check that the token is prefixed with the search string, <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py?rev=129265#L286>. Did you intend to make this conditional more lenient than its counterpart in _tokenize_line() for these bug types?

No, that was not the intent, so good catch. I will change it to == 0 in both cases (although it does seem unlikely that these substrings would show up anywhere else).

[Dirk Pranke]

Created attachment 165444 [details]
update w/ review feedback

[Daniel Bates]

Comment on attachment 165444 [details]
update w/ review feedback

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().

[Dirk Pranke]

(In reply to comment #11)
> (From update of attachment 165444 [details])
> 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.
> 

Will fix.

> > 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".
>

Will fix.
 
> > 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, "//")
> 

Will do.

> 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.
> 

I have a strong and long-standing dislike of using $_ for anything :). Not very perl-like of me, I know.

> > 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.
> 

Will do.

> > 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.
> 

I know, but I consider them clearer :)

> > 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().

ok.

[Dirk Pranke]

Created attachment 165907 [details]
Patch

[Dirk Pranke]

Committed r129717: <http://trac.webkit.org/changeset/129717>

[Eric Seidel (no email)]

Comment on attachment 165907 [details]
Patch

Cleared review? from attachment 165907 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).