[Webkit-unassigned] [Bug 31799] run-webkit-tests doesn't accept directories/files with --skipped=only parameter

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=31799


David Kilzer (ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #43700|review?                     |review-
               Flag|                            |




--- Comment #3 from David Kilzer (ddkilzer) <ddkilzer at webkit.org>  2009-11-23 13:34:26 PST ---
(From update of attachment 43700)
> 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.