Bug 31799 – run-webkit-tests doesn't accept directories/files with --skipped=only parameter

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

If you call run-webkit-tests script with --skipped=only parameter,
you can't add directories as constraint, but script runs all of
the skipped tests. It can take a long time on Qt and GTK port,
because there are scores of skipped tests. Or it can go into
an infinite loop because of a bug.

[Csaba Osztrogonác]

@@ -2106,7 +2116,13 @@ sub readSkippedFiles
                 if ($skipped && $skipped !~ /^#/) {
                     if ($skippedOnly) {
                         if (!&fileShouldBeIgnored($skipped)) {
-                            push(@ARGV, $skipped);
+                            if (!$path) {
+                                push(@ARGV, $skipped);
+                            } elsif ($skipped =~/^($path)/) {
+                                push(@ARGV, $skipped);
+                            } elsif ($path =~/^($skipped)/) {
+                                push(@ARGV, $path);
+                            }
                         } elsif ($verbose) {
                             print "    $skipped\n";
                         }

1st case: Original case, if you simple call "run-webkit-tests --skipped-only"

2nd case: If you call "run-webkit-tests --skipped=only SKIPPED_DIR" and there is a SKIPPED_DIR/skipped_file on skiplist, then SKIPPED_DIR/skipped_file will be pushed.

3rd case: If you call "run-webkit-tests --skippes=only dir1/dir2/dir3" and dir1 is on skiplist, then dir1/dir2/dir3 will be pushed.

+    # Remove duplicated tests
+    @testsToRun = keys %{{ map { $_ => 1 } @testsToRun }};

It is necessary if there are duplicated entries on skiplist.
(eg. dir1/dir2/dir3 and dir1/dir2) It won't affect --iterations and --repeat-each parameters.

[Csaba Osztrogonác]

Created attachment 43700 [details]
proposed patch

[David Kilzer (:ddkilzer)]

Comment on attachment 43700 [details]
proposed patch

> diff --git a/WebKitTools/Scripts/run-webkit-tests b/WebKitTools/Scripts/run-webkit-tests
> index 1e37286..f339a76 100755
> --- a/WebKitTools/Scripts/run-webkit-tests
> +++ b/WebKitTools/Scripts/run-webkit-tests
> @@ -455,7 +454,16 @@ if (!checkWebCoreWCSSSupport(0)) {
>  }
>  
>  processIgnoreTests($ignoreTests, "ignore-tests") if $ignoreTests;
> -readSkippedFiles() unless $ignoreSkipped;
> +unless ($ignoreSkipped)
> +{

I think this reads better:

    if (!$ignoreSkipped) {

I'm not a big fan of 'unless', and I find it harder to read when used in place of an 'if' block.

The curly brace should be on the same line as the unless/if statement as well.

> +    if (!$skippedOnly || $#ARGV == -1) {

I think it's clearer to check the scalar value of @ARGV to see if it contains elements:

    if (!$skippedOnly || @ARGV == 0) {

> +        readSkippedFiles("");
> +    } else {
> +        foreach my $argnum (0 .. $#ARGV) {
> +            readSkippedFiles(shift);
> +        }

The foreach statement needs a comment about how it's preventing an infinite loop by only iterating over the original @ARGV list (since readSkippedFiles() appends to @ARGV).  Maybe:

    # Since readSkippedFiles() appends to @ARGV, we must use a foreach
    # loop so that we only iterate over the original argument list.

Using the implicit form of 'shift' on @ARGV was also confusing.  (I expected shift to operate on @_, but I didn't realize it would fall back to @ARGV in this case since there was no @_ in this context.)  I think this would read better:

    readSkippedFiles(shift @ARGV);

> @@ -2089,8 +2097,10 @@ sub fileShouldBeIgnored
>      return 0;
>  }
>  
> -sub readSkippedFiles
> +sub readSkippedFiles($)
>  {
> +    my ($path) = @_;

A better name might be $constraintPath since we're using it to constrain the skipped list entries.

> +
>      foreach my $level (@platformTestHierarchy) {
>          if (open SKIPPED, "<", "$level/Skipped") {
>              if ($verbose) {
> @@ -2106,7 +2116,13 @@ sub readSkippedFiles
>                  if ($skipped && $skipped !~ /^#/) {
>                      if ($skippedOnly) {
>                          if (!&fileShouldBeIgnored($skipped)) {

Bonus points for removing the unneeded '&' in the 'if' statement above.  Not necessary though.

> -                            push(@ARGV, $skipped);

Overall, it would be nice to add a comment in each if/elsif/elsif block below that describes what's happening.

> +                            if (!$path) {

Perhaps:

    # Always add $skipped since no constraint path was specified on the command line.

> +                                push(@ARGV, $skipped);
> +                            } elsif ($skipped =~/^($path)/) {

Perhaps:

    # Add $skipped only if it matches the current path constraint, e.g.,
    # "--skipped=only dir1" with "dir1/file1.html" on the skipped list.

Please add a space after the "=~" operator.

> +                                push(@ARGV, $skipped);
> +                            } elsif ($path =~/^($skipped)/) {

Perhaps:

    # Add current path constraint if it is more specific than the skip list entry,
    # e.g., "--skipped=only dir1/dir2/dir3" with "dir1" on the skipped list.

Please add a space after the "=~" operator.

> +                                push(@ARGV, $path);
> +                            }
>                          } elsif ($verbose) {
>                              print "    $skipped\n";
>                          }
> @@ -2177,6 +2193,9 @@ sub findTestsToRun
>          }
>      }
>  
> +    # Remove duplicated tests
> +    @testsToRun = keys %{{ map { $_ => 1 } @testsToRun }};

Typo: use "duplicate" instead of "duplicated"

r- to address minor issues.

[Csaba Osztrogonác]

(In reply to comment #3)

Thanks for your reflections and advices. The modified patch is submitted.
Sorry for coding style violations. I will regard lots better next time.

> >      foreach my $level (@platformTestHierarchy) {
> >          if (open SKIPPED, "<", "$level/Skipped") {
> >              if ($verbose) {
> > @@ -2106,7 +2116,13 @@ sub readSkippedFiles
> >                  if ($skipped && $skipped !~ /^#/) {
> >                      if ($skippedOnly) {
> >                          if (!&fileShouldBeIgnored($skipped)) {
> 
> Bonus points for removing the unneeded '&' in the 'if' statement above.  Not
> necessary though.
I didn't find unneeded if. Which do you mean?

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> (In reply to comment #3)
> 
> Thanks for your reflections and advices. The modified patch is submitted.
> Sorry for coding style violations. I will regard lots better next time.

Some of it is personal preference, too.  I'm not sure all of this is codified in our style documentation (since that mostly reflects C++ code).

> > >                          if (!&fileShouldBeIgnored($skipped)) {
> > 
> > Bonus points for removing the unneeded '&' in the 'if' statement above.  Not
> > necessary though.
> 
> I didn't find unneeded if. Which do you mean?

I meant changing this:

-                         if (!&fileShouldBeIgnored($skipped)) {
+                         if (!fileShouldBeIgnored($skipped)) {

The '&' operator is unnecessary in this case (and is a remnant of an earlier Perl style).

[Csaba Osztrogonác]

Created attachment 43865 [details]
updated patch

[Csaba Osztrogonác]

(In reply to comment #6)
> The '&' operator is unnecessary in this case (and is a remnant of an earlier
> Perl style).

OK, I removed it. And submitted the updated patch that I forgot previously. :)

[David Kilzer (:ddkilzer)]

Comment on attachment 43865 [details]
updated patch

r=me  Thanks!

[Csaba Osztrogonác]

(In reply to comment #8)
> (From update of attachment 43865 [details])
> r=me  Thanks!

Sending        WebKitTools/ChangeLog
Sending        WebKitTools/Scripts/run-webkit-tests
Transmitting file data ..
Committed revision 51395.