Bug 185054 – REGRESSION (r229955): run-webkit-tests runs tests in skipped directories and subdirectories of directory given on command line

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

According the description of --skipped in run-webkit-tests --help, when --skipped is omitted run-webkit-tests should "skip tests [marked SKIP explicitly or implicitly because they are in a directory marked SKIP (*)] unless explicitly listed on the command line". 

Steps to reproduce:

The following steps assume you are running the tests on a Mac and that http/tests/quicklook is skipped on Mac, which it is at the time of writing it is by <https://trac.webkit.org/browser/trunk/LayoutTests/TestExpectations?rev=231065#L110>. (http/tests/quicklook is skipped on all non-iOS ports).

1. Run `Tools/Scripts/run-webkit-tests http/tests/quicklook`.

Then run-webkit-tests will run the tests in http/tests/quicklook. But it should not have because --skipped was not given and http/test/quicklook is skipped on Mac.

Similarly, running `Tools/Scripts/run-webkit-tests http/tests` will run tests in http/tests/quicklook. But it should not have by the same aforementioned reason.

(*) This criterion is implied by the second paragraph in the ChangeLog that added --skipped: <https://trac.webkit.org/changeset/120348/>.

[Daniel Bates]

On another note, run-webkit-tests does not seem to honor --skipped. Running `Tools/Scripts/run-webkit-tests --skipped=always http/tests/quicklook` runs the tests. But it should not because the directory http/tests/quicklook is skipped on Mac.

[Daniel Bates]

This bugs significantly impacts my development because I often test correctness of a change in select directories to save time and build confidence before running all the tests. Having run-webkit-tests run tests that should be skipped when I invoke it with a selection of directories makes it difficult to differentiate between regressions I caused (if any) from test failures associated with skipped tests that are being run.

[Radar WebKit Bug Importer]

<rdar://problem/39773209>

[Daniel Bates]

This regressed with the change in <https://trac.webkit.org/changeset/229955/> (bug #183681).

[Daniel Bates]

If I remove <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/port/mac.py?rev=230998#L63> and <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/port/mac.py?rev=230998#L64> then the bug does not occur when I run `Tools/Scripts/run-webkit-tests http/tests/quicklook`.

[Daniel Bates]

Created attachment 339568 [details]
Patch

I did not write a test because the port's OS version is a private field as it represents an implementation detail and I did not feel the risk of regression was great enough to justify exposing this detail. Moreover, I was unclear how I could indirectly test this change by way of MacPort.default_baseline_search_path().

[Daniel Bates]

EWS reveals that there is a way to test this behavior change.

[Daniel Bates]

Created attachment 339574 [details]
Patch and unit tests

[Jonathan Bedard]

Comment on attachment 339574 [details]
Patch and unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=339574&action=review

Looks sane to me.  Unofficial r+.

> Tools/Scripts/webkitpy/port/mac_unittest.py:163
> +        self.assertEqual(port.version_name(), VersionNameMap.map(port.host.platform).to_name(MacPort.CURRENT_VERSION, platform=MacPort.port_name))

We shouldn't need the platform passed to VersionNameMap.map() and passed to VersionNameMap.to_name(...).  One of the two should suffice.

[Daniel Bates]

(In reply to Jonathan Bedard from comment #9)
> > Tools/Scripts/webkitpy/port/mac_unittest.py:163
> > +        self.assertEqual(port.version_name(), VersionNameMap.map(port.host.platform).to_name(MacPort.CURRENT_VERSION, platform=MacPort.port_name))
> 
> We shouldn't need the platform passed to VersionNameMap.map() and passed to
> VersionNameMap.to_name(...).  One of the two should suffice.

Will change VersionNameMap expression in this line and throughout the patch to read:

VersionNameMap().to_name(MacPort.CURRENT_VERSION, platform=MacPort.port_name)

Additional remarks:

VersionNameMap is so confusing to use (see bug #185386). I counted at least 8 ways to map a version number to name! Filed bug #185387.

[Daniel Bates]

Committed r231449: <https://trac.webkit.org/changeset/231449>