Bug 174800 – Access expectations path through apple_additions

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

[Jonathan Bedard]

When expectation files are added through --additional-platform-directory, the resulting precedence will often be wrong.  For Mac, Windows and iOS, use apple_additions to determine any additional test expectations.

[Radar WebKit Bug Importer]

<rdar://problem/33498899>

[Jonathan Bedard]

Created attachment 316324 [details]
Patch

[Jonathan Bedard]

I spoke with Alexey today (8/22).  He wanted unit tests to confirm this change.

[Jonathan Bedard]

Created attachment 318832 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 318832 [details]
Patch

Clearing flags on attachment: 318832

Committed r221079: <http://trac.webkit.org/changeset/221079>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 318882 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 318882 [details]
Patch

r=me

[Daniel Bates]

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.

[Daniel Bates]

(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.

[Daniel Bates]

Comment on attachment 318882 [details]
Patch

cq-'ing since the help description for --additional-platform-directory is out-of-date.

[Daniel Bates]

(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.

[WebKit Commit Bot]

Comment on attachment 318882 [details]
Patch

Clearing flags on attachment: 318882

Committed r221087: <http://trac.webkit.org/changeset/221087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

Committed r221089: <http://trac.webkit.org/changeset/221089>