Bug 92515

Summary: [Chromium-Android] Virtual test suites fail
Product: WebKit Reporter: Xianzhu Wang <wangxianzhu>
Component: Tools / TestsAssignee: Xianzhu Wang <wangxianzhu>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, kbalazs, ojan, peter, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Android   
Attachments:
Description Flags
Patch
none
This patch doesn't use DriverProxy
none
landing none

Description Xianzhu Wang 2012-07-27 09:38:44 PDT
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.
Comment 1 Dirk Pranke 2012-07-27 09:50:58 PDT
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.
Comment 2 Xianzhu Wang 2012-07-27 10:05:30 PDT
(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.
Comment 3 Xianzhu Wang 2012-08-06 11:46:23 PDT
Created attachment 156735 [details]
Patch
Comment 4 Balazs Kelemen 2012-08-06 13:20:16 PDT
(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.
Comment 5 Dirk Pranke 2012-08-06 13:37:27 PDT
(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 6 Xianzhu Wang 2012-08-06 14:12:10 PDT
Comment on attachment 156735 [details]
Patch

Will upload a new patch soon.
Comment 7 Xianzhu Wang 2012-08-06 14:37:08 PDT
Created attachment 156760 [details]
This patch doesn't use DriverProxy
Comment 8 Xianzhu Wang 2012-08-06 14:53:27 PDT
Created attachment 156766 [details]
landing
Comment 9 WebKit Review Bot 2012-08-06 16:22:20 PDT
Comment on attachment 156766 [details]
landing

Clearing flags on attachment: 156766

Committed r124810: <http://trac.webkit.org/changeset/124810>
Comment 10 WebKit Review Bot 2012-08-06 16:22:24 PDT
All reviewed patches have been landed.  Closing bug.