[webkit-reviews] review denied: [Bug 27893] run-webkit-tests Skipped Only mode includes tests that are skipped by other flags. : [Attachment 34059] Updated patch without skipped=only-all mode

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 4 07:58:39 PDT 2009


Adam Treat <treat at kde.org> has denied Mike Fenton
<mike.fenton at torchmobile.com>'s request for review:
Bug 27893: run-webkit-tests Skipped Only mode includes tests that are skipped
by other flags.
https://bugs.webkit.org/show_bug.cgi?id=27893

Attachment 34059: Updated patch without skipped=only-all mode
https://bugs.webkit.org/attachment.cgi?id=34059&action=review

------- Additional Comments from Adam Treat <treat at kde.org>
> +2009-07-31  Mike Fenton  <mike.fenton at torchmobile.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Change --skipped=only mode to use $ignoreDirectories to only
> +	   run tests that configured by the script, or by the default
> +	   values used in the script.

I think the ChangeLog could do a better job of explaining what is happening
here.  Basically, you want --skipped=only to honor the --no-http flag.	That is
not currently happening.  If I understand things correctly, your patch
explicitly fixes this by making --skipped=only honor the ignoreDirectories, one
of which is 'http' if --no-http is passed.

Spell this out as the explicit goal in the ChangeLog and how it was
accomplished.

Also, it is not clear to me that this patch accomplishes what the bug report
says it does: to make --skipped=only honor *all* other flags.  It seems that
this bugfix was rather narrowly tailored to --no-http.	Which is fine!	But it
is good to be clear about this.

> +sub ignoreFileByDirectory {
> +    my($filePath) = @_;
> +    foreach my $ignoredDir (keys %ignoredDirectories) {
> +	   if ($filePath =~ m/^$ignoredDir/) {
> +	       return 1;
> +	   }
> +    }

Perhaps 'fileShouldBeIgnored' instead to be more descriptive?

r- for ChangeLog mostly.


More information about the webkit-reviews mailing list