[webkit-reviews] review denied: [Bug 31799] run-webkit-tests doesn't accept directories/files with --skipped=only parameter : [Attachment 43700] proposed patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Csaba Osztrogonac
<ossy at webkit.org>'s request for review:
Bug 31799: run-webkit-tests doesn't accept directories/files with
--skipped=only parameter
https://bugs.webkit.org/show_bug.cgi?id=31799

Attachment 43700: proposed patch
https://bugs.webkit.org/attachment.cgi?id=43700&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> 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.