Bug 27893 – run-webkit-tests Skipped Only mode includes tests that are skipped by other flags.

• Component: New Bugs
• Status: RESOLVED FIXED

[Mike Fenton]

Current when run-webkit-tests is run with the --skipped=only option all tests currently in the Skipped file for the platform are run without consideration for other flags like -no-http or platform defaults for the tests configurations.

The current mode of --skipped=only should be transitioned to a new mode (--skipped=only-all) which maintains the current behavior.

--skipped=only should be updated to correctly use the existing flags in the script to only run skipped tests that the functionality exists to potentially pass the test.

This update will match the behavior of --skipped=ignore which does obey flags used by run-webkit-tests.

[Mike Fenton]

Created attachment 33899 [details]
Patch to improve the behavior of --skipped=only and add --skipped=only-all mode to run-webkit-tests

[Eric Seidel (no email)]

Comment on attachment 33899 [details]
Patch to improve the behavior of --skipped=only and add --skipped=only-all mode to run-webkit-tests

I'm not sure only-all is useful, maybe it is.

[Mike Fenton]

I tend to agree, however, I didn't want to remove the old functionality entirely.

[Adam Treat]

I happen to agree with Eric.  This is a bug fix pure and simple.  No need for --only-all which is confusing to me.

[Mike Fenton]

Created attachment 34059 [details]
Updated patch without skipped=only-all mode

I've updated the patch to strictly fix the --skipped=only mode.  No only-all mode is included in this patch.

[Adam Treat]

Comment on attachment 34059 [details]
Updated patch without skipped=only-all mode

> [phone]  Mike Fenton  <[e-mail]>
> +
> +        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.

[Mike Fenton]

Created attachment 34064 [details]
Updated changeLog and rename of function.

This patch makes the suggested changes to the previous patch.

1) Rename the function.

2) Update the changelog to more clearly identify the reason for the change.

[Adam Barth]

Comment on attachment 34064 [details]
Updated changeLog and rename of function.

Clearing review flag on attachment: 34064

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/run-webkit-tests
Committed r46771
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/run-webkit-tests
r46771 = 042145e38e9fb864caea4ce6b7799aa60e67b093 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46771

[Adam Barth]

All reviewed patches have been landed.  Closing bug.