Bug 192162 - webkitpy: Implement device type specific expected results
Summary: webkitpy: Implement device type specific expected results
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks: 193537
  Show dependency treegraph
 
Reported: 2018-11-29 10:04 PST by Jonathan Bedard
Modified: 2019-01-25 11:56 PST (History)
9 users (show)

See Also:


Attachments
Patch (27.48 KB, patch)
2018-12-10 08:27 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (248.70 KB, application/zip)
2018-12-10 09:31 PST, Build Bot
no flags Details
Directory Approach (84.99 KB, patch)
2018-12-11 09:20 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (72.26 KB, patch)
2018-12-12 09:57 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (72.26 KB, patch)
2018-12-12 10:10 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.52 MB, application/zip)
2018-12-12 12:11 PST, Build Bot
no flags Details
Patch (72.86 KB, patch)
2018-12-12 15:01 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (74.11 KB, patch)
2018-12-12 16:37 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.83 MB, application/zip)
2018-12-12 19:33 PST, Build Bot
no flags Details
Patch (80.58 KB, patch)
2018-12-13 08:34 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (81.34 KB, patch)
2018-12-13 17:35 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (33.17 KB, patch)
2019-01-14 15:56 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (406.86 KB, patch)
2019-01-17 09:10 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (deleted)
2019-01-17 11:51 PST, Build Bot
no flags Details
Patch (416.66 KB, patch)
2019-01-17 17:36 PST, Jonathan Bedard
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (22.27 MB, application/zip)
2019-01-17 20:29 PST, Build Bot
no flags Details
Patch (414.00 KB, patch)
2019-01-17 22:02 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2018-11-29 10:04:11 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-11-29 10:04:51 PST
<rdar://problem/46345449>
Comment 2 Jonathan Bedard 2018-12-10 08:27:25 PST
Created attachment 356960 [details]
Patch
Comment 3 Build Bot 2018-12-10 09:31:43 PST
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.
Comment 4 Build Bot 2018-12-10 09:31:44 PST
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
Comment 5 Jonathan Bedard 2018-12-11 09:20:39 PST
Created attachment 357055 [details]
Directory Approach
Comment 6 Jonathan Bedard 2018-12-12 09:57:06 PST
Created attachment 357148 [details]
Patch
Comment 7 Jonathan Bedard 2018-12-12 10:10:29 PST
Created attachment 357149 [details]
Patch
Comment 8 Build Bot 2018-12-12 12:11:29 PST
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
Comment 9 Build Bot 2018-12-12 12:11:31 PST
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
Comment 10 Jonathan Bedard 2018-12-12 15:01:32 PST
Created attachment 357170 [details]
Patch
Comment 11 Jonathan Bedard 2018-12-12 16:37:24 PST
Created attachment 357194 [details]
Patch
Comment 12 Build Bot 2018-12-12 19:33:10 PST
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
Comment 13 Build Bot 2018-12-12 19:33:22 PST
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
Comment 14 Jonathan Bedard 2018-12-13 08:34:36 PST
Created attachment 357231 [details]
Patch
Comment 15 Jonathan Bedard 2018-12-13 13:33:57 PST
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.
Comment 16 Jonathan Bedard 2018-12-13 17:35:13 PST
Created attachment 357275 [details]
Patch
Comment 17 Jonathan Bedard 2019-01-10 16:35:52 PST
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.
Comment 18 Jonathan Bedard 2019-01-14 15:56:42 PST
Created attachment 359090 [details]
Patch
Comment 19 Build Bot 2019-01-14 15:59:27 PST
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.
Comment 20 Jonathan Bedard 2019-01-14 16:12:21 PST
(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.
Comment 21 WebKit Commit Bot 2019-01-15 09:46:37 PST
Comment on attachment 359090 [details]
Patch

Clearing flags on attachment: 359090

Committed r239989: <https://trac.webkit.org/changeset/239989>
Comment 22 WebKit Commit Bot 2019-01-15 09:46:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Jonathan Bedard 2019-01-17 09:10:20 PST
Reopening to attach new patch.
Comment 24 Jonathan Bedard 2019-01-17 09:10:21 PST
Created attachment 359383 [details]
Patch
Comment 25 Lucas Forschler 2019-01-17 09:58:12 PST
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
Comment 26 Jonathan Bedard 2019-01-17 10:13:52 PST
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.
Comment 27 Aakash Jain 2019-01-17 11:36:06 PST
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.
Comment 28 Build Bot 2019-01-17 11:51:00 PST
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
Comment 29 Build Bot 2019-01-17 11:51:04 PST
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
Comment 30 Jonathan Bedard 2019-01-17 17:36:34 PST
Created attachment 359433 [details]
Patch
Comment 31 Build Bot 2019-01-17 20:29:19 PST
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
Comment 32 Build Bot 2019-01-17 20:29:22 PST
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
Comment 33 Jonathan Bedard 2019-01-17 22:02:27 PST
Created attachment 359448 [details]
Patch
Comment 34 Aakash Jain 2019-01-18 07:05:07 PST
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
Comment 35 Jonathan Bedard 2019-01-18 08:38:01 PST
Committed r240150: <https://trac.webkit.org/changeset/240150>
Comment 36 Jonathan Bedard 2019-01-18 08:44:36 PST
(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.
Comment 37 Jonathan Bedard 2019-01-18 08:54:12 PST
(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.
Comment 38 Jonathan Bedard 2019-01-18 08:54:25 PST
Committed r240151: <https://trac.webkit.org/changeset/240151>
Comment 39 Jonathan Bedard 2019-01-18 09:54:54 PST
Committed r240153: <https://trac.webkit.org/changeset/240153>
Comment 40 Jonathan Bedard 2019-01-18 09:56:20 PST
(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>
Comment 41 Jonathan Bedard 2019-01-18 10:37:13 PST
Committed r240157: <https://trac.webkit.org/changeset/240157>
Comment 42 Jonathan Bedard 2019-01-25 11:56:58 PST
Committed r240492: <https://trac.webkit.org/changeset/240492>