Bug 185054

Summary: REGRESSION (r229955): run-webkit-tests runs tests in skipped directories and subdirectories of directory given on command line
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, glenn, jbedard, lforschler, rniwa, webkit-bug-importer
Priority: P1 Keywords: InRadar, Regression
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=66308
Bug Depends on: 183681    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch and unit tests rniwa: review+

Daniel Bates
Reported 2018-04-26 15:02:34 PDT
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/>.
Attachments
Patch (2.04 KB, patch)
2018-05-04 11:10 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Patch and unit tests (6.42 KB, patch)
2018-05-04 11:32 PDT, Daniel Bates 		rniwa: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Daniel Bates
Comment 1 2018-04-26 15:04:52 PDT
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
Comment 2 2018-04-26 15:13:17 PDT
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
Comment 3 2018-04-26 15:13:48 PDT
<rdar://problem/39773209>
Daniel Bates
Comment 4 2018-04-26 16:21:13 PDT
This regressed with the change in <https://trac.webkit.org/changeset/229955/> (bug #183681).
Daniel Bates
Comment 5 2018-04-26 16:48:30 PDT
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
Comment 6 2018-05-04 11:10:21 PDT
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
Comment 7 2018-05-04 11:16:01 PDT
EWS reveals that there is a way to test this behavior change.
Daniel Bates
Comment 8 2018-05-04 11:32:23 PDT
Created attachment 339574 [details] Patch and unit tests
Jonathan Bedard
Comment 9 2018-05-04 14:01:01 PDT
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
Comment 10 2018-05-07 11:30:55 PDT
(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
Comment 11 2018-05-07 11:33:00 PDT
Committed r231449: <https://trac.webkit.org/changeset/231449>
Note You need to log in before you can comment on or make changes to this bug.