From http://trac.webkit.org/changeset/122780 (bug 88542), ChromiumAndroidPort doesn't use DriverProxy but uses ChromiumAndroidDriver directly because we don't want DriverProxy to create two drivers for us. ChromiumAndroidDriver switches between pixel and non-pixel mode by itself. However, the code to support virtual test suite is in DriverProxy, so now the feature is broken on Chromium-Android. Dirk, can we move the following code from DriverProxy.run_test() into Driver.run_test()? base = self._port.lookup_virtual_test_base(driver_input.test_name) if base: virtual_driver_input = copy.copy(driver_input) virtual_driver_input.test_name = base virtual_driver_input.args = self._port.lookup_virtual_test_args(driver_input.test_name) return self.run_test(virtual_driver_input) I tried that but it broke some unit tests. I'm wondering if the change would break some ports. If not, I could fix the unit tests.
No, you can't move this into Driver.run_test() because the command line args might change, leading you to need to restart the driver (defeating the point of DriverProxy). However, if you're willing to accept restarts yourself, you can create a ChromiumAndroidDriver that subclasses Driver and override run_test() as needed. The longer-term fix is to support the other command line flags on a per-test basis. Once we get https://bugs.webkit.org/show_bug.cgi?id=92398 to land, we'll have most of the logic in place and then we extend it for the chromium ports (and anyone else that wants to use virtual suites) to support other sets of command line flags.
(In reply to comment #1) > No, you can't move this into Driver.run_test() because the command line args might change, leading you to need to restart the driver (defeating the point of DriverProxy). > > However, if you're willing to accept restarts yourself, you can create a ChromiumAndroidDriver that subclasses Driver and override run_test() as needed. > > The longer-term fix is to support the other command line flags on a per-test basis. > > Once we get https://bugs.webkit.org/show_bug.cgi?id=92398 to land, we'll have most of the logic in place and then we extend it for the chromium ports (and anyone else that wants to use virtual suites) to support other sets of command line flags. Thanks for explanations. I think I'd better make a workaround in our downstream first and wait for the changes about per_test_args landed in upstream.
Created attachment 156735 [details] Patch
(In reply to comment #3) > Created an attachment (id=156735) [details] > Patch Do you still need DriverProxy? Ref tests are now supported with a simple driver instance since the harness pass the pixel test option for each test that need it. Dpranke and I are actually planning to remove it: bug 93269. I hope you can handle the special config of virtual test suites by extending the command line flags of tests.
(In reply to comment #4) > (In reply to comment #3) > > Created an attachment (id=156735) [details] [details] > > Patch > > Do you still need DriverProxy? Ref tests are now supported with a simple driver instance since the harness pass the pixel test option for each test that need it. Dpranke and I are actually planning to remove it: bug 93269. I hope you can handle the special config of virtual test suites by extending the command line flags of tests. The Android port still needs the DriverProxy for the same reason the other Chromium ports do; however, since the Android port still has its own driver, we could just move the retstart-if-command-line-is-different logic directly into the ChromiumAndroidDriver code. As you say, it would be good to update the port to support the other command line args on a per-test basis as well.
Comment on attachment 156735 [details] Patch Will upload a new patch soon.
Created attachment 156760 [details] This patch doesn't use DriverProxy
Created attachment 156766 [details] landing
Comment on attachment 156766 [details] landing Clearing flags on attachment: 156766 Committed r124810: <http://trac.webkit.org/changeset/124810>
All reviewed patches have been landed. Closing bug.