|
Description
Jonathan Bedard
2018-11-29 10:04:11 PST
Created attachment 356960 [details]
Patch
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. 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
Created attachment 357055 [details]
Directory Approach
Created attachment 357148 [details]
Patch
Created attachment 357149 [details]
Patch
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 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
Created attachment 357170 [details]
Patch
Created attachment 357194 [details]
Patch
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 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
Created attachment 357231 [details]
Patch
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. Created attachment 357275 [details]
Patch
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. Created attachment 359090 [details]
Patch
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.
(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 on attachment 359090 [details] Patch Clearing flags on attachment: 359090 Committed r239989: <https://trac.webkit.org/changeset/239989> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 359383 [details]
Patch
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 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 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 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 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
Created attachment 359433 [details]
Patch
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 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
Created attachment 359448 [details]
Patch
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 Committed r240150: <https://trac.webkit.org/changeset/240150> (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. (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. Committed r240151: <https://trac.webkit.org/changeset/240151> Committed r240153: <https://trac.webkit.org/changeset/240153> (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> Committed r240157: <https://trac.webkit.org/changeset/240157> Committed r240492: <https://trac.webkit.org/changeset/240492> |