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.
@@ -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.
Created attachment 43700 [details] proposed patch
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.
(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?
(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).
Created attachment 43865 [details] updated patch
(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. :)
Comment on attachment 43865 [details] updated patch r=me Thanks!
(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.