Bug 91533 – [Chromium-Android] Run ref tests together to avoid expensive driver restarts

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

[Xianzhu Wang]

Driver restarts on Android is expensive (several seconds each time). Switching between normal tests and ref tests requires a driver restart to switch between non-pixel-test mode and pixel-test mode. To reduce the time to run layout tests on Android, we can group all ref tests together.

[Dirk Pranke]

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.

[Xianzhu Wang]

(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?

[Dirk Pranke]

(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 :).

[Xianzhu Wang]

Created attachment 152804 [details]
Patch for preview

[Dirk Pranke]

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.

[Tony Chang]

Out of curiosity, how much is the slowdown?

Maybe it would make more sense to have a DriverPool so we can reuse DRT instances.

[Xianzhu Wang]

Created attachment 152823 [details]
Patch

[Xianzhu Wang]

Created attachment 152825 [details]
Patch

[Xianzhu Wang]

(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.)

[Dirk Pranke]

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.

[Xianzhu Wang]

Created attachment 152883 [details]
Patch

[Xianzhu Wang]

(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.

[Dirk Pranke]

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.

[Xianzhu Wang]

(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!

[Xianzhu Wang]

Created attachment 152889 [details]
Rebased for landing

[WebKit Review Bot]

Comment on attachment 152889 [details]
Rebased for landing

Clearing flags on attachment: 152889

Committed r122914: <http://trac.webkit.org/changeset/122914>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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