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.
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 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 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
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.
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.
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 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.
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 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
(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.
(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>
2018-12-10 08:27 PST, Jonathan Bedard
2018-12-10 09:31 PST, EWS Watchlist
2018-12-11 09:20 PST, Jonathan Bedard
2018-12-12 09:57 PST, Jonathan Bedard
2018-12-12 10:10 PST, Jonathan Bedard
2018-12-12 12:11 PST, EWS Watchlist
2018-12-12 15:01 PST, Jonathan Bedard
2018-12-12 16:37 PST, Jonathan Bedard
2018-12-12 19:33 PST, EWS Watchlist
2018-12-13 08:34 PST, Jonathan Bedard
2018-12-13 17:35 PST, Jonathan Bedard
2019-01-14 15:56 PST, Jonathan Bedard
2019-01-17 09:10 PST, Jonathan Bedard
2019-01-17 11:51 PST, EWS Watchlist
2019-01-17 17:36 PST, Jonathan Bedard
2019-01-17 20:29 PST, EWS Watchlist
2019-01-17 22:02 PST, Jonathan Bedard