Bug 192162 – webkitpy: Implement device type specific expected results

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

[Jonathan Bedard]

It is often the case that a test will have expectations which differ based on the device type the test is being run on. We should optionally break up test expectations into folders with device specific expectations contained within.

[Radar WebKit Bug Importer]

<rdar://problem/46345449>

[Jonathan Bedard]

Created attachment 356960 [details]
Patch

[EWS Watchlist]

Comment on attachment 356960 [details]
Patch

Attachment 356960 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10338667

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 356967 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Jonathan Bedard]

Created attachment 357055 [details]
Directory Approach

[Jonathan Bedard]

Created attachment 357148 [details]
Patch

[Jonathan Bedard]

Created attachment 357149 [details]
Patch

[EWS Watchlist]

Comment on attachment 357149 [details]
Patch

Attachment 357149 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10370859

New failing tests:
fast/text/ipad/bold-tall-body-text-style.html
fast/text-autosizing/ios/ipad/text-size-adjust-inline-style-expected-ipad.html
fast/text/ipad/bold-tall-body-text-style-expected-mismatch-ipad.html

[EWS Watchlist]

Created attachment 357154 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Jonathan Bedard]

Created attachment 357170 [details]
Patch

[Jonathan Bedard]

Created attachment 357194 [details]
Patch

[EWS Watchlist]

Comment on attachment 357194 [details]
Patch

Attachment 357194 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10376839

New failing tests:
ietestcenter/css3/namespaces/prefix-010.xml
ietestcenter/css3/namespaces/prefix-011.xml
ietestcenter/css3/namespaces/prefix-007.xml

[EWS Watchlist]

Created attachment 357205 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[Jonathan Bedard]

Created attachment 357231 [details]
Patch

[Jonathan Bedard]

I think it's worth sharing some results from local testing here: device type specific expectations will definitely be a rarity. I ran all our iOS tests on an iPad instead of an iPhone SE, and encountered 96 failures. Assuming all these failures are a result of running on iPad instead of iPhone (which is doubtful, but it gives us a pretty good estimate). This means that, including our current device specific tests, we have 118 tests currently checked in that require device specific expected results.

[Jonathan Bedard]

Created attachment 357275 [details]
Patch

[Jonathan Bedard]

After discussions, the approach this patch is taking is going to change quite dramatically. Instead of introducing device type specific expected results, the baseline search path will be mutated based on the device type used.

[Jonathan Bedard]

Created attachment 359090 [details]
Patch

[EWS Watchlist]

Attachment 359090 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/port/base.py:1204:  [Port.expectations_files] An attribute affected in webkitpy.port.base_unittest line 190 hide this method  [pylint/E0202] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

(In reply to Build Bot from comment #19)
> Attachment 359090 [details] did not pass style-queue:
> 
> 
> ERROR: Tools/Scripts/webkitpy/port/base.py:1204:  [Port.expectations_files]
> An attribute affected in webkitpy.port.base_unittest line 190 hide this
> method  [pylint/E0202] [5]
> Total errors found: 1 in 16 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Normally this message would be helpful. In this case, Python is complaining about a function being overwritten during testing.

[WebKit Commit Bot]

Comment on attachment 359090 [details]
Patch

Clearing flags on attachment: 359090

Committed r239989: <https://trac.webkit.org/changeset/239989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 359383 [details]
Patch

[Lucas Forschler]

Comment on attachment 359383 [details]
Patch

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

Bundling the layout test changes along with this change makes the patch huge. I understand they need to land at the same time, but it might be worth pulling them into separate patches and landing sequentially. That would break this up a bit to be more readable, but would cause at least one run to fail.

> Tools/ChangeLog:11
> +        that while multiple baseline search paths are used, any single test will only ever be run on single device type.

run on 'a' single device type?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:589
> +

This block of code looks like it us used more than once. (I see it above in another function). it might be worth exploring making it re-usable in it's own function.

> Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py:239
> +        # This works because tests are run on the first device type which won't skip them, reguardless of other expectations, and never re-run.

typo: regardless

[Jonathan Bedard]

Comment on attachment 359383 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:589
>> +
> 
> This block of code looks like it us used more than once. (I see it above in another function). it might be worth exploring making it re-usable in it's own function.

The obvious choice is to put it in self._collect_tests(...). The problem is, we need a function which returns a list of paths and test_names if successful, but RunDetails if it fails. I think that such a function is more confusing than it is helpful.

[Aakash Jain]

Comment on attachment 359383 [details]
Patch

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

rs=me with few comments.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:95
>      def _collect_tests(self, args):

It would probably be more clear to pass device_type as an argument (with default value of None), and have the caller pass self.current_device_type to device_type.

That would eliminate the possible error of calling this method before setting appropriate value to self.current_device_type.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:110
>      def _prepare_lists(self, paths, test_names):

It would probably be more clear to pass device_type as an argument (with default value of None), and have the caller pass self.current_device_type to device_type.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:184
> +        for device_type in device_type_list:

it seems like None is a valid value for device_type (since device_type_list can be [None], as per supported_device_types()). Please make sure it is intentional and works fine.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:554
>      def _print_expectation_line_for_test(self, format_string, test):

Ditto.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:558
> +    def _print_expectations_for_subset(self, test_col_width, tests_to_run, tests_to_skip={}):

Ditto. Do we need this change?

> Tools/Scripts/webkitpy/port/base.py:190
> +        return [None]

Please add a comment here explaining the reason for return [None] instead of empty list.

[EWS Watchlist]

Comment on attachment 359383 [details]
Patch

Attachment 359383 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10785512

New failing tests:
platform/ipad/media/modern-media-controls/pip-support/pip-support-enabled.html
platform/iphone-7/fast/events/touch/force-press-on-link.html
platform/ipad/fast/forms/choose-select-option.html
platform/iphone-7/fast/events/touch/force-press-event.html
platform/ipad/fast/forms/unfocus-inside-fixed-hittest.html
platform/ipad/media/modern-media-controls/media-documents/media-document-video-ios-sizing.html
platform/ipad/fast/viewport/meta-viewport-ignored.html
platform/ipad/media/modern-media-controls/media-documents/media-document-audio-ios-sizing.html
platform/ipad/media/modern-media-controls/pip-support/pip-support-tap.html
platform/ipad/fast/forms/select-form-run-twice.html
platform/ipad/fast/viewport/width-is-device-width.html
platform/ipad/fast/viewport/empty-meta.html
platform/ipad/media/controls/close-page-with-picture-in-picture-video-assertion-failure.html
platform/ipad/fast/forms/focus-input-via-button.html

[EWS Watchlist]

Created attachment 359398 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Jonathan Bedard]

Created attachment 359433 [details]
Patch

[EWS Watchlist]

Comment on attachment 359433 [details]
Patch

Attachment 359433 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10791131

New failing tests:
platform/ipad/media/modern-media-controls/pip-support/pip-support-tap.html
platform/ipad/media/controls/close-page-with-picture-in-picture-video-assertion-failure.html

[EWS Watchlist]

Created attachment 359444 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Jonathan Bedard]

Created attachment 359448 [details]
Patch

[Aakash Jain]

This seems to break EWS. Bubbles missing. Please investigate.

Also this breaks API tests on iOS: https://ews-build.webkit-uat.org/#/builders/20/builds/220

AttributeError: 'IOSSimulatorPort' object has no attribute 'DEFAULT_DEVICE_TYPE'

API tests on mac seems to work fine: https://ews-build.webkit-uat.org/#/builders/19/builds/356

[Jonathan Bedard]

Committed r240150: <https://trac.webkit.org/changeset/240150>

[Jonathan Bedard]

(In reply to Aakash Jain from comment #34)
> This seems to break EWS. Bubbles missing. Please investigate.
> 
> Also this breaks API tests on iOS:
> https://ews-build.webkit-uat.org/#/builders/20/builds/220
> 
> AttributeError: 'IOSSimulatorPort' object has no attribute
> 'DEFAULT_DEVICE_TYPE'
> 
> API tests on mac seems to work fine:
> https://ews-build.webkit-uat.org/#/builders/19/builds/356

Wish I had seen this before landing.

API tests are an easy fix that I was aware of but forgot to change, landing fix now.

Not sure about the EWS bit, though. This change doesn't touch any EWS bits, unless EWS is pulling from ports in a very unexpected way.

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #36)
> (In reply to Aakash Jain from comment #34)
> > This seems to break EWS. Bubbles missing. Please investigate.
> > 
> > Also this breaks API tests on iOS:
> > https://ews-build.webkit-uat.org/#/builders/20/builds/220
> > 
> > AttributeError: 'IOSSimulatorPort' object has no attribute
> > 'DEFAULT_DEVICE_TYPE'
> > 
> > API tests on mac seems to work fine:
> > https://ews-build.webkit-uat.org/#/builders/19/builds/356
> 
> Wish I had seen this before landing.
> 
> API tests are an easy fix that I was aware of but forgot to change, landing
> fix now.
> 
> Not sure about the EWS bit, though. This change doesn't touch any EWS bits,
> unless EWS is pulling from ports in a very unexpected way.

I think the EWS bubbles is an interesting, although unrelated, bug. The patch in question was uploaded manually, and then I clicked the 'Run EWS bubble' (or something to that effect) that appears. I just uploaded a patch to <https://bugs.webkit.org/show_bug.cgi?id=192164> which has triggered all the EWS bubbles just fine.

[Jonathan Bedard]

Committed r240151: <https://trac.webkit.org/changeset/240151>

[Jonathan Bedard]

Committed r240153: <https://trac.webkit.org/changeset/240153>

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #37)
> ...
> 
> I think the EWS bubbles is an interesting, although unrelated, bug. The
> patch in question was uploaded manually, and then I clicked the 'Run EWS
> bubble' (or something to that effect) that appears. I just uploaded a patch
> to <https://bugs.webkit.org/show_bug.cgi?id=192164> which has triggered all
> the EWS bubbles just fine.

Wrong link: <https://bugs.webkit.org/show_bug.cgi?id=193537>

[Jonathan Bedard]

Committed r240157: <https://trac.webkit.org/changeset/240157>

[Jonathan Bedard]

Committed r240492: <https://trac.webkit.org/changeset/240492>