| Summary: | Access expectations path through apple_additions | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||
| Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | aakash_jain, ap, buildbot, commit-queue, dbates, ddkilzer, glenn, lforschler, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Jonathan Bedard
2017-07-24 15:22:06 PDT
Created attachment 316324 [details]
Patch
I spoke with Alexey today (8/22). He wanted unit tests to confirm this change. Created attachment 318832 [details]
Patch
Comment on attachment 318832 [details] Patch Clearing flags on attachment: 318832 Committed r221079: <http://trac.webkit.org/changeset/221079> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 318882 [details]
Patch
Comment on attachment 318882 [details]
Patch
r=me
Comment on attachment 318882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318882&action=review > Tools/Scripts/webkitpy/port/ios.py:103 > + os_index = -1 > + for i in xrange(2): > + if split_name[os_index] == 'wk1' or split_name[os_index] == 'wk2' or split_name[os_index] == 'simulator' or split_name[os_index] == 'device': > + os_index -= 1 > + if split_name[os_index] != split_name[0]: > + os_name = apple_additions().mac_os_name(split_name[os_index]) > if not os_name: > return None > - name = split_name[0] + '-' + os_name + ('-' + '-'.join(split_name[2:]) if len(split_name) > 2 else '') > + split_name[os_index] = os_name > + name = '-'.join(split_name) This logic seems so circuitous. I mean, the caller knows the components (e.g. "wk2" and "device"), the caller concatenates these components to form a path only for this code to split the path. (In reply to WebKit Commit Bot from comment #5) > Comment on attachment 318832 [details] > Patch > > Clearing flags on attachment: 318832 > > Committed r221079: <http://trac.webkit.org/changeset/221079> I do not understand the need to the interleave the additional platform directories with the platform directories when looking for test baselines. An example would have been helpful. The description of this bug, the ChangeLog message, and <rdar://problem/33498899> do not provide an example or elaborate further on why "the resulting precedence will often be wrong" using the approach prior to this change and why the resulting precedence will be right after this change. It is also weird that the behavior of --additional-platform-directory now differs depending on what port you are using. If you are using Mac, iOS or Windows then any --additional-platform-directory will be interleaved with the regular platform directories. For all other ports, --additional-platform-directory will take precedence over the platform directories. Regardless, the description for --additional-platform-directory in "run-webkit-tests --help" is now out-of-date. We should update this description to add a remark to explain the Mac, iOS, and Windows-specific behavior. Comment on attachment 318882 [details]
Patch
cq-'ing since the help description for --additional-platform-directory is out-of-date.
(In reply to Daniel Bates from comment #11) > It is also weird that the behavior of --additional-platform-directory now > differs depending on what port you are using. If you are using Mac, iOS or > Windows then any --additional-platform-directory will be interleaved with > the regular platform directories. For all other ports, > --additional-platform-directory will take precedence over the platform > directories. > > Regardless, the description for --additional-platform-directory in > "run-webkit-tests --help" is now out-of-date. We should update this > description to add a remark to explain the Mac, iOS, and Windows-specific > behavior. (In reply to Daniel Bates from comment #12) > Comment on attachment 318882 [details] > Patch > > cq-'ing since the help description for --additional-platform-directory is > out-of-date. Disregard these remarks. Comment on attachment 318882 [details] Patch Clearing flags on attachment: 318882 Committed r221087: <http://trac.webkit.org/changeset/221087> All reviewed patches have been landed. Closing bug. Committed r221089: <http://trac.webkit.org/changeset/221089> |