Bug 91533

I don't think this is a good idea. reftests are getting increasingly scattered through all the directories. The long term plan we've had on the other ports is to support per-test arguments so you could force pixel testing on for a particular test, and that way you'd only need one DRT.

(In reply to comment #1) > I don't think this is a good idea. reftests are getting increasingly scattered through all the directories. > > The long term plan we've had on the other ports is to support per-test arguments so you could force pixel testing on for a particular test, and that way you'd only need one DRT. It'll be great to switch pixel test on/off in one DRT. However, the issue is significant on Android and I've already created a patch. Can we make it an intermediate solution before we have the pixel-test per-test argument?

(In reply to comment #2) > (In reply to comment #1) > > I don't think this is a good idea. reftests are getting increasingly scattered through all the directories. > > > > The long term plan we've had on the other ports is to support per-test arguments so you could force pixel testing on for a particular test, and that way you'd only need one DRT. > > It'll be great to switch pixel test on/off in one DRT. > > However, the issue is significant on Android and I've already created a patch. Can we make it an intermediate solution before we have the pixel-test per-test argument? if you post the patch I'll be happy to look at it, but I can't guarantee I'll like it :).

Created attachment 152804 [details] Patch for preview

Comment on attachment 152804 [details] Patch for preview View in context: https://bugs.webkit.org/attachment.cgi?id=152804&action=review I guess I can live with this as long as it's optional. Can you file a bug to remove this and also file a bug to explicitly track adding per-test arg support to DRT and NRWT, and add a couple of FIXMEs in the code to note the hack? > Tools/ChangeLog:8 > + Added a command line option '--shard-ref-tests' to group ref tests in dedicated shards. Please add a comment here about why we want to group the tests; you can almost guess form the subject line above but some comment about DriverProxy would probably be helpful.

Out of curiosity, how much is the slowdown? Maybe it would make more sense to have a DriverPool so we can reuse DRT instances.

Created attachment 152823 [details] Patch

Created attachment 152825 [details] Patch

(In reply to comment #8) > Created an attachment (id=152825) [details] > Patch (In reply to comment #5) > (From update of attachment 152804 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152804&action=review > > I guess I can live with this as long as it's optional. Can you file a bug to remove this and also file a bug to explicitly track adding per-test arg support to DRT and NRWT, and add a couple of FIXMEs in the code to note the hack? > Filed bug 91538 and bug 91539. Added FIXME in run_webkit_tests.py parse_args(). > > Tools/ChangeLog:8 > > + Added a command line option '--shard-ref-tests' to group ref tests in dedicated shards. > > Please add a comment here about why we want to group the tests; you can almost guess form the subject line above but some comment about DriverProxy would probably be helpful. Done. (In reply to comment #6) > Out of curiosity, how much is the slowdown? > > Maybe it would make more sense to have a DriverPool so we can reuse DRT instances. Our stat shows that the switching between pixel-test and non-pixel-test occurs about 300 times (total 20~30 minutes) during a full NRWT run. Actually DriverProxy already pools two drivers (one pixel-test and the other non-pixel test) but we don't use it because we don't support multiple native_test activities on Android. Only one driver/DRT instance is allowed on one device. (We will support multiple devices to run layout tests in parallel in the future.)

Comment on attachment 152825 [details] Patch close ... please add a test for this in run_webkit_tests_integrationtest.py so we can be sure the code is exercised by test-webkitpy.

Created attachment 152883 [details] Patch

(In reply to comment #10) > (From update of attachment 152825 [details]) > close ... please add a test for this in run_webkit_tests_integrationtest.py so we can be sure the code is exercised by test-webkitpy. Done.

Comment on attachment 152883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152883&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:62 > + def __init__(self, ref_tests, **kwargs): oh, I meant add an integration test in run_webkit_tests_integrationtest.py that tested that --shard-ref-tests actually ran properly (I don't care as much that the tests are actually sharded in that case, just that we don't error out). This is fine, too, though.

(In reply to comment #13) > (From update of attachment 152883 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152883&action=review > > > Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:62 > > + def __init__(self, ref_tests, **kwargs): > > oh, I meant add an integration test in run_webkit_tests_integrationtest.py that tested that --shard-ref-tests actually ran properly (I don't care as much that the tests are actually sharded in that case, just that we don't error out). > Sorry for misunderstanding that :) > This is fine, too, though. Thanks!

Created attachment 152889 [details] Rebased for landing

Comment on attachment 152889 [details] Rebased for landing Clearing flags on attachment: 152889 Committed r122914: <http://trac.webkit.org/changeset/122914>

All reviewed patches have been landed. Closing bug.

This caused python tests to fail on some bots: http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK1%20%28Tests%29/builds/1026/steps/webkitpy-test/logs/stdio